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

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

Merged
merged 2 commits into from Oct 3, 2018

Conversation

Projects
None yet
6 participants
@davidtwco
Member

davidtwco commented Oct 1, 2018

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

@rust-highfive

This comment was marked as outdated.

Show comment
Hide comment
@rust-highfive

rust-highfive Oct 1, 2018

Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:51:12] .................................................................................................... 2000/4317
[00:51:15] ..................................................................i................................. 2100/4317
[00:51:18] .................................................................................................... 2200/4317
[00:51:21] .................................................................................................... 2300/4317
[00:51:24] ...............iiiiiiiii............................................................................ 2400/4317
[00:51:29] .................................................................................................... 2600/4317
[00:51:33] ...................................................................................................i 2700/4317
[00:51:36] .................................................................................................... 2800/4317
[00:51:39] ...........................................................i.i..ii.................................. 2900/4317
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:04:50] 
[01:04:50] running 106 tests
[01:04:53] i..ii..iii....i...i.........i..iii...........i.....i....ii...i.i.ii..............i...ii..ii.i....iii 100/106
[01:04:53] test result: ok. 76 passed; 0 failed; 30 ignored; 0 measured; 0 filtered out
[01:04:53] 
[01:04:53]  finished in 3.355
[01:04:53] travis_fold:end:test_codegen
---
travis_time:start:test_incremental
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:04:55] 
[01:04:55] running 92 tests
[01:05:07] .......................................F....................................................
[01:05:07] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:499:22
[01:05:07] 
[01:05:07] ---- [incremental] incremental/hashes/trait_defs.rs stdout ----
[01:05:07] 
[01:05:07] 
[01:05:07] error in revision `cfail2`: test compilation failed although it shouldn't!
[01:05:07] status: exit code: 1
[01:05:07] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/hashes/trait_defs.rs" "--target=x86_64-unknown-linux-gnu" "--cfg" "cfail2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/trait_defs/trait_defs.inc" "-Z" "incremental-verify-ich" "-Z" "incremental-queries" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/trait_defs/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-Zincremental-ignore-spans" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/trait_defs/auxiliary"
[01:05:07] ------------------------------------------
[01:05:07] 
[01:05:07] ------------------------------------------
[01:05:07] stderr:
[01:05:07] stderr:
[01:05:07] ------------------------------------------
[01:05:07] {"message":"`Hir(TraitChangeModeSelfOwnToMut::method)` should be clean but is not","code":null,"level":"error","spans":[{"file_name":"/checkout/src/test/incremental/hashes/trait_defs.rs","byte_start":7313,"byte_end":7335,"line_start":277,"line_end":277,"column_start":5,"column_end":27,"is_primary":true,"text":[{"text":"    fn method(mut self) {}","highlight_start":5,"highlight_end":27}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error: `Hir(TraitChangeModeSelfOwnToMut::method)` should be clean but is not\n  --> /checkout/src/test/incremental/hashes/trait_defs.rs:277:5\n   |\nLL |     fn method(mut self) {}\n   |     ^^^^^^^^^^^^^^^^^^^^^^\n\n"}
[01:05:07] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"re

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Collaborator

rust-highfive commented Oct 1, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:51:12] .................................................................................................... 2000/4317
[00:51:15] ..................................................................i................................. 2100/4317
[00:51:18] .................................................................................................... 2200/4317
[00:51:21] .................................................................................................... 2300/4317
[00:51:24] ...............iiiiiiiii............................................................................ 2400/4317
[00:51:29] .................................................................................................... 2600/4317
[00:51:33] ...................................................................................................i 2700/4317
[00:51:36] .................................................................................................... 2800/4317
[00:51:39] ...........................................................i.i..ii.................................. 2900/4317
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:04:50] 
[01:04:50] running 106 tests
[01:04:53] i..ii..iii....i...i.........i..iii...........i.....i....ii...i.i.ii..............i...ii..ii.i....iii 100/106
[01:04:53] test result: ok. 76 passed; 0 failed; 30 ignored; 0 measured; 0 filtered out
[01:04:53] 
[01:04:53]  finished in 3.355
[01:04:53] travis_fold:end:test_codegen
---
travis_time:start:test_incremental
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:04:55] 
[01:04:55] running 92 tests
[01:05:07] .......................................F....................................................
[01:05:07] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:499:22
[01:05:07] 
[01:05:07] ---- [incremental] incremental/hashes/trait_defs.rs stdout ----
[01:05:07] 
[01:05:07] 
[01:05:07] error in revision `cfail2`: test compilation failed although it shouldn't!
[01:05:07] status: exit code: 1
[01:05:07] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/hashes/trait_defs.rs" "--target=x86_64-unknown-linux-gnu" "--cfg" "cfail2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/trait_defs/trait_defs.inc" "-Z" "incremental-verify-ich" "-Z" "incremental-queries" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/trait_defs/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-Zincremental-ignore-spans" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/trait_defs/auxiliary"
[01:05:07] ------------------------------------------
[01:05:07] 
[01:05:07] ------------------------------------------
[01:05:07] stderr:
[01:05:07] stderr:
[01:05:07] ------------------------------------------
[01:05:07] {"message":"`Hir(TraitChangeModeSelfOwnToMut::method)` should be clean but is not","code":null,"level":"error","spans":[{"file_name":"/checkout/src/test/incremental/hashes/trait_defs.rs","byte_start":7313,"byte_end":7335,"line_start":277,"line_end":277,"column_start":5,"column_end":27,"is_primary":true,"text":[{"text":"    fn method(mut self) {}","highlight_start":5,"highlight_end":27}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error: `Hir(TraitChangeModeSelfOwnToMut::method)` should be clean but is not\n  --> /checkout/src/test/incremental/hashes/trait_defs.rs:277:5\n   |\nLL |     fn method(mut self) {}\n   |     ^^^^^^^^^^^^^^^^^^^^^^\n\n"}
[01:05:07] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"re

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 1, 2018

Contributor

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.

Contributor

nikomatsakis commented Oct 1, 2018

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.

davidtwco added some commits Oct 1, 2018

Improve implicit self mutability suggestions.
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.
Improve mutability error suggestions.
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`.
} =>
{
err.span_label(span, format!("cannot {ACT}", ACT = act));
err.span_label(span, "try removing `&mut` here");

This comment has been minimized.

@estebank

estebank Oct 1, 2018

Contributor

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.

@estebank

estebank Oct 1, 2018

Contributor

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.

This comment has been minimized.

@davidtwco

davidtwco Oct 1, 2018

Member

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

@davidtwco

davidtwco Oct 1, 2018

Member

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

This comment has been minimized.

@estebank

estebank Oct 1, 2018

Contributor

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.

@estebank

estebank Oct 1, 2018

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 2, 2018

Contributor

@bors r+

Contributor

nikomatsakis commented Oct 2, 2018

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 2, 2018

Contributor

📌 Commit 2be3069 has been approved by nikomatsakis

Contributor

bors commented Oct 2, 2018

📌 Commit 2be3069 has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 3, 2018

Contributor

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

Contributor

bors commented Oct 3, 2018

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

bors added a commit that referenced this pull request Oct 3, 2018

Auto merge of #54720 - davidtwco:issue-51191, r=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

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 3, 2018

Contributor

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

Contributor

bors commented Oct 3, 2018

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

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-51191 branch Oct 3, 2018

@pnkfelix pnkfelix added the A-NLL label Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment