Modify compile-fail/E0389 error message WIP #48914

gaurikholkar-zz · 2018-03-10T15:27:28Z

This fixes #47388

cc @nikomatsakis @estebank

r? @nikomatsakis

Certain ui tests were failing locally. I'll check if the same happens here too.

rust-highfive · 2018-03-10T15:27:33Z

r? @estebank

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

gaurikholkar-zz · 2018-03-10T15:30:39Z

src/test/compile-fail/borrowck/borrowck-overloaded-index-ref-index.rs

@@ -70,5 +70,5 @@ fn main() {
    };
    s[2] = 20;
    //[ast]~^ ERROR cannot assign to immutable indexed content
-    //[mir]~^^ ERROR cannot assign to immutable item
+    //[mir]~^^ ERROR cannot assign through immutable item


I'm not sure about the look of this test though.

This one seems ok-ish to me, but we might be able to improve upon it. I'll think about it.

gaurikholkar-zz · 2018-03-10T15:32:39Z

This is how the error message looks like

error[E0594]: cannot assign through `&`-reference fancy_ref
  --> src/test/compile-fail/E0389.rs:18:5
   |
17 |     let fancy_ref = &(&mut fancy);
   |                     ------------- help: consider changing this to be a mutable reference: `&mut`
18 |     fancy_ref.num = 6; //~ ERROR E0389
   |     ^^^^^^^^^^^^^^^^^ cannot assign to field of `&`-reference

Do I need to add a ui test for this? There's an already existing compile-fail test

estebank · 2018-03-10T18:37:01Z

Do I need to add a ui test for this? There's an already existing compile-fail test

Could you move the file from compile-fail to ui?

estebank · 2018-03-10T18:38:53Z

src/librustc_mir/borrow_check/mod.rs

-                    let item_msg = match self.describe_place(place) {
-                        Some(name) => format!("immutable item `{}`", name),
-                        None => "immutable item".to_owned(),
+                    let err_info = match *place_err {


Given how deeply nested this is, it might make sense to move it to its own method, or at least adding a comment on top describing what the code is supposed to be doing.

estebank · 2018-03-10T18:41:25Z

src/librustc_mir/borrow_check/mod.rs

-                        Some(name) => format!("immutable item `{}`", name),
-                        None => "immutable item".to_owned(),
+                    let err_info = match *place_err {
+                        Place::Projection(ref proj) => {


You can match on multiple levels to avoid deeply nested structures:

enum Foo { A { f: Bar }, B(Bar), } enum Bar { X, Y, } let x = Foo::B(Bar::X); match x { Foo::A { f: Bar::Y } => 1, _ => 0, }

estebank · 2018-03-10T18:43:00Z

src/librustc_mir/borrow_check/mod.rs

+                                _ => None,
+                            }
+                        }
+                        _ => None,


Given how many branches there are here, it might make sense to make err_info mutable with a default value of None, and override where applicable, but this might not be needed if the amount of branches is reduced.

estebank · 2018-03-10T18:43:15Z

src/librustc_mir/borrow_check/mod.rs

                        }
+                        err.emit()
+                    }else{


spaces around else

estebank · 2018-03-10T18:43:43Z

src/librustc_mir/borrow_check/mod.rs

+                        err.span_label(span, "cannot mutate");
+                        if place != place_err {
+                            if let Some(name) = self.describe_place(place_err) {
+                                err.note(&format!("Value not mutable causing this error: `{}`",


lowercase sentences: value not mutable

estebank · 2018-03-10T18:44:21Z

src/librustc_mir/lib.rs

@@ -39,6 +39,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment!
 #![feature(collection_placement)]
 #![feature(nonzero)]
 #![feature(underscore_lifetimes)]
+#![feature(crate_visibility_modifier)]


Was this needed?

yes, due to the usage here crate trait FindAssignments { in collect_writes.rs

estebank · 2018-03-10T18:45:44Z

src/test/compile-fail/borrowck/borrowck-overloaded-index-ref-index.rs

@@ -70,5 +70,5 @@ fn main() {
    };
    s[2] = 20;
    //[ast]~^ ERROR cannot assign to immutable indexed content
-    //[mir]~^^ ERROR cannot assign to immutable item
+    //[mir]~^^ ERROR cannot assign through immutable item


This one seems ok-ish to me, but we might be able to improve upon it. I'll think about it.

estebank · 2018-03-10T18:46:05Z

src/test/compile-fail/borrowck/borrowck-issue-14498.rs

@@ -27,7 +27,7 @@ fn indirect_write_to_imm_box() {
    let y: Box<_> = box &mut x;
    let p = &y;
    ***p = 2; //[ast]~ ERROR cannot assign to data in a `&` reference
-              //[mir]~^ ERROR cannot assign to immutable item `***p`
+              //[mir]~^ ERROR cannot assign through `&`-reference `p`


Not convinced about this change...

certainly better than it was ("imutable item"), but not I think perfect yet -- and not necessarily better than AST variant, I don't think

The AST based error is this:

error[E0389]: cannot assign to data in a `&` reference --> src/main.rs:8:5 | 8 | ***p = 2; | ^^^^^^^^ assignment into an immutable reference

which I think has too much jargon, but some promising elements. This also intersects your style guide @estebank. Personally, my preference is that the "main" message is kind of generic and formal, like it is here, telling you nothing of the program itself, but that those details are revealed in the "underlines" and in a "conversational" tone. Maybe something like this?

error[E0389]: cannot assign to data in a `&` reference --> src/main.rs:8:5 | 8 | ***p = 2; | ^^^^^^^^ `p` is a `&` reference, so the data it refers to cannot be written

Is this the common message for all such cases or just for the above case?

error[E0594]: cannot assign to data in a `&` reference --> src/test/compile-fail/borrowck/borrowck-issue-14498.rs:29:5 | 28 | let p = &y; | -- help: consider changing this to be a mutable reference: `&mut` 29 | ***p = 2; | ^^^^^^^^ `p` is a `&` reference, so the data it refers to cannot be written

is how the message looks like now @nikomatsakis

nikomatsakis

Left a few nits, to start.

nikomatsakis · 2018-03-13T17:47:34Z

src/librustc_mir/borrow_check/mod.rs

                let mut err = self.tcx
                    .cannot_borrow_path_as_mutable(span, &item_msg, Origin::Mir);
                err.span_label(span, "cannot borrow as mutable");

                if place != place_err {
                    if let Some(name) = self.describe_place(place_err) {
-                        err.note(&format!("Value not mutable causing this error: `{}`", name));
+                        err.note(&format!("value not mutable causing this error: `{}`", name));


Pre-existing, but this message is not particularly grammatical. In fact, I have a hard time even understanding it -- I guess it is saying that "the value which is causing this path not to be mutable is..."?

nikomatsakis · 2018-03-13T17:50:35Z

src/librustc_mir/util/collect_writes.rs

+
+crate trait FindAssignments {
+    fn find_assignments(&self, local: Local) -> Vec<Location>;
+    }


Nit: formatting

nikomatsakis · 2018-03-13T17:51:20Z

src/librustc_mir/util/collect_writes.rs

+    // fn super_local()
+}
+
+crate trait FindAssignments {


I would move this to the top of the file -- this is what readers care about. Also, I would add a comment to the method:

Finds all statements that assign directly to local (i.e., X = ...) and returns their locations.

nikomatsakis · 2018-03-13T17:51:34Z

src/librustc_mir/util/collect_writes.rs

+
+// The Visitor walks the MIR to return the assignment statements corresponding
+// to a Local.
+pub struct FindLocalAssignmentVisitor {


This does not need to be public, it is an impl detail. Just make it private.

nikomatsakis · 2018-03-13T17:53:09Z

src/librustc_mir/borrow_check/mod.rs

+                                                let item_msg = if error_reported {
+                                                    if let Some(name) =
+                                                            self.describe_place(place_err) {
+                                                        let var = str::replace(&name, "*", "");


Hmm, I'm not crazy about the string munging =) Maybe we should just do describe_place(&place.base)?

nikomatsakis · 2018-03-13T17:54:01Z

src/librustc_mir/borrow_check/mod.rs

+                                                        let var = str::replace(&name, "*", "");
+                                                        format!("`&`-reference `{}`", var)
+                                                    } else {
+                                                        self.get_main_error_message(place)


I would pull this into a helper function and I would change these get_main_error_message cases to just return None. The helper would be something like:

fn specialized_description(..) -> Option<String>

then if it is None you can use get_main_error_message(place).

bors · 2018-03-15T02:21:31Z

☔ The latest upstream changes (presumably #47630) made this pull request unmergeable. Please resolve the merge conflicts.

pietroalbini · 2018-03-20T08:08:04Z

@gaurikholkar ping from triage! There are a few comments from the reviewer, could you address them?

TimNN · 2018-04-06T15:46:38Z

Your PR failed on Travis. 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.

Resolving deltas: 100% (612610/612610), completed with 4869 local objects.
---
[00:00:39] configure: rust.quiet-tests     := True
---
[00:44:32] ..........................................................................i.........................
[00:44:38] .................i..................................................................................
---
[00:45:14] ..............................................................................................i.....
[00:45:22] .......................................F.............................i..............................
[00:45:28] ....................................................................................................
[00:45:35] ....................................................................................................
[00:45:43] ....................................................................................................
`&`-reference `fancy_ref` [E0594]"
---
[00:45:44]         msg: "E0594"
---
[00:45:44] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zmiri -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zmiri -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:45:44] expected success, got: exit code: 101
[00:45:44]
[00:45:44]
[00:45:44] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:45:44] Build completed unsuccessfully in 0:02:29
[00:45:44] Makefile:58: recipe for target 'check' failed
[00:45:44] make: *** [check] Error 1

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.

gaurikholkar-zz · 2018-04-07T05:24:39Z

@estebank @nikomatsakis repoened the issue

TimNN · 2018-04-07T05:31:21Z

Your PR failed on Travis. 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.

Resolving deltas: 100% (612665/612665), completed with 4869 local objects.
---
[00:00:52] configure: rust.quiet-tests     := True

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.

gaurikholkar-zz · 2018-04-07T05:50:34Z

Not sure why tidy checks are failing. Couldn't find any trailing whitespace.

TimNN · 2018-04-07T07:56:03Z

Your PR failed on Travis. 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.

Resolving deltas: 100% (612667/612667), completed with 4869 local objects.
---
[00:01:04] configure: rust.quiet-tests     := True
---
[00:04:35] tidy error: /checkout/src/librustc_mir/borrow_check/mod.rs:1578: trailing whitespace
[00:04:35] tidy error: /checkout/src/librustc_mir/borrow_check/mod.rs:1582: trailing whitespace
[00:04:35] tidy error: /checkout/src/librustc_mir/borrow_check/mod.rs:1644: trailing whitespace
[00:04:36] some tidy checks failed
[00:04:36]
[00:04:36]
[00:04:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:36] expected success, got: exit code: 1
[00:04:36]
[00:04:36]
[00:04:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:36] Build completed unsuccessfully in 0:01:54
[00:04:36] Makefile:79: recipe for target 'tidy' failed
[00:04:36] make: *** [tidy] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:159c8622:start=1523087670093303512,finish=1523087670099866766,duration=6563254
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:28befa78
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:28befa78:start=1523087670105669571,finish=1523087670112459931,duration=6790360
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:05e4a96c
$ dmesg | grep -i kill
[   10.173691] init: failsafe main process (1094) killed by TERM signal

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.

kennytm · 2018-04-07T09:02:47Z

src/librustc_mir/borrow_check/mod.rs

+                                            } else {
+                                                self.get_default_err_msg(place)
+                                            };
+


A line consisting of only spaces is also considered "trailing whitespaces".

thanks @kennytm :)

@kennytm can you help out with the fixes for the tidy errors? I've tried certain things but they don't seem to work. One of the errors is also in the previously written code.

@gaurikholkar The line numbers seems wrong. You could run tidy locally by

./x.py test src/tools/tidy

The lines with trailing whitespaces are:

tidy error: src/librustc_mir/borrow_check/mod.rs:1532: trailing whitespace tidy error: src/librustc_mir/borrow_check/mod.rs:1536: trailing whitespace tidy error: src/librustc_mir/borrow_check/mod.rs:1595: trailing whitespace

Thanks you @kennytm . Yeah the line nos were wrong. My local tidy script panics -> logs

@gaurikholkar you should just remove the entire binaryen folder, it is no longer used.

TimNN · 2018-04-07T09:13:46Z

Your PR failed on Travis. 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.

Resolving deltas: 100% (613151/613151), completed with 4866 local objects.
---
[00:00:50] configure: rust.quiet-tests     := True

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.

TimNN · 2018-04-08T17:35:31Z

Your PR failed on Travis. 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.

Resolving deltas: 100% (612769/612769), completed with 4866 local objects.
---
[00:00:42] configure: rust.quiet-tests     := True
---
[00:05:01] tidy error: /checkout/src/librustc_mir/borrow_check/mod.rs:1578: trailing whitespace
[00:05:01] tidy error: /checkout/src/librustc_mir/borrow_check/mod.rs:1582: trailing whitespace
[00:05:01] tidy error: /checkout/src/librustc_mir/borrow_check/mod.rs:1641: trailing whitespace
[00:05:02] some tidy checks failed
[00:05:02]
[00:05:02]
[00:05:02] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:02] expected success, got: exit code: 1
[00:05:02]
[00:05:02]
[00:05:02] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:02] Build completed unsuccessfully in 0:02:00
[00:05:02] Makefile:79: recipe for target 'tidy' failed
[00:05:02] make: *** [tidy] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: canno

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.

nikomatsakis · 2018-04-10T16:53:40Z

This is looking pretty good to me! Let's land it. We can always tinker more.

@bors r+

Nice job @gaurikholkar

bors · 2018-04-10T16:53:41Z

📌 Commit c792d1e has been approved by nikomatsakis

bors · 2018-04-10T19:00:27Z

⌛ Testing commit c792d1e with merge 0b72d48...

@nikomatsakis

Modify compile-fail/E0389 error message WIP This fixes #47388 cc @nikomatsakis @estebank r? @nikomatsakis Certain ui tests were failing locally. I'll check if the same happens here too.

estebank · 2018-04-10T20:35:55Z

src/test/ui/nll/issue-47388.stderr

+  --> $DIR/issue-47388.rs:18:5
+   |
+LL |     let fancy_ref = &(&mut fancy);
+   |                     ------------- help: consider changing this to be a mutable reference: `&mut`


I believe either the span or the suggestion text might be incorrect. It should either suggest &mut (&mut fancy) for this span, or use a span only selecting the &.

Filed #49859

bors · 2018-04-10T21:50:48Z

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

pietroalbini · 2018-04-10T21:52:09Z

@estebank I guess now we need another PR for that...

gaurikholkar added 7 commits March 10, 2018 20:51

changes for new compile-fail/E0389 message

7d590b5

adding Visitor changes

308e30e

update visit_local for FindLocalAssignmentVisitor

f60788b

minor refactorings to fix trait import issue

3f0ce08

add collect_writes.rs

bfc9b76

minor changes

7a266a6

code refactor, modify compile-fail tests

0c7fc04

gaurikholkar-zz requested review from nikomatsakis and estebank March 10, 2018 15:27

rust-highfive assigned estebank Mar 10, 2018

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

gaurikholkar-zz changed the title ~~Modify comiple-fail/E0389 error message~~ Modify compile-fail/E0389 error message Mar 10, 2018

gaurikholkar-zz changed the title ~~Modify compile-fail/E0389 error message~~ Modify compile-fail/E0389 error message WIP Mar 10, 2018

gaurikholkar-zz commented Mar 10, 2018

View reviewed changes

tidy fixes

fdb2f7f

fix warnings

55bd914

estebank reviewed Mar 10, 2018

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2018

estebank reviewed Mar 10, 2018

View reviewed changes

gaurikholkar added 3 commits March 11, 2018 00:25

add ui test for E0594

50299c6

fix tidy issues

7745b52

address code review comments

311a8be

nikomatsakis self-assigned this Mar 13, 2018

nikomatsakis requested changes Mar 13, 2018

View reviewed changes

fix ui test

e18a83b

modify the error message- CR Comments

e5a96a4

gaurikholkar-zz force-pushed the e0389 branch from 83c0451 to e5a96a4 Compare April 7, 2018 07:47

kennytm reviewed Apr 7, 2018

View reviewed changes

fix tidy errors

cbde62c

shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. labels Apr 7, 2018

tidy fixes

2ad20e8

tidy fixes

c792d1e

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 10, 2018

nikomatsakis approved these changes Apr 10, 2018

View reviewed changes

estebank requested changes Apr 10, 2018

View reviewed changes

bors merged commit c792d1e into rust-lang:master Apr 10, 2018

This was referenced Apr 10, 2018

Introduce RangeInclusive::{new, start, end} methods and make the fields private. #49724

Merged

Add ok-wrapping to catch blocks, per RFC #49371

Merged

estebank mentioned this pull request Apr 10, 2018

Incorrect span in &mut suggestion #49859

Closed

Modify compile-fail/E0389 error message WIP #48914

Modify compile-fail/E0389 error message WIP #48914

Conversation

gaurikholkar-zz commented Mar 10, 2018 • edited Loading

rust-highfive commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaurikholkar-zz commented Mar 10, 2018

estebank commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaurikholkar-zz Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 15, 2018

pietroalbini commented Mar 20, 2018

TimNN commented Apr 6, 2018

gaurikholkar-zz commented Apr 7, 2018

TimNN commented Apr 7, 2018

gaurikholkar-zz commented Apr 7, 2018

TimNN commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimNN commented Apr 7, 2018

TimNN commented Apr 8, 2018

nikomatsakis commented Apr 10, 2018

bors commented Apr 10, 2018

bors commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 10, 2018

pietroalbini commented Apr 10, 2018

gaurikholkar-zz commented Mar 10, 2018 •

edited

Loading

gaurikholkar-zz Mar 10, 2018 •

edited

Loading