Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not eagerly reject inference vars when trying to resolve method calls. #126316

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 12, 2024

Reintroduces the changes reverted in #126258 and fixes the regression in the old solver.

The new solver will still fail on

pub trait Parser<E> {
    fn parse(&self) -> E;
}

impl<E, T: Fn() -> E> Parser<E> for T {
    fn parse(&self) -> E {
        self()
    }
}

pub fn recursive_fn<E>() -> impl Parser<E> {
    move || recursive_fn().parse()
    //[next]~^ ERROR: type annotations needed
}

because while type checking the method call .parse(), when looking for candidates, we end up not finding any, as the only information we have is that there's an unresolved AliasTy(opaque, infer_var) floating around while looking for a method call on infer_var.

This obviously seems fixable by looking at said obligation, but I'd prefer to tackle that separately (I got half an impl locally, but there are multiple paths forward that we should discuss)

r? @lcnr @compiler-errors

I'm going to split out the effect-free cleanups and the inference changes to separate PRs, so we can review them in isolation, but with the goal of enabling this PR to land

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 12, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 12, 2024
…r-errors

Avoid a bunch of booleans in favor of Result<(), ErrorGuaranteed> as that more robustly proves that an error has been emitted

pulled out of rust-lang#126316

This PR cannot have any effect on compilation.
All it does is shift a `Ty::new_misc_error` to a `span_delayed_bug` and preserve the `ErrorGuaranteed` in all other cases
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
Rollup merge of rust-lang#126317 - oli-obk:recursive_rpit4, r=compiler-errors

Avoid a bunch of booleans in favor of Result<(), ErrorGuaranteed> as that more robustly proves that an error has been emitted

pulled out of rust-lang#126316

This PR cannot have any effect on compilation.
All it does is shift a `Ty::new_misc_error` to a `span_delayed_bug` and preserve the `ErrorGuaranteed` in all other cases
@bors

This comment was marked as outdated.

@bors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@oli-obk

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to take some time to look at when exactly we don't error if there's an auto-deref step resulting in an infer var, will get to this next week hopefully

@bors

This comment was marked as outdated.

@bors

This comment was marked as outdated.

@bors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the recursive_rpit3 branch 4 times, most recently from 39dd4bd to 6aa66e4 Compare December 3, 2024 11:35
@@ -1,31 +1,15 @@
error[E0282]: type annotations needed
--> $DIR/hidden-type-is-opaque-2.rs:10:17
error[E0599]: no method named `reify_as` found for type `_` in the current scope
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors like this are pretty useless and should be fixed before landing the PR

Comment on lines 9 to 13
pub fn parse() -> Option<Vec<()>> {
let iter = std::iter::once(Some(())).map(|o| o.map(Into::into));
let items = iter.collect::<Option<Box<_>>>()?; //~ ERROR E0282
//~^ NOTE this is an inference error on crate `time` caused by an API change in Rust 1.80.0; update `time` to version `>=0.3.35`
let items = iter.collect::<Option<Box<_>>>()?;
Some(items.into())
//~^ NOTE type must be known at this point
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes the time inference regression caused by #99969

I can see how that happens, but it's best effort, as with most inference errors or fixes we can't make long term guarantees...

Comment on lines 8 to 12
fn shines_a_beacon_through_the_darkness() {
let x: Option<_> = None; //~ ERROR type annotations needed
let x: Option<_> = None;
x.unwrap().method_that_could_exist_on_some_type();
//~^ ERROR no method named `method_that_could_exist_on_some_type` found for type `_`
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per the comment above, this diagnostic needs to be fixed again

@bors
Copy link
Contributor

bors commented Dec 9, 2024

☔ The latest upstream changes (presumably #134052) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants