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

borrowck: do not suggest to change "&mut self" to "&mut mut self" #33319

Merged
merged 1 commit into from
May 11, 2016

Conversation

birkenfeld
Copy link
Contributor

Matching the snippet string might not be the cleanest, but matching
the AST node instead seems to end in a lot of nested if lets, so I
don't know what's better.

Of course it's entirely possible that there is another API altogether
that I just don't know of?

Fixes #31424.

@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@jseyfried
Copy link
Contributor

This would prevent the following correct suggestion:

struct Struct;

impl Struct {
    fn foo(self: &mut Self) {
        (&mut self).bar();
        //~^ ERROR cannot borrow immutable argument `self` as mutable
        //~| HELP to make the argument mutable, use `mut` as shown:
        //~|    fn foo(mut self: &mut Self) {
    }

    fn bar(&mut self) {}
}

It might be a good idea to match the AST node so that we can continue providing the suggestion in the above case. It would be even better if we could provide the above HELP message even when self is declared as &mut self instead of self: &mut Self.

That being said, I think it's more important to not provide a bad suggestion in the common case of &mut self, so r=me unless you would like to work one of the above improvements.

@birkenfeld
Copy link
Contributor Author

I've looked a bit at what would be involved, and it's tricky: the "pat" node I can get only ever spans the "self" (it's the special pseudo node added by Arg::new_self). The next parent is the whole Item, and the individual Args have no spans. So it'd be a lot of work for this edge case (which is probably quite obscure, since a &mut &mut T is not often necessary).

But I've rebased and added a test, will push that now.

Matching the snippet string might not be the cleanest, but matching
the AST node instead seems to end in a lot of nested `if let`s...

Fixes rust-lang#31424.
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// forbid-output: &mut mut self
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice -- I didn't know about forbid-output

@jseyfried
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 10, 2016

📌 Commit fef8276 has been approved by jseyfried

@bors
Copy link
Contributor

bors commented May 10, 2016

⌛ Testing commit fef8276 with merge 6dbb0e8...

bors added a commit that referenced this pull request May 10, 2016
borrowck: do not suggest to change "&mut self" to "&mut mut self"

Matching the snippet string might not be the cleanest, but matching
the AST node instead seems to end in a lot of nested `if let`s, so I
don't know what's better.

Of course it's entirely possible that there is another API altogether
that I just don't know of?

Fixes #31424.
@bors bors merged commit fef8276 into rust-lang:master May 11, 2016
@birkenfeld birkenfeld deleted the issue31424 branch May 12, 2016 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants