-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix ignored_unit_patterns: include refs in the suggestion
#15578
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: master
Are you sure you want to change the base?
Conversation
|
Lintcheck changes for 4aaa667
This comment will be updated if you push new changes |
| let x: (usize, &&&&&()) = (1, &&&&&&()); | ||
| match x { | ||
| (1, ()) => unimplemented!(), | ||
| (1, &&&&&()) => unimplemented!(), |
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.
Hm, seems like matching () is also valid and the &s are not required. Does that also work (without compiler error) in closures?
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 it depends. You're right that the refs are not required in match arms, and they are not required in closures most of the time. But as #15187 shows, it stops working when you try casting the closure to impl Fn(&()), for example.
The solution I tried first was to eliminate all the cases where we know the refs aren't needed (e.g. all match arm patterns), but it now looks like the only case where they are needed are the situations where a closure accepting &() later gets coerced to fn(&())/impl Fn*(&()). Therefore, we could theoretically try catching only those cases, but that will probably require writing some kind of a HIR visitor, and those things are kind of intimidating to me tbh...
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.
Imo the first solution (don't add refs when suggesting for match arms, but do add them for all closure params) is a good balance of simplicity and correctness
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 it's not even that!
-
casting/coercing the closure to both an
fnand aFn*trait object works actually just fine -
changing anything about the issue's reproducer, like:
- using the type directly instead of the
LifetimeBoundtype alias - replacing
dynwithimplin the type alias
either stops the thing from compiling, or makes it so that
()starts getting accepted as well - using the type directly instead of the
I guess I'm not well-versed enough in the type system shenanigans to pinpoint the combination of factors that makes this exact expression require &() instead of (). If you have any ideas, let me know (and let me know if you know a way I could test for said factors) -- otherwise, we might want to just close the issue as WONTFIX, given how specific it is?..
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.
@flip1995 friendly ping
Fixes #15187
changelog: [
ignored_unit_patterns]: include refs in the suggestionr? @flip1995