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

Make need_type_info_err more conservative #73027

Merged
merged 2 commits into from
Jun 20, 2020
Merged

Make need_type_info_err more conservative #73027

merged 2 commits into from
Jun 20, 2020

Conversation

doctorn
Copy link
Contributor

@doctorn doctorn commented Jun 5, 2020

Makes sure arg patterns we are going to suggest on are actually contained within the span of the obligation that caused the inference error (credit to @lcnr for suggesting this fix).

There's a subtle trade-off regarding the handling of local patterns which I've left a comment about.

Resolves #72690

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2020
@doctorn
Copy link
Contributor Author

doctorn commented Jun 5, 2020

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned varkor Jun 5, 2020
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.

LGTM.

src/librustc_infer/infer/error_reporting/need_type_info.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I need to look a bit deeper into this PR to confirm it is ok to merge. It certainly fixes some cases, but others are still present. I think we might be able to come up with a more robust way of handling this.

Ideally we would also be able to handle let _ = "x".as_ref(); with a suggestion to give _ a type (which we don't today).

I think that we might be able to fix this by changing how the visitor works to try and keep into account some kind of inference chain, but again, I'm not sure how that would work.

Comment on lines 1 to +5
error[E0282]: type annotations needed
--> $DIR/expect-two-infer-vars-supply-ty-with-bound-region.rs:8:27
--> $DIR/expect-two-infer-vars-supply-ty-with-bound-region.rs:8:5
|
LL | with_closure(|x: u32, y| {});
| ^ consider giving this closure parameter a type
| ^^^^^^^^^^^^ cannot infer type for type parameter `B` declared on the function `with_closure`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically a regression, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the cost of making the suggestion more conservative. The point is at the moment we're struggling with things like this playground which urgently requires attention

Comment on lines +1 to +10
error[E0282]: type annotations needed for the closure `fn(Expr<'_, _>) -> Expr<'_, _>`
--> $DIR/issue-23046.rs:18:9
|
LL | let ex = |x| {
| ^ consider giving this closure parameter the explicit type `Expr<'_, VAR>`, where the type parameter `VAR` is specified
LL | let_(add(x,x), |y| {
| ^^^^ cannot infer type for type parameter `VAR` declared on the function `let_`
|
help: give this closure an explicit return type without `_` placeholders
|
LL | let_(add(x, x), |x|-> Expr<'_, _> { x })})};
| ^^^^^^^^^^^^^^^^ ^
Copy link
Contributor

Choose a reason for hiding this comment

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

...so it could be argued is this (but inference works with either suggestion, so this is acceptable).

Comment on lines +51 to +60
error[E0283]: type annotations needed for `std::string::String`
--> $DIR/issue-72690.rs:41:5
|
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
LL | let _ = String::from("x");
| - consider giving this pattern a type
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to still give an incorrect suggestion.

Copy link
Contributor Author

@doctorn doctorn Jun 5, 2020

Choose a reason for hiding this comment

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

For these two incorrect suggestions the immediate solution is to require that local.span contains target_span, but doing so causes a huge number of other regressions - hence the trade-off discussed in the comment I added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(There's some discussion in this thread)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just looked at the thread. I would feel better about merging this PR as is if we had someone focusing on adding better tracking to ObligationCause. I already did a lot there for obligations in the type namespace but won't have time to look at it in value namespace. Would you be interested in digging into doing that?

Copy link
Contributor Author

@doctorn doctorn Jun 10, 2020

Choose a reason for hiding this comment

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

Yeah definitely! If you send me some details about what needs doing/where to start I can get on that right away

Comment on lines +62 to +71
error[E0283]: type annotations needed for `std::string::String`
--> $DIR/issue-72690.rs:47:5
|
LL | let _ = String::from("x");
| - consider giving this pattern a type
LL | String::from("x".as_ref());
| ^^^^^^^^^^^^ cannot infer type for struct `std::string::String`
|
= note: cannot satisfy `std::string::String: std::convert::From<&_>`
= note: required by `std::convert::From::from`
Copy link
Contributor

Choose a reason for hiding this comment

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

...and this.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 19, 2020

📌 Commit b4ddd91 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2020
…arth

Rollup of 16 pull requests

Successful merges:

 - rust-lang#71420 (Specialization is unsound)
 - rust-lang#71899 (Refactor `try_find` a little)
 - rust-lang#72689 (add str to common types)
 - rust-lang#72791 (update coerce docs and unify relevant tests)
 - rust-lang#72934 (forbid mutable references in all constant contexts except for const-fns)
 - rust-lang#73027 (Make `need_type_info_err` more conservative)
 - rust-lang#73347 (Diagnose use of incompatible sanitizers)
 - rust-lang#73359 (shim.rs: avoid creating `Call` terminators calling `Self`)
 - rust-lang#73399 (Clean up E0668 explanation)
 - rust-lang#73436 (Clean up E0670 explanation)
 - rust-lang#73440 (Add src/librustdoc as an alias for src/tools/rustdoc)
 - rust-lang#73442 (pretty/mir: const value enums with no variants)
 - rust-lang#73452 (Unify region variables when projecting associated types)
 - rust-lang#73458 (Use alloc::Layout in DroplessArena API)
 - rust-lang#73484 (Update the doc for std::prelude to the correct behavior)
 - rust-lang#73506 (Bump Rustfmt and RLS)

Failed merges:

r? @ghost
@bors bors merged commit 2d1bd57 into rust-lang:master Jun 20, 2020
SNCPlay42 added a commit to SNCPlay42/rust that referenced this pull request Oct 19, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 20, 2020
Trait predicate ambiguities are not always in `Self`

When reporting ambiguities in trait predicates, the compiler incorrectly assumed the ambiguity was always in the type the trait should be implemented on, and never the generic parameters of the trait. This caused silly suggestions for predicates like `<KnownType as Trait<_>>`, such as giving explicit types to completely unrelated variables that happened to be of type `KnownType`.

This also reverts rust-lang#73027, which worked around this issue in some cases and does not appear to be necessary any more.

fixes rust-lang#77982
fixes rust-lang#78055
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Type annotations needed" error is shown in a misleading wrong location
7 participants