-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Mut on ident patterns. #10026
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
Mut on ident patterns. #10026
Conversation
Would this be nicer as |
I have a feeling that way back when we used to infer what the kind of binding was (maybe it was inferred to be by reference or by value?), but now we don't have that any more. I agree with @huonw that we should probably just replace it with Also, would you mind expending the |
@huonw, @alexcrichton: Done and done. |
let lo = self.span.lo; | ||
let pat = self.parse_pat(); | ||
|
||
if is_mutbl && !ast_util::pat_is_ident(pat) { | ||
self.obsolete(*self.span, ObsoleteMutWithMultipleBindings) |
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.
Is ObsoleteMutWithMultipleBindings
unused after this removal?
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.
Yep, and now gone!
The only thing holding me back from an r+ on this is that I don't fully understand mem_categorization and friends dealing with the analysis of mutable variables. It looks correct to me, but I can't quite say for sure. If someone else who understand those portions wants to r+, sounds good to me. Otherwise, if you add some tests with errors about mutable matches, I'd r+ this. Things like you can take a mutable loan out of a binding, but you can't do it twice, or moving out and then re-assigning, etc. |
I'll take a look |
Overall this looks reasonably good to me except that the tests look insufficient. I'd like to see various compile-fail and run-pass tests covering various weird cases like |
I'm glad to see this landed, but I feel like the tests are still insufficient. There are lots and lots of edge cases here and I don't see much coverage. I also see no coverage of the possible interactions with the borrow checker and so on. I'll open a bug with some suggestions for tests we need. |
Add missing slash to produce function documentation changelog: none
Fixes #9792.