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

When annotations needed, look at impls for more accurate suggestions #128653

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

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Aug 4, 2024

When encountering an expression that needs type annotations, if we have the trait DefId we look for all the impls that could be satisfied by the expression we have (without looking at additional obligations) and suggest the fully qualified to specific impls. For left over type parameters, we replace them with _.

error[E0283]: type annotations needed
  --> $DIR/E0283.rs:35:24
   |
LL |     let bar = foo_impl.into() * 1u32;
   |                        ^^^^
   |
note: multiple `impl`s satisfying `Impl: Into<_>` found
  --> $DIR/E0283.rs:17:1
   |
LL | impl Into<u32> for Impl {
   | ^^^^^^^^^^^^^^^^^^^^^^^
   = note: and another `impl` found in the `core` crate:
           - impl<T, U> Into<U> for T
             where U: From<T>;
help: try using a fully qualified path to specify the expected types
   |
LL |     let bar = <_ as Into<_>>::into(foo_impl) * 1u32;
   |               +++++++++++++++++++++        ~
help: try using a fully qualified path to specify the expected types
   |
LL |     let bar = <Impl as Into<u32>>::into(foo_impl) * 1u32;
   |               ++++++++++++++++++++++++++        ~

When we encounter a blanket <Ty as Into<Other> impl, look at the From impls so that we can suggest the appropriate Other. We also filter the impl that we'll suggest if the predicate originating this inference chain was a projection for the Output associated type of a math trait:

error[E0284]: type annotations needed
  --> $DIR/issue-70082.rs:7:33
   |
LL |     let y: f64 = 0.01f64 * 1i16.into();
   |                          -      ^^^^
   |                          |
   |                          type must be known at this point
   |
   = note: cannot satisfy `<f64 as Mul<_>>::Output == f64`
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<i64>>::into(1i16);
   |                            +++++++++++++++++++++++++    ~

This approach is not generalized for for blanket-impls, so we can end up with suggestions like <_ as Trait<_>>::foo(bar), but at least we don't end up with <_ as Into<_>::into(bar) most of the time. It'd be nice to have a more complete mechanism that does account for all obligations when resolving methods.

@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 4, 2024
Comment on lines 644 to 645
let mut printer = fmt_printer(self, Namespace::ValueNS);
printer.print_def_path(assoc.def_id, identity_method).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Also, that whole for_suggestion hack is not necessary. You should just rebase these substs onto the trait's associated item (which you can access via assoc_item.trait_item_def_id), and print that, since you're really just printing a method path.

Copy link
Member

@compiler-errors compiler-errors Aug 4, 2024

Choose a reason for hiding this comment

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

Also, it's probably worthwhile to do this inside of a probe, and instead of doing can_eq just make an ObligationCtxt and use eq + select_where_possible so the infer vars are constrained correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@compiler-errors I'm still to add the probe, but otherwise comments are addressed. I'll do another round of clean up, but I'm intrigued if you think e302e7f is worth it.

@estebank
Copy link
Contributor Author

estebank commented Aug 4, 2024

@compiler-errors thanks for all the comments! While I have you here, do you think there would be any maintainable way of supporting the general case for "indirect impls that would apply" (like the relationship that Into and From have)? I'm ok with special casing From and TryFrom, but it'd be amazing if we could do it "the right way" instead.

@compiler-errors
Copy link
Member

@estebank: No. I thought about it a bit over the last hour, and cannot think of a way of enumerating such "two-step" impls given the way that the trait system works. I'm OK with just special-casing From and TryFrom -- though, please try your best to make it abstract enough to avoid code duplication.

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2024
@estebank
Copy link
Contributor Author

estebank commented Aug 4, 2024

please try your best to make it abstract enough to avoid code duplication.

Yep, this is effectively a draft, I'll clean it up before it's ready to merge. This was more of a "will this work?" experiment to begin with and published so I could ask you about some of these details in approach.

@estebank estebank force-pushed the ambiguity-suggestion-2 branch from 8020c4c to eeeae60 Compare August 5, 2024 21:48
}

let filter = if let Some(ty::ProjectionPredicate {
projection_term: ty::AliasTerm { def_id, .. },
Copy link
Member

Choose a reason for hiding this comment

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

How does this DefId differ from the one passed as an argument?

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 is the DefId of an associated type (like in the tests, Mul::Output, while the other def_id is the one for the method (in the same test, <i16 as Into<_>::into(..)).

@estebank estebank force-pushed the ambiguity-suggestion-2 branch 2 times, most recently from a158bbc to ac19dee Compare August 8, 2024 17:48
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 8, 2024
@fee1-dead
Copy link
Member

r? compiler

@rustbot rustbot assigned TaKO8Ki and unassigned fee1-dead Aug 10, 2024
let impl_trait_ref =
tcx.impl_trait_ref(impl_def_id).unwrap().instantiate(tcx, impl_args);
let impl_self_ty = impl_trait_ref.self_ty();
if self.infcx.can_eq(param_env, impl_self_ty, self_ty) {
Copy link
Member

Choose a reason for hiding this comment

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

Use an ObligationCtxt and a probe

Comment on lines 733 to 832
if tcx.is_diagnostic_item(sym::blanket_into_impl, impl_def_id)
&& let Some(did) = tcx.get_diagnostic_item(sym::From)
{
let mut found = false;
tcx.for_each_impl(did, |impl_def_id| {
// We had an `<A as Into<B>::into` and we've hit the blanket
// impl for `From<A>`. So we try and look for the right `From`
// impls that *would* apply. We *could* do this in a generalized
// version by evaluating the `where` clauses, but that would be
// way too involved to implement. Instead we special case the
// arguably most common case of `expr.into()`.
let Some(header) = tcx.impl_trait_header(impl_def_id) else {
return;
};
let target = header.trait_ref.skip_binder().args.type_at(0);
if filter.is_some() && filter != Some(target) {
return;
};
let target = header.trait_ref.skip_binder().args.type_at(0);
let ty = header.trait_ref.skip_binder().args.type_at(1);
if ty == self_ty {
if target_type {
let mut ty_str = format!("{target}");
if &ty_str == "_" {
ty_str = "/* Type */".to_string();
}
paths.push(ty_str);
} else {
paths.push(format!("<{self_ty} as Into<{target}>>::into"));
}
found = true;
}
});
if found {
return;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This needs to live in a different method. It's way too deeply nested.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2024
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 19, 2024
@compiler-errors

This comment was marked as resolved.

@estebank estebank force-pushed the ambiguity-suggestion-2 branch from ac19dee to adc3ea6 Compare August 20, 2024 23:01
@bors

This comment was marked as outdated.

@estebank estebank force-pushed the ambiguity-suggestion-2 branch from adc3ea6 to 50f81d2 Compare August 30, 2024 01:04
@traviscross traviscross added the A-diagnostics Area: Messages for errors, warnings, and lints label Sep 29, 2024
When encountering an expression that needs type annotations, if we have the trait `DefId` we look for all the `impl`s that could be satisfied by the expression we have (without looking at additional obligations) and suggest the fully qualified to specific impls. For left over type parameters, we replace them with `_`.

```
error[E0283]: type annotations needed
  --> $DIR/E0283.rs:35:24
   |
LL |     let bar = foo_impl.into() * 1u32;
   |                        ^^^^
   |
note: multiple `impl`s satisfying `Impl: Into<_>` found
  --> $DIR/E0283.rs:17:1
   |
LL | impl Into<u32> for Impl {
   | ^^^^^^^^^^^^^^^^^^^^^^^
   = note: and another `impl` found in the `core` crate:
           - impl<T, U> Into<U> for T
             where U: From<T>;
help: try using a fully qualified path to specify the expected types
   |
LL |     let bar = <_ as Into<_>>::into(foo_impl) * 1u32;
   |               +++++++++++++++++++++        ~
help: try using a fully qualified path to specify the expected types
   |
LL |     let bar = <Impl as Into<u32>>::into(foo_impl) * 1u32;
   |               ++++++++++++++++++++++++++        ~
```

This approach does not account for blanket-impls, so we can end up with suggestions like `<_ as Into<_>>::into(foo)`. It'd be nice to have a more complete mechanism that does account for all obligations when resolving methods.

Do not suggest `path::to<impl Trait for Type>::method`

In the pretty-printer, we have a weird way to display fully-qualified for non-local impls, where we show a regular path, but the section corresponding to the `<Type as Trait>` we use non-syntax for it like `path::to<impl Trait for Type>`. It should be `<Type for path::to::Trait>`, but this is only better when we are printing code to be suggested, not to find where the `impl` actually is, so we add a new flag to the printer for this.

Special case `Into` suggestion to look for `From` `impl`s

When we encounter a blanket `<Ty as Into<Other>` `impl`, look at the `From` `impl`s so that we can suggest the appropriate `Other`:

```
error[E0284]: type annotations needed
  --> $DIR/issue-70082.rs:7:33
   |
LL |     let y: f64 = 0.01f64 * 1i16.into();
   |                          -      ^^^^
   |                          |
   |                          type must be known at this point
   |
   = note: cannot satisfy `<f64 as Mul<_>>::Output == f64`
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<i32>>::into(1i16);
   |                            +++++++++++++++++++++++++    ~
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<i64>>::into(1i16);
   |                            +++++++++++++++++++++++++    ~
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<i128>>::into(1i16);
   |                            ++++++++++++++++++++++++++    ~
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<isize>>::into(1i16);
   |                            +++++++++++++++++++++++++++    ~
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<f32>>::into(1i16);
   |                            +++++++++++++++++++++++++    ~
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<f64>>::into(1i16);
   |                            +++++++++++++++++++++++++    ~
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<AtomicI16>>::into(1i16);
   |                            +++++++++++++++++++++++++++++++    ~
```

Suggest an appropriate type for a binding of method chain

Do the same we do with fully-qualified type suggestions to the suggestion to specify a binding type:

```
error[E0282]: type annotations needed
  --> $DIR/slice-pattern-refutable.rs:14:9
   |
LL |     let [a, b, c] = Zeroes.into() else {
   |         ^^^^^^^^^
   |
help: consider giving this pattern a type
   |
LL |     let [a, b, c]: _ = Zeroes.into() else {
   |                  +++
help: consider giving this pattern a type
   |
LL |     let [a, b, c]: [usize; 3] = Zeroes.into() else {
   |                  ++++++++++++
```

review comments

 - Pass `ParamEnv` through
 - Remove now-unnecessary `Formatter` mode
 - Rework the way we pick up the bounds

Add naïve mechanism to filter `Into` suggestions involving math ops

```
error[E0284]: type annotations needed
  --> $DIR/issue-70082.rs:7:33
   |
LL |     let y: f64 = 0.01f64 * 1i16.into();
   |                          -      ^^^^
   |                          |
   |                          type must be known at this point
   |
   = note: cannot satisfy `<f64 as Mul<_>>::Output == f64`
help: try using a fully qualified path to specify the expected types
   |
LL |     let y: f64 = 0.01f64 * <i16 as Into<f64>>::into(1i16);
   |                            +++++++++++++++++++++++++    ~
```

Note that we only suggest `Into<f64>`, and not `Into<i32>`, `Into<i64>`, `Into<i128>`, `Into<isize>`, `Into<f32>` or `Into<AtomicI16>`.

Replace `_` with `/* Type */` in let binding type suggestion

Rework the `predicate` "trafficking" to be more targetted

Rename `predicate` to `originating_projection`. Pass in only the `ProjectionPredicate` instead of the `Predicate` to avoid needing to destructure as much.
We will usually have a better, more specific, alternative suggestion.
@estebank estebank force-pushed the ambiguity-suggestion-2 branch from 50f81d2 to 78f1303 Compare November 6, 2024 00:51
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2024
@bors
Copy link
Contributor

bors commented Dec 19, 2024

☔ The latest upstream changes (presumably #134499) 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
A-diagnostics Area: Messages for errors, warnings, and lints 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants