NLL fails to suggest "try removing `&mut` here" #54720

davidtwco · 2018-10-01T17:26:02Z

This PR adds try removing `&mut` here suggestions to functions where a mutable borrow is being taken of a &mut self or a self: &mut Self. This PR also enables the suggestion for adding a mut pattern to by-value implicit self arguments without mut patterns already.

r? @nikomatsakis

src/librustc/ich/impls_hir.rs

src/librustc_mir/borrow_check/mutability_errors.rs

nikomatsakis · 2018-10-01T20:58:40Z

r=me but the incremental tests are unhappy. Looks like the test just needs to be updated though. You probably just need to change

#[cfg(not(cfail1))]
#[rustc_clean(label="Hir", cfg="cfail2")]
#[rustc_clean(label="Hir", cfg="cfail3")]
trait TraitChangeModeSelfOwnToMut: Sized {
    #[rustc_clean(label="Hir", cfg="cfail2")]
    #[rustc_clean(label="Hir", cfg="cfail3")]
    #[rustc_dirty(label="HirBody", cfg="cfail2")]
    #[rustc_clean(label="HirBody", cfg="cfail3")]
    fn method(mut self) {}
}

to

#[cfg(not(cfail1))]
#[rustc_clean(label="Hir", cfg="cfail2")]
#[rustc_clean(label="Hir", cfg="cfail3")]
trait TraitChangeModeSelfOwnToMut: Sized {
    #[rustc_dirty(label="Hir", cfg="cfail2")] // changed!
    #[rustc_clean(label="Hir", cfg="cfail3")]
    #[rustc_dirty(label="HirBody", cfg="cfail2")]
    #[rustc_clean(label="HirBody", cfg="cfail3")]
    fn method(mut self) {}
}

In particular, in the old system, the Hir didn't reflect the mut in the signature, but now it does.

This commit adds an `ImplicitSelfKind` to the HIR and the MIR that keeps track of whether a implicit self argument is immutable by-value, mutable by-value, immutable reference or mutable reference so that the addition of the `mut` keyword can be suggested for the immutable by-value case.

This commit improves mutability error suggestions by suggesting the removal of `&mut` where a mutable borrow is being taken of a `&mut self` or a `self: &mut Self`.

estebank · 2018-10-01T23:04:52Z

src/librustc_mir/borrow_check/mutability_errors.rs

+                } =>
+            {
+                err.span_label(span, format!("cannot {ACT}", ACT = act));
+                err.span_label(span, "try removing `&mut` here");


I believe that if we use local_decl.source_info.span here it would point at &mut self instead of (&mut self), which would let you make it a machine applicable suggestion instead of a label. Changing this will likely require you to change the above condition, but marginally so.

I might be misunderstanding but, wouldn't local_decl.source_info.span point to the argument not the use of it as span does?

I believe you're right, I misread the code. It'd be nice to get the actual span to get the behavior I wanted, but it might be too hard to do in this case. Don't let it be a blocker.

nikomatsakis · 2018-10-02T13:37:31Z

@bors r+

bors · 2018-10-02T13:37:32Z

📌 Commit 2be3069 has been approved by nikomatsakis

bors · 2018-10-03T10:49:34Z

⌛ Testing commit 2be3069 with merge 6ddab3e...

@nikomatsakis

NLL fails to suggest "try removing `&mut` here" Fixes #51191. This PR adds ``try removing `&mut` here`` suggestions to functions where a mutable borrow is being taken of a `&mut self` or a `self: &mut Self`. This PR also enables the suggestion for adding a `mut` pattern to by-value implicit self arguments without `mut` patterns already. r? @nikomatsakis

bors · 2018-10-03T13:15:17Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6ddab3e to master...

eddyb · 2019-08-10T08:07:01Z

src/librustc/hir/mod.rs

-    /// (but not a `self: Xxx` one).
-    pub has_implicit_self: bool,
+    /// Does the function have an implicit self?
+    pub implicit_self: ImplicitSelfKind,


This is a bit sad, I was hoping this could stay out of the HIR permanently.

eddyb · 2019-08-10T08:08:56Z

src/librustc/mir/mod.rs

@@ -583,11 +583,27 @@ pub enum BindingForm<'tcx> {
    /// This is a binding for a non-`self` binding, or a `self` that has an explicit type.
    Var(VarBindingForm<'tcx>),
    /// Binding for a `self`/`&self`/`&mut self` binding where the type is implicit.
-    ImplicitSelf,
+    ImplicitSelf(ImplicitSelfKind),
    /// Reference used in a guard expression to ensure immutability.
    RefForGuard,
 }


I guess the ideal of lowering away pattern-matching completely is too naive in the face of diagnostics 😭

eddyb · 2019-08-10T08:09:16Z

src/librustc/mir/mod.rs

+    /// Represents a `fn x(self);`.
+    Imm,
+    /// Represents a `fn x(mut self);`.
+    Mut,


Can't these two be distinguished by binding_mode?

eddyb · 2019-08-10T08:10:59Z

src/librustc/mir/mod.rs

+    /// Represents a `fn x(&self);`.
+    ImmRef,
+    /// Represents a `fn x(&mut self);`.
+    MutRef,


Can't these two be distinguished by type? (maybe even distinguish &self from self by type?)

Suggest removing `&mut` from a `&mut borrow` Modify the code added in rust-lang#54720. Closes rust-lang#75871

rust-highfive assigned nikomatsakis Oct 1, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2018

This comment has been minimized.

Sign in to view

nikomatsakis approved these changes Oct 1, 2018

View reviewed changes

src/librustc/ich/impls_hir.rs Show resolved Hide resolved

src/librustc_mir/borrow_check/mutability_errors.rs Show resolved Hide resolved

davidtwco added 2 commits October 2, 2018 00:35

Improve mutability error suggestions.

2be3069

This commit improves mutability error suggestions by suggesting the removal of `&mut` where a mutable borrow is being taken of a `&mut self` or a `self: &mut Self`.

estebank reviewed Oct 1, 2018

View reviewed changes

davidtwco force-pushed the issue-51191 branch from 8244a16 to 2be3069 Compare October 1, 2018 23:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2018

bors merged commit 2be3069 into rust-lang:master Oct 3, 2018

davidtwco deleted the issue-51191 branch October 3, 2018 13:18

bors mentioned this pull request Oct 3, 2018

Rewrite the UnconditionalRecursion lint to use MIR #54490

Merged

pnkfelix mentioned this pull request Oct 4, 2018

NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663

Closed

pnkfelix added the A-NLL Area: Non-lexical lifetimes (NLL) label Oct 8, 2018

eddyb reviewed Aug 10, 2019

View reviewed changes

tesuji mentioned this pull request Sep 24, 2020

Suggest removing &mut from a &mut borrow #77110

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2020

Auto merge of rust-lang#77110 - lzutao:e0596-amp_mut, r=davidtwco

3525087

Suggest removing `&mut` from a `&mut borrow` Modify the code added in rust-lang#54720. Closes rust-lang#75871

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NLL fails to suggest "try removing `&mut` here" #54720

NLL fails to suggest "try removing `&mut` here" #54720

davidtwco commented Oct 1, 2018

This comment has been minimized.

nikomatsakis commented Oct 1, 2018

estebank Oct 1, 2018

davidtwco Oct 1, 2018

estebank Oct 1, 2018

nikomatsakis commented Oct 2, 2018

bors commented Oct 2, 2018

bors commented Oct 3, 2018

bors commented Oct 3, 2018

eddyb Aug 10, 2019

eddyb Aug 10, 2019

eddyb Aug 10, 2019

eddyb Aug 10, 2019

NLL fails to suggest "try removing &mut here" #54720

NLL fails to suggest "try removing &mut here" #54720

Conversation

davidtwco commented Oct 1, 2018

This comment has been minimized.

nikomatsakis commented Oct 1, 2018

estebank Oct 1, 2018

Choose a reason for hiding this comment

davidtwco Oct 1, 2018

Choose a reason for hiding this comment

estebank Oct 1, 2018

Choose a reason for hiding this comment

nikomatsakis commented Oct 2, 2018

bors commented Oct 2, 2018

bors commented Oct 3, 2018

bors commented Oct 3, 2018

eddyb Aug 10, 2019

Choose a reason for hiding this comment

eddyb Aug 10, 2019

Choose a reason for hiding this comment

eddyb Aug 10, 2019

Choose a reason for hiding this comment

eddyb Aug 10, 2019

Choose a reason for hiding this comment

NLL fails to suggest "try removing `&mut` here" #54720

NLL fails to suggest "try removing `&mut` here" #54720