-
Notifications
You must be signed in to change notification settings - Fork 14k
Unconstrained parameter fix #148788
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
base: main
Are you sure you want to change the base?
Unconstrained parameter fix #148788
Conversation
|
This PR modifies |
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| ) | ||
| }; | ||
|
|
||
| diag.multipart_suggestions(msg, suggestions, Applicability::MaybeIncorrect); |
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 don't think we should use a multi-part suggestion for this, these should be multiple distinct suggestions
|
|
||
| // search if the parameter is used in the impl body | ||
| let mut visitor = ParamUsageVisitor { | ||
| tcx, // Pass the TyCtxt |
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.
| tcx, // Pass the TyCtxt | |
| tcx, |
| LL - unsafe impl<Q: Trait> Send for Inner {} | ||
| LL + unsafe impl Send for Inner {} | ||
| | | ||
| LL | unsafe impl<Q: Trait> Send for Inner<Q> {} |
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.
We should only suggest this if it could be correct (maybe by checking if the self type has generic parameters), or we should be very clear that it's just illustrative:
help: if `Inner` takes generic parameters, consider instantiating it with `Q` to constrain `Q`
|
LL - unsafe impl<Q: Trait> Send for Inner {}
LL + unsafe impl<Q: Trait> Send for Inner<Q> {}
| ^^^ `Q` could be used here
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.
Well there would another error if the Inner struct took a generic parameter. I made a similar example here:
struct Foo<N> {x: N}
impl<N> Foo {}which obviously gives the error, that the generic parameter N should be provided for the implementation of Foo:
error[E0107]: missing generics for struct `Foo`
--> src/lib.rs:3:9
|
3 | impl<N> Foo {}
| ^^^ expected 1 generic argument
|
note: struct defined here, with 1 generic parameter: `N`
--> src/lib.rs:1:8
|
1 | struct Foo<N> {x: N}
| ^^^ -
help: add missing generic argument
|
3 | impl<N> Foo<N> {}
| +++
For more information about this error, try `rustc --explain E0107`.Therefore I would even suggest never suggesting to add the generic argument in general or did I miss some edgecase?
Or we could suggest adding it as a type parameter to the struct and to the impl
| | | ||
| help: make use of the lifetime parameter `'a` in the `self` type | ||
| | | ||
| LL | impl<'a> Foo<fn(&()), 'a> { |
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.
Similarly to above, we should make sure this is correct or clearly indicate that it is purely illustrative.
| self.tcx | ||
| } | ||
|
|
||
| /// We use `ControlFlow` to stop visiting as soon as we find what we're looking for. |
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.
Some of the comments in this impl feel redundant
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
This PR is an attempt to solve the issue described in the issue #107295