Clearer Error Message for Duplicate Definition #42076

alex-ozdemir · 2017-05-18T03:37:39Z

Clearer use of the error message and span labels to communicate duplication definitions/imports.

fixes #42061

rust-highfive · 2017-05-18T03:37:45Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Mark-Simulacrum · 2017-05-18T04:19:52Z

Could you give us a before/after comparison?

Mark-Simulacrum · 2017-05-18T04:21:20Z

src/test/compile-fail/E0254.rs

@@ -11,7 +11,7 @@
 #![feature(collections)]

 extern crate collections;
-//~^ NOTE previous import of `collections` here
+//~^ NOTE previous import of the extern crate `collections` here


I feel like this should just be the crate, there's no such thing as an "extern crate."

I don't mind either way.

The original behavior was to regard this as an extern crate, although it was identified as such in the error header rather than the span label.

alex-ozdemir · 2017-05-18T04:32:33Z

Of course!

Source (example.rs)

trait Foo { }
struct Foo { }

fn main() {}

Original message:

error[E0428]: a trait named `Foo` has already been defined in this module
 --> example.rs:2:1
  |
1 | trait Foo { }
  | ------------- previous definition of `Foo` here
2 | struct Foo { }
  | ^^^^^^^^^^^^^^ `Foo` already defined

error: aborting due to previous error

New message:

error[E0428]: the name `Foo` is defined twice in this module
 --> example.rs:2:1
  |
1 | trait Foo { }
  | ------------- previous definition of the trait `Foo` here
2 | struct Foo { }
  | ^^^^^^^^^^^^^^ `Foo` redefined here

error: aborting due to previous error

alexcrichton · 2017-05-18T20:11:52Z

Thanks for the PR @alex-ozdemir! We'll make sure @arielb1 or another team member reviews this soon

arielb1 · 2017-05-19T10:00:13Z

r? @nrc

nrc

This looks good, I especially like the changes to the notes on the second use. Two issues inline.

nrc · 2017-05-23T23:34:15Z

src/librustc_resolve/lib.rs

        };

+        let msg = format!("the name `{}` is defined twice in this {}", name, container);


This message would be inaccurate if the name were defined more than twice, right?

Good point. I'll change this.

nrc · 2017-05-23T23:34:56Z

src/librustc_resolve/lib.rs

+            false => "definition",
+        };
+
+        let new_participle = match binding.is_import() {


naming nits: why old_ and new_ here?

I should use this more consistently. The original (old) definition/import and the new one may have different participles/nouns.

petrochenkov · 2017-05-24T06:59:35Z

The old message was suboptimal, but it tried to convey something about namespaces

a **type** named `X` has already been defined ...

The new message just says "name"

the **name** `X` is defined twice in this module

which is not strictly correct because several items in a module can have the same name if they are in different namespaces

type X = u8;
fn X() {}
macro X {}

Maybe extend the message to

the name `X` is defined twice in this module in type namespace

?

petrochenkov · 2017-05-24T07:01:20Z

src/test/compile-fail/issue-28472.rs

@@ -13,10 +13,10 @@
 extern {
  fn foo();

-  pub //~ ERROR a value named `foo` has already been defined
+  pub //~ ERROR E0428


Could you keep using messages for testing and avoid error codes outside of E0XXX.rs tests.

Sure thing.

alex-ozdemir · 2017-05-25T00:05:08Z

@petrochenkov: Good idea, will do.

Question: which is better?

the name `X` is bound multiple times in the {type,...} namespace of this {module,...}

the name `X` is defined multiple times in the {type,...} namespace of this {module,...}

The former is more correct (it could be any duplication of imports & definitions), but the word bound is less common.

petrochenkov · 2017-05-25T06:53:44Z

@alex-ozdemir
I think "defined" should be kept.

alex-ozdemir · 2017-05-26T04:25:47Z

Thanks for the feedback!

New error format:

error[E0428]: the name `Foo` is defined multiple times
 --> example.rs:2:1
  |
1 | trait Foo { }
  | ------------- previous definition of the trait `Foo` here
2 | struct Foo { }
  | ^^^^^^^^^^^^^^ `Foo` redefined here
  = note: `Foo` must be defined only once in the type namespace of this module

error: aborting due to previous error

kennytm · 2017-05-26T09:18:20Z

The test compile-fail/proc-macro/shadow.rs failed, you need to update the test's error messages.

Error details

[00:39:59] ---- [compile-fail] compile-fail/proc-macro/shadow.rs stdout ----
[00:39:59] 	
[00:39:59] error: /checkout/src/test/compile-fail-fulldeps/proc-macro/shadow.rs:16: unexpected "error": '16:1: 16:23: the name `derive_a` is defined multiple times [E0259]'
[00:39:59] 
[00:39:59] error: /checkout/src/test/compile-fail-fulldeps/proc-macro/shadow.rs:16: expected error not found: the name `derive_a` is defined twice
[00:39:59] 
[00:39:59] error: 1 unexpected errors found, 1 expected errors not found
[00:39:59] status: exit code: 101
[00:39:59] command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc /checkout/src/test/compile-fail-fulldeps/proc-macro/shadow.rs -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps --target=x86_64-unknown-linux-gnu --error-format json -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps/proc-macro/shadow.stage2-x86_64-unknown-linux-gnu.compile-fail.libaux -C prefer-dynamic -o /checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail-fulldeps/proc-macro/shadow.stage2-x86_64-unknown-linux-gnu -Crpath -O -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers
[00:39:59] unexpected errors (from JSON output): [
[00:39:59]     Error {
[00:39:59]         line_num: 16,
[00:39:59]         kind: Some(
[00:39:59]             Error
[00:39:59]         ),
[00:39:59]         msg: "16:1: 16:23: the name `derive_a` is defined multiple times [E0259]"
[00:39:59]     }
[00:39:59] ]
[00:39:59] 
[00:39:59] not found errors (from test file): [
[00:39:59]     Error {
[00:39:59]         line_num: 16,
[00:39:59]         kind: Some(
[00:39:59]             Error
[00:39:59]         ),
[00:39:59]         msg: "the name `derive_a` is defined twice"
[00:39:59]     }
[00:39:59] ]
[00:39:59] 
[00:39:59] thread '[compile-fail] compile-fail/proc-macro/shadow.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:1129
[00:39:59] note: Run with `RUST_BACKTRACE=1` for a backtrace.

alex-ozdemir · 2017-05-26T16:35:51Z

Good spot!

aidanhs · 2017-06-01T00:50:14Z

I've pinged @nrc on IRC to get a second review pass for this PR.

nrc · 2017-06-01T04:36:22Z

r = me with commits squashed. Thanks!

arielb1 · 2017-06-06T11:49:10Z

@alex-ozdemir

We're just waiting for you to squash the commits. Can you get that done so we can finish working on this PR?

alex-ozdemir · 2017-06-08T11:48:07Z

Okay, sorry for the delay. I had forgotten that Rust doesn't use the standard PR workflow that allows y'all to squash & merge on your own.

Assuming I squashed correctly the integration tests should pass again, and we'll be good to go!

Mark-Simulacrum · 2017-06-11T16:25:55Z

Looks like the squash possibly introduced some problems, could you take a look and fix those please? https://travis-ci.org/rust-lang/rust/jobs/240754613#L786

alex-ozdemir · 2017-06-15T14:42:44Z

Whoops!

All good now.

Mark-Simulacrum · 2017-06-15T14:48:54Z

@bors r=nrc

bors · 2017-06-15T14:48:55Z

📌 Commit 89bd87e has been approved by nrc

bors · 2017-06-15T15:02:20Z

🔒 Merge conflict

bors · 2017-06-15T15:07:02Z

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

Clearer use of the error message and span labels to communicate duplicaiton defitions/imports. New error format: ``` error[E0428]: the name `Foo` is defined twice --> example.rs:2:1 | 1 | trait Foo { } | ------------- previous definition of the trait `Foo` here 2 | struct Foo { } | ^^^^^^^^^^^^^^ `Foo` redefined here = note: `Foo` must be defined only once in the type namespace of this module error: aborting due to previous error ```

alex-ozdemir · 2017-06-16T14:25:04Z

I believe the merge conflicts are resolved.

arielb1 · 2017-06-20T20:08:06Z

Looks like this PR fell between the chairs a bit. Currently waiting for review by @nrc.

nrc · 2017-06-20T20:42:19Z

@bors: r+

bors · 2017-06-20T20:42:20Z

📌 Commit a82890e has been approved by nrc

bors · 2017-06-21T00:06:08Z

⌛ Testing commit a82890e with merge bb14389...

Clearer Error Message for Duplicate Definition Clearer use of the error message and span labels to communicate duplication definitions/imports. fixes #42061

bors · 2017-06-21T04:14:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing bb14389 to master...

rust-highfive assigned arielb1 May 18, 2017

Mark-Simulacrum reviewed May 18, 2017

View reviewed changes

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2017

rust-highfive assigned nrc and unassigned arielb1 May 19, 2017

nrc reviewed May 23, 2017

View reviewed changes

petrochenkov reviewed May 24, 2017

View reviewed changes

Mark-Simulacrum 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 Jun 4, 2017

alex-ozdemir force-pushed the master branch 4 times, most recently from c7e4cdb to 87c8bf7 Compare June 8, 2017 11:45

alex-ozdemir force-pushed the master branch from 87c8bf7 to 89bd87e Compare June 15, 2017 00:44

alex-ozdemir force-pushed the master branch from 89bd87e to a82890e Compare June 15, 2017 15:21

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

nrc 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 Jun 20, 2017

bors added a commit that referenced this pull request Jun 21, 2017

Auto merge of #42076 - alex-ozdemir:master, r=nrc

bb14389

Clearer Error Message for Duplicate Definition Clearer use of the error message and span labels to communicate duplication definitions/imports. fixes #42061

bors merged commit a82890e into rust-lang:master Jun 21, 2017

		};

		let msg = format!("the name `{}` is defined twice in this {}", name, container);

Clearer Error Message for Duplicate Definition #42076

Clearer Error Message for Duplicate Definition #42076

Uh oh!

Conversation

alex-ozdemir commented May 18, 2017

Uh oh!

rust-highfive commented May 18, 2017

Uh oh!

Mark-Simulacrum commented May 18, 2017

Uh oh!

Mark-Simulacrum May 18, 2017

Choose a reason for hiding this comment

Uh oh!

alex-ozdemir May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-ozdemir commented May 18, 2017

Uh oh!

alexcrichton commented May 18, 2017

Uh oh!

arielb1 commented May 19, 2017

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

nrc May 23, 2017

Choose a reason for hiding this comment

Uh oh!

alex-ozdemir May 24, 2017

Choose a reason for hiding this comment

Uh oh!

nrc May 23, 2017

Choose a reason for hiding this comment

Uh oh!

alex-ozdemir May 24, 2017

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov May 24, 2017

Choose a reason for hiding this comment

Uh oh!

alex-ozdemir May 25, 2017

Choose a reason for hiding this comment

Uh oh!

alex-ozdemir commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented May 25, 2017

Uh oh!

alex-ozdemir commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennytm commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-ozdemir commented May 26, 2017

Uh oh!

aidanhs commented Jun 1, 2017

Uh oh!

nrc commented Jun 1, 2017

Uh oh!

arielb1 commented Jun 6, 2017

Uh oh!

alex-ozdemir commented Jun 8, 2017

Uh oh!

Mark-Simulacrum commented Jun 11, 2017

Uh oh!

alex-ozdemir commented Jun 15, 2017

Uh oh!

Mark-Simulacrum commented Jun 15, 2017

Uh oh!

bors commented Jun 15, 2017

Uh oh!

bors commented Jun 15, 2017

Uh oh!

bors commented Jun 15, 2017

Uh oh!

alex-ozdemir May 18, 2017 •

edited

Loading

petrochenkov commented May 24, 2017 •

edited

Loading

alex-ozdemir commented May 25, 2017 •

edited

Loading

alex-ozdemir commented May 26, 2017 •

edited

Loading

kennytm commented May 26, 2017 •

edited

Loading