Show span for trait that doesn't implement Copy #37493

estebank · 2016-10-31T04:56:40Z

Point at span for trait that doesn't implement Copy.

Given the following code:

struct NoCopy;

enum Test {
    MyVariant(NoCopy)
}

impl Copy for Test {}

fn main() {}

show the following output:

error[E0205]: the trait `Copy` may not be implemented for this type
 --> file.rs:7:6
  |
4 |     MyVariant(NoCopy)
  |     ----------------- `MyVariant` defined here
...
7 | impl Copy for Test {}
  |      ^^^^ variant `MyVariant` does not implement `Copy`

error: aborting due to previous error

Re: #19950.

rust-highfive · 2016-10-31T04:56:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (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.

estebank · 2016-10-31T05:00:42Z

The current output is:

error[E0205]: the trait `Copy` may not be implemented for this type
 --> <anon>:7:6
  |
7 | impl Copy for Test {}
  |      ^^^^ variant `MyVariant` does not implement `Copy`

error: aborting due to previous error

estebank · 2016-11-07T04:27:08Z

r? @jonathandturner

sophiajt · 2016-11-08T13:09:28Z

Cool. Yeah, I like this, I think. Does it work for things that aren't enum variants also?

Looking at the code, I was a little confused:

    pub fn span_if_local(&self, id: DefId) -> Option<Span> {
        self.as_local_node_id(id).map(|id| self.span(id))
    }

    pub fn span_if_local_opt(&self, id: DefId) -> Option<Span> {
        self.as_local_node_id(id).and_then(|id| self.opt_span(id))
    }

Do we need the new function you're adding? Seem similar

estebank · 2016-11-08T19:05:00Z

Do we need the new function you're adding? Seem similar

@jonathandturner during my testing if I found cases where an id would be used that could not be returned by self.span, causing a panic. I didn't replace span_if_local with the body you see in ..._opt because I was unsure wether this behavior was intended and relied upon in other parts of the codebase. If it isn't, then using opt_span(_) instead of span(_) would be better.

Does it work for things that aren't enum variants also?

It should work for structs too. I can add an unittest for that case if you want.

estebank · 2016-11-11T00:16:47Z

@jonathandturner left only span_if_local but with the non-panic behavior. Added UI test.

brson · 2016-11-11T03:40:27Z

src/librustc_typeck/coherence/mod.rs

+                                                   this type");
+                    err.span_label(span, &format!("field `{}` does not implement `Copy`", name));
+                    if let Some(def_span) = tcx.map.span_if_local(did) {
+                        err.span_label(def_span, &format!("`{}` defined here", name));


I don't see tests for this "defined here" branch. There's a test for variant "defined here" case, but not for fields. Can you add one if there aren't?

brson · 2016-11-11T04:03:31Z

Although I think this improvement is fine as is, some thoughts about this example:

error[E0205]: the trait `Copy` may not be implemented for this type
 --> file.rs:7:6
  |
4 |     MyVariant(NoCopy)
  |     ----------------- `MyVariant` defined here
...
7 | impl Copy for Test {}
  |      ^^^^ variant `MyVariant` does not implement `Copy`

It's peculiar that the subject of the error, the type "Test", is mentioned in an extremely non-prominent way. The main error text, "may not be implemented for this type", makes it sound like a type is the most important thing in this error, but the two snippets printed are about variants, and don't mention the type.

Underlining Copy with the annotation "variant MyVariant does not implement Copy" is not as clear is it could be. What's the relationship between "Copy" in impl Copy for Test and MyVariant.

When the variant is defined before the impl, as in this example, the first snippet is no longer pointing to the subject of the error, that is, impl Copy for Test. I found that confusing. I'm seeing an error about an impl Copy and a type, being shown a variant of something, then an impl Copy of a type that is the subject of the error, then a statement that "MyVariant does not implement Copy". It doesn't quite all connect.

Considering the information at hand I might find something like the following more illuminating:

error[E0205]: the trait `Copy` may not be implemented for type `Test`
 --> file.rs:7:6
  |
7 | impl Copy for Test {}
...
4 |     MyVariant(NoCopy)
  |     ----------------- because `Test` variant `MyVariant` does not implement `Copy`

then maybe even "because NoCopy does not implement Copy. To make Test Copy, derive Copy for etc."

I'm not sure how correct it is to say that a variant implements a trait, since variants are not types.

brson · 2016-11-11T04:22:17Z

Oh, here's another try:

error[E0205]: enum variant `Test::MyVariant` does not implement `Copy`
 --> file.rs:7:6
  |
4 |     MyVariant(NoCopy)
  |                        ---- because `NoCopy` is not `Copy`
...
7 | impl Copy for Test {}
  |      ^^^^ required because `Copy` is implemented for `Test` here

This makes it about the variant, assuming that the solution is to make NoCopy copy, and that's what they should look at.

I don't know if that's true or not though. Just some ideas.

The more important case anyway is the one where impl Copy isn't written explicitly but derived. Can you add a test case that uses #[derive(Copy)] and generates these errors?

estebank · 2016-11-12T06:34:48Z

@brson changed the output to:

error[E0205]: the trait `Copy` may not be implemented for type `Foo`
  --> $DIR/issue-19950.rs:17:6
   |
14 |     MyVariant(NoCopy)
   |     ----------------- this variant doesn't implement `Copy`
...
17 | impl Copy for Foo {}
   |      ^^^^ variant `Foo::MyVariant` doesn't implement `Copy`

error[E0204]: the trait `Copy` may not be implemented for type `Bar`
  --> $DIR/issue-19950.rs:23:1
   |
23 | impl Copy for Bar {}
   | ^^^^^^^^^^^^^^^^^^^^ field `item` doesn't implement `Copy`

How does that look?

The more important case anyway is the one where impl Copy isn't written explicitly but derived. Can you add a test case that uses #[derive(Copy)] and generates these errors?

There's already a non ui test for it

sophiajt · 2016-11-12T13:37:08Z

I'd say this is probably getting close enough. The one question I had looking at it comparing the two was here:

error[E0204]: the trait `Copy` may not be implemented for type `Bar`
  --> $DIR/issue-19950.rs:23:1
   |
23 | impl Copy for Bar {}
   | ^^^^^^^^^^^^^^^^^^^^ field `item` doesn't implement `Copy`

In the E0205 case we show where the problem is, but here you have to go find where item is defined. Is there any way to show that with a secondary label like the E0205 case?

estebank · 2016-11-12T21:39:29Z

I've been trying to get the field item to be shown, but for some reason it (ItemStruct) is not part of the Map, while MyVariant (EntryVariant) is. That's why I'm not showing the definition. I'm looking at fixing that.

estebank · 2016-11-13T01:32:43Z

So I've found that struct items are never part of the Map. I've made it so that they're added, pointing to the struct's Node, as I haven't been able to find a clean way to add an EntryStructField. Is it ok if I create a different PR to properly add support for this, given the current output:

error[E0205]: the trait `Copy` may not be implemented for type `Foo`
 --> foo.rs:7:6
  |
4 |         MyVariant(NoCopy)
  |         ----------------- this variant doesn't implement `Copy`
...
7 | impl Copy for Foo {}
  |      ^^^^ variant `Foo::MyVariant` doesn't implement `Copy`

error[E0204]: the trait `Copy` may not be implemented for type `Bar`
  --> foo.rs:13:1
   |
9  | struct Bar {
   | - field `item` in this struct doesn't implement `Copy`
...
13 | impl Copy for Bar {}
   | ^^^^^^^^^^^^^^^^^^^^ field `item` doesn't implement `Copy`

error: aborting due to 2 previous errors

I am intrigued as to the rationale for variants to be indexed, while struct fields aren't.

eddyb · 2016-11-13T10:36:45Z

src/librustc/hir/map/collector.rs

+                    } else {
+                        // This should be replaced with `visit_struct_field` once there's a Node
+                        // for struct fields. In the meantime, register the parent node to have
+                        // something to point to.


Ugh, this. I think I've added nodes for fields in at least two branches already.
Do you mind waiting for #37676? It could land in less than a week and has the correct fix.

I hadn't seen that PR. That's quite a refactor! I have no problem waiting for it.

estebank · 2016-11-29T00:54:40Z

@jonathandturner, @eddyb: after #37676 landing, now both cases point to the element (variant/field) that doesn't implement Copy:

error[E0205]: the trait `Copy` may not be implemented for type `Foo`
  --> $DIR/issue-19950.rs:17:6
   |
14 |     MyVariant(NoCopy)
   |     ----------------- this variant doesn't implement `Copy`
...
17 | impl Copy for Foo {}
   |      ^^^^ variant `Foo::MyVariant` doesn't implement `Copy`

error[E0204]: the trait `Copy` may not be implemented for type `Bar`
  --> $DIR/issue-19950.rs:23:1
   |
20 |     item: NoCopy,
   |     ------------ this field doesn't implement `Copy`
...
23 | impl Copy for Bar {}
   | ^^^^^^^^^^^^^^^^^^^^ field `item` in struct `Bar` doesn't implement `Copy`

sophiajt · 2016-11-29T01:12:44Z

@estebank - interesting. so if that's true, does that mean the main issue here got fixed?

estebank · 2016-11-29T01:23:44Z

@jonathandturner #37676 added spans for struct fields to the map, which allows me to point at the correct field in this error.

The current output doesn't point at the field or variant that doesn't implement Copy:

error[E0205]: the trait `Copy` may not be implemented for this type
 --> <anon>:7:6
  |
7 | impl Copy for Foo {}
  |      ^^^^ variant `MyVariant` does not implement `Copy`

error[E0204]: the trait `Copy` may not be implemented for this type
  --> <anon>:13:1
   |
13 | impl Copy for Bar {}
   | ^^^^^^^^^^^^^^^^^^^^ field `item` does not implement `Copy`

sophiajt · 2016-11-29T01:24:56Z

Oh I got it! That's awesome then. Ready for review?

estebank · 2016-11-29T01:38:07Z

Yeah! 👍

sophiajt · 2016-11-30T19:51:33Z

src/librustc_typeck/coherence/mod.rs

-                                            name,
-                                            self_type));
+                                   &format!("field `{}` doesn't implement `Copy`",
+                                            name));
                    if let Some(def_span) = tcx.map.span_if_local(did) {
                        err.span_label(def_span, &"this field doesn't implement `Copy`");
                    }


I was thinking something more like:

if let Some(def_span) = tcx.map.span_if_local(did) { err.span_label(def_span, &"this field doesn't implement `Copy`"); err.span_label(span, &format!("field `{}` doesn't implement `Copy`", name)); } else { err.span_label(span, &format!("field `{}` in struct `{}` doesn't implement `Copy`", name, self_type)); }

That way, in the case we don't have the def_span available, we can still give them a little more hint as to where it's coming from.

@jonathandturner We should talk about a transition plan: ideally, span_if_local wouldn't exist after #37954, as every DefId has an associated Span.
The problem is that there's no source available and there's a few ways in which we could approach this (including embedding the full source in metadata).
In any case, long-term I'd like to not have code that branches out like this, but rather generate "field bar of struct Foo" from just its DefId, and point to its span if local, unless we can somehow show the source itself.

Ah okay, got it. We can experiment with that later, but sounds like that'll affect more than just this PR. I don't want to hold this one up while we figure out the bigger picture.

sophiajt · 2016-12-01T03:06:41Z

@bors r+

bors · 2016-12-01T03:06:42Z

📌 Commit 7ba99a4 has been approved by jonathandturner

bors · 2016-12-01T06:51:12Z

⌛ Testing commit 7ba99a4 with merge c75925e...

bors · 2016-12-01T06:59:18Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

estebank · 2016-12-01T08:04:20Z

Had to rebase against master to fix error. Tests have completed, I'll be needing a new r+, @jonathandturner.

sophiajt · 2016-12-02T18:07:40Z

@bors r+

bors · 2016-12-02T18:07:41Z

📌 Commit 44edb55 has been approved by jonathandturner

bors · 2016-12-02T18:15:06Z

⌛ Testing commit 44edb55 with merge a1b0040...

bors · 2016-12-02T19:43:49Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

sophiajt · 2016-12-02T19:46:41Z

Failure seems legit:

------------------------------------------
stderr:
------------------------------------------
error[E0204]: the trait `Copy` may not be implemented for type `Bar`
  --> C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src/test\ui\span\issue-19950.rs:23:1
   |
20 |     item: NoCopy,
   |     ------------ this field doesn't implement `Copy`
...
23 | impl Copy for Bar {}
   | ^^^^^^^^^^^^^^^^^^^^ field `item` doesn't implement `Copy`

error[E0205]: the trait `Copy` may not be implemented for type `Foo`
  --> C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src/test\ui\span\issue-19950.rs:17:6
   |
14 |     MyVariant(NoCopy)
   |     ----------------- this variant doesn't implement `Copy`
...
17 | impl Copy for Foo {}
   |      ^^^^ variant `Foo::MyVariant` doesn't implement `Copy`

error: aborting due to 2 previous errors


------------------------------------------

thread '[ui] ui\span\issue-19950.rs' panicked at 'explicit panic', C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src\tools\compiletest\src\runtest.rs:2407
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    [ui] ui\span\issue-19950.rs

estebank · 2016-12-05T04:00:05Z

Just took a look at it and this was caused because the ordering of messages is not consistent across platforms. Is it ok if I split this test case into two separate files while I work on ordering the output of errors in a separate PR?

sophiajt · 2016-12-06T16:47:20Z

@estebank - are you saying that a single test might give multiple results non-deterministically? If that's the case, that sounds bad (and something we should definitely fix)

estebank · 2016-12-06T18:39:28Z

@jonathandturner The error was that the order of the errors was swapped. Given it failed only in win32 that made me think that the order the errors are emitted is different in different platforms (even though that makes little sense). I think more likely there was a change in the emit order between the time it got tested by travis and the time it go tested by bors.

bors · 2016-12-16T07:28:13Z

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

bors · 2016-12-21T02:53:39Z

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

bors · 2016-12-28T23:09:42Z

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

Change error message to ```nocode error[E0205]: the trait `Copy` may not be implemented for type `Foo` --> $DIR/issue-19950.rs:17:6 | 14 | MyVariant(NoCopy) | ----------------- this variant doesn't implement `Copy` ... 17 | impl Copy for Foo {} | ^^^^ variant `Foo::MyVariant` doesn't implement `Copy` error[E0204]: the trait `Copy` may not be implemented for type `Bar` --> $DIR/issue-19950.rs:23:1 | 20 | item: NoCopy, | ------------ this field doesn't implement `Copy` ... 23 | impl Copy for Bar {} | ^^^^^^^^^^^^^^^^^^^^ field `item` in struct `Bar` doesn't implement `Copy` ```

bors · 2017-01-05T15:08:53Z

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

arielb1 · 2017-01-05T17:42:48Z

Superseded by #38152.

Sorry, didn't notice this PR was open. Thanks for the PR!

rust-highfive assigned Aatch Oct 31, 2016

estebank force-pushed the point-to-missing-copy-span-19950 branch 2 times, most recently from 9a416ad to 9575724 Compare October 31, 2016 19:07

rust-highfive assigned sophiajt and unassigned Aatch Nov 7, 2016

estebank force-pushed the point-to-missing-copy-span-19950 branch 2 times, most recently from b4a9ca1 to fdc8777 Compare November 10, 2016 05:43

alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 10, 2016

brson reviewed Nov 11, 2016

View reviewed changes

estebank force-pushed the point-to-missing-copy-span-19950 branch from 27ef67e to df8a29a Compare November 13, 2016 01:28

estebank force-pushed the point-to-missing-copy-span-19950 branch from df8a29a to c19113b Compare November 13, 2016 03:09

eddyb reviewed Nov 13, 2016

View reviewed changes

estebank force-pushed the point-to-missing-copy-span-19950 branch from c19113b to 9c8f7fa Compare November 28, 2016 23:48

estebank force-pushed the point-to-missing-copy-span-19950 branch from 2b35c63 to 7ba99a4 Compare November 30, 2016 02:26

sophiajt reviewed Nov 30, 2016

View reviewed changes

estebank force-pushed the point-to-missing-copy-span-19950 branch from 7ba99a4 to 44edb55 Compare December 1, 2016 08:02

estebank force-pushed the point-to-missing-copy-span-19950 branch 2 times, most recently from 83ffdbc to a8e24a2 Compare December 5, 2016 05:55

estebank force-pushed the point-to-missing-copy-span-19950 branch from a8e24a2 to 8c640fe Compare December 20, 2016 18:53

estebank force-pushed the point-to-missing-copy-span-19950 branch from 8c640fe to 8d5c21a Compare December 22, 2016 05:31

estebank force-pushed the point-to-missing-copy-span-19950 branch from 8d5c21a to d9d75b5 Compare December 28, 2016 23:30

arielb1 closed this Jan 5, 2017

Show span for trait that doesn't implement Copy #37493

Show span for trait that doesn't implement Copy #37493

Conversation

estebank commented Oct 31, 2016

rust-highfive commented Oct 31, 2016

estebank commented Oct 31, 2016

estebank commented Nov 7, 2016

sophiajt commented Nov 8, 2016

estebank commented Nov 8, 2016 • edited Loading

estebank commented Nov 11, 2016

brson Nov 11, 2016

Choose a reason for hiding this comment

brson commented Nov 11, 2016 • edited Loading

brson commented Nov 11, 2016 • edited Loading

estebank commented Nov 12, 2016

sophiajt commented Nov 12, 2016

estebank commented Nov 12, 2016

estebank commented Nov 13, 2016

eddyb Nov 13, 2016

Choose a reason for hiding this comment

estebank Nov 14, 2016

Choose a reason for hiding this comment

estebank commented Nov 29, 2016

sophiajt commented Nov 29, 2016

estebank commented Nov 29, 2016

sophiajt commented Nov 29, 2016 • edited Loading

estebank commented Nov 29, 2016

sophiajt Nov 30, 2016

Choose a reason for hiding this comment

eddyb Nov 30, 2016

Choose a reason for hiding this comment

sophiajt Dec 1, 2016

Choose a reason for hiding this comment

sophiajt commented Dec 1, 2016

bors commented Dec 1, 2016

bors commented Dec 1, 2016

bors commented Dec 1, 2016

estebank commented Dec 1, 2016 • edited Loading

sophiajt commented Dec 2, 2016

bors commented Dec 2, 2016

bors commented Dec 2, 2016

bors commented Dec 2, 2016

sophiajt commented Dec 2, 2016

estebank commented Dec 5, 2016 • edited Loading

sophiajt commented Dec 6, 2016

estebank commented Dec 6, 2016

bors commented Dec 16, 2016

bors commented Dec 21, 2016

bors commented Dec 28, 2016

bors commented Jan 5, 2017

arielb1 commented Jan 5, 2017 • edited Loading

estebank commented Nov 8, 2016 •

edited

Loading

brson commented Nov 11, 2016 •

edited

Loading

brson commented Nov 11, 2016 •

edited

Loading

sophiajt commented Nov 29, 2016 •

edited

Loading

estebank commented Dec 1, 2016 •

edited

Loading

estebank commented Dec 5, 2016 •

edited

Loading

arielb1 commented Jan 5, 2017 •

edited

Loading