-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add a note when suggesting to use Impl Trait #112088
Conversation
@davidtwco Requesting your review as you reviewed the previous PR here #104755 Thanks in advance |
Did you bless tests or are there no UI tests affected by this change? |
This comment has been minimized.
This comment has been minimized.
@compiler-errors Oh yeah I did a |
This comment has been minimized.
This comment has been minimized.
Add tests Update compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs Co-authored-by: Lukas <lukas-code@outlook.com> Fixed tests
a326e06
to
0bba4e5
Compare
@@ -856,6 +856,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
format!("impl {}", all_bounds_str), | |||
Applicability::MaybeIncorrect, | |||
); | |||
err.note(format!("the type of `{}` is chosen by the caller", expected_ty_as_param.name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this note adds much value in this context. Not exactly sure why, but something about the ordering or placement of this note feels to me like it's missing context.
In the tests/ui/return/return-impl-trait-bad.stderr
stderr below:
LL | fn used_in_trait<T>() -> T
| - -
| | |
| | expected `T` because of return type
| | help: consider using an impl return type: `impl Send`
| this type parameter
My sense would be that "this type parameter" label could be replaced with the text above... Not sure how hard that is though.
Alternatively, "the type of {}
is chosen by the caller" should probably be extended to explain its significance. Maybe something along the lines of "and may be a type that is different than {(the other type mentioned)}"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the message to explain its significance. However finding exactly where "this type parameter" is derived from and put into the message here is a bit hard. Do let me know if this is sufficient, or if not some more context on how I can find the construction of the above "this type parameter"
@@ -53,6 +53,7 @@ LL | "don't suggest this, because the generic param is used in the bound." | |||
| | |||
= note: expected type parameter `T` | |||
found reference `&'static str` | |||
= note: the type of `T` is chosen by the caller and may be a type that is different than `T` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be a type that is different than
T
Hmm... typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was not clear with what I meant here.
the type of
T
is chosen by the caller and may be a type that is different thanT
This sentence mentions T
twice, which does not make sense. The other type in this error is &'static str
, which should be mentioned.
The job Click to see the possible cause of the failure (guessed by this bot)
|
@@ -856,6 +856,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
format!("impl {}", all_bounds_str), | |||
Applicability::MaybeIncorrect, | |||
); | |||
|
|||
let ty::Param(found_ty_as_param) = found.kind() else { return }; | |||
err.note(format!("the type of `{}` is chosen by the caller and may be a type that is different from `{}`", expected_ty_as_param.name, found_ty_as_param.name)); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@compiler-errors Here found_ty_as_param should return the parameter found right, or am I understanding something wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need found_ty_as_param
-- the found ty might be any type, and you can just render that to string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err.note(format!("the type of `{}` is chosen by the caller and may be a type that is different from `{}` ", expected_ty_as_param.name, found.to_string()));
Converting found to string still does not return the type expected.
@rustbot author |
@sladyn98 any updates on this? |
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
…Trait Revives rust-lang#112088 and rust-lang#104755. Co-authored-by: sladynnunes <snunes@usc.edu> Co-authored-by: Thalia Nero <chrisvn00@gmail.com>
…Trait Revives rust-lang#112088 and rust-lang#104755. Co-authored-by: sladynnunes <snunes@usc.edu> Co-authored-by: Thalia Nero <chrisvn00@gmail.com>
…Trait Revives rust-lang#112088 and rust-lang#104755. Co-authored-by: sladynnunes <snunes@usc.edu> Co-authored-by: Thalia Nero <chrisvn00@gmail.com>
…Trait Revives rust-lang#112088 and rust-lang#104755. Co-authored-by: sladynnunes <snunes@usc.edu> Co-authored-by: Thalia Nero <chrisvn00@gmail.com>
Note that the caller chooses a type for type param ``` error[E0308]: mismatched types --> $DIR/return-impl-trait.rs:23:5 | LL | fn other_bounds<T>() -> T | - - | | | | | expected `T` because of return type | | help: consider using an impl return type: `impl Trait` | expected this type parameter ... LL | () | ^^ expected type parameter `T`, found `()` | = note: expected type parameter `T` found unit type `()` = note: the caller chooses the type of T which can be different from () ``` Tried to see if "expected this type parameter" can be replaced, but that goes all the way to `rustc_infer` so seems not worth the effort and can affect other diagnostics. Revives rust-lang#112088 and rust-lang#104755.
Rollup merge of rust-lang#122195 - jieyouxu:impl-return-note, r=fmease Note that the caller chooses a type for type param ``` error[E0308]: mismatched types --> $DIR/return-impl-trait.rs:23:5 | LL | fn other_bounds<T>() -> T | - - | | | | | expected `T` because of return type | | help: consider using an impl return type: `impl Trait` | expected this type parameter ... LL | () | ^^ expected type parameter `T`, found `()` | = note: expected type parameter `T` found unit type `()` = note: the caller chooses the type of T which can be different from () ``` Tried to see if "expected this type parameter" can be replaced, but that goes all the way to `rustc_infer` so seems not worth the effort and can affect other diagnostics. Revives rust-lang#112088 and rust-lang#104755.
Note that the caller chooses a type for type param ``` error[E0308]: mismatched types --> $DIR/return-impl-trait.rs:23:5 | LL | fn other_bounds<T>() -> T | - - | | | | | expected `T` because of return type | | help: consider using an impl return type: `impl Trait` | expected this type parameter ... LL | () | ^^ expected type parameter `T`, found `()` | = note: expected type parameter `T` found unit type `()` = note: the caller chooses the type of T which can be different from () ``` Tried to see if "expected this type parameter" can be replaced, but that goes all the way to `rustc_infer` so seems not worth the effort and can affect other diagnostics. Revives rust-lang#112088 and rust-lang#104755.
Add a note to E308 explaining that generic type variables are chosen by the caller rather than the implementer, when suggesting to use impl Trait instead of a generic type variable.
This PR completes the work left here #104755
r? rust-lang/diagnostics