Call out the types that are non local on E0117 #65318

estebank · 2019-10-11T19:39:04Z

CC #24745.

rust-highfive · 2019-10-11T19:39:07Z

r? @varkor

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

src/librustc_typeck/coherence/orphan.rs

src/test/ui/coherence/coherence-fundamental-trait-objects.old.stderr

varkor · 2019-10-11T21:29:34Z

src/test/ui/coherence/coherence-impls-copy.old.stderr

@@ -60,7 +60,7 @@ error[E0117]: only traits defined in the current crate can be implemented for ar
 LL | impl Copy for (MyType, MyType) {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate
   |
-   = note: the impl does not reference only types defined in this crate
+   = note: `(MyType, MyType)` is not defined in the current create


This should just be MyType.

I'm not sure why this error is emitted at all, I think it might be due to the tuple? I don't know how we'd change the wording to be more accurate.

If it's possible to impl for MyType, it should also be possible to impl for (MyType, MyType), so I think we should emit the types that are causing the root problem.

Implementing a trait external to the current crate on a tuple, array or slice of a type from the current crate is not ever allowed. I extended the output to make that a bit clearer, but can add more text if that would help.

varkor · 2019-10-13T15:06:38Z

src/test/ui/coherence/coherence-fundamental-trait-objects.old.stderr

-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate
+   | ^^^^^^^^^^^^^^----------------------
+   | |             |
+   | |             `(dyn coherence_fundamental_trait_lib::Fundamental<Local> + 'static)` is not defined in the current crate


I think this should be `coherence_fundamental_trait_lib::Fundamental<Local>` is not defined in the current crate. Having the dyn and lifetime bound is confusing (also + 'static is default, so needn't be included).

I couldn't remove the dyn, but that shouldn't be too big of a problem.

src/test/ui/coherence/coherence-impls-copy.old.stderr

estebank · 2019-10-13T18:32:39Z

src/librustc_typeck/coherence/orphan.rs

+                    err.span_label(sp, "impl doesn't use only types from inside the current crate");
+                    for (ty, is_target_ty) in &tys {
+                        // FIXME: We want to remove the type arguments from the displayed type.
+                        //        The reverse of `resolve_vars_if_possible`.


Does anyone know what the proper way of "reversing" resolve_vars_if_possible would be?

varkor · 2019-10-20T12:43:40Z

src/test/ui/coherence/coherence-fundamental-trait-objects.old.stderr

-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate
+   | ^^^^^^^^^^^^^^----------------------
+   | |             |
+   | |             `dyn coherence_fundamental_trait_lib::Fundamental<Local>` is not defined in the current crate


I think we can get away with just printing the path, without dyn or the generics, etc. E.g. here we would just have:

`coherence_fundamental_trait_lib::Fundamental` is not defined in the current crate

because dyn and <Local> are red herrings: they're not what's causing the error (if they were, we should have specific error messages calling them out, like array impls).

Ah, sorry, I just read your comment here: #65318 (comment). Sorry if I seem like a broken record: I'm just worried that these more specific error messages might be more confusing when they contain this extra information.

I'm looking over the current test output, and the only cases where we mention dyn already have dyn in the code (or would if the user follows the warnings). The dyn is written unconditionally in the pprinting machinery, and removing would only work for some cases (you could have (dyn T + K), which couldn't be represented with acurately bare path).

varkor · 2019-10-24T20:30:05Z

src/test/ui/coherence/coherence-orphan.old.stderr

+   | ^^^^^---------------^^^^^-----
+   | |    |                   |
+   | |    |                   `isize` is not defined in the current crate
+   | |    `usize` is not defined in the current crate


The span for usize is a bit odd: I would expect it to point to the generic argument.

We don't have a way to point at specific type params at the moment, but if be happy to add that in a follow up PR.

I do believe it will be fairly involved to get the right span, which is why I'd like to do it after merging this.

Okay, sounds good to me :)

varkor · 2019-10-24T20:32:54Z

src/librustc/traits/coherence.rs

@@ -237,7 +237,7 @@ pub fn trait_ref_is_local_or_fundamental<'tcx>(
 }

 pub enum OrphanCheckErr<'tcx> {
-    NoLocalInputType,
+    NonLocalInputType(Vec<(Ty<'tcx>, bool)>),


It would be nice having a comment here explaining what the field means.

varkor · 2019-10-24T20:33:08Z

r=me with the small span comment addressed.

estebank · 2019-10-27T18:49:44Z

@bors r=varkor

bors · 2019-10-27T18:49:46Z

📌 Commit dc41d6f18c6c9756e83a53193e2376531f0cdfeb has been approved by varkor

bors · 2019-10-27T19:40:48Z

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

estebank · 2019-10-28T18:19:16Z

@bors r=varkor

bors · 2019-10-28T18:19:18Z

📌 Commit 627691f has been approved by varkor

Call out the types that are non local on E0117 CC rust-lang#24745.

@ghost

Rollup of 5 pull requests Successful merges: - #65294 (Lint ignored `#[inline]` on function prototypes) - #65318 (Call out the types that are non local on E0117) - #65531 (Update backtrace to 0.3.40) - #65562 (Improve the "try using a variant of the expected type" hint.) - #65809 (Add new EFIAPI ABI) Failed merges: r? @ghost

bors · 2019-10-29T07:38:32Z

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

rust-highfive assigned varkor Oct 11, 2019

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

This comment has been minimized.

Sign in to view

estebank force-pushed the coherence branch from 0149a61 to 7042b0a Compare October 11, 2019 20:26

cramertj reviewed Oct 11, 2019

View reviewed changes

src/librustc_typeck/coherence/orphan.rs Outdated Show resolved Hide resolved

varkor reviewed Oct 11, 2019

View reviewed changes

src/test/ui/coherence/coherence-fundamental-trait-objects.old.stderr Outdated Show resolved Hide resolved

varkor reviewed Oct 11, 2019

View reviewed changes

varkor reviewed Oct 13, 2019

View reviewed changes

src/test/ui/coherence/coherence-impls-copy.old.stderr Outdated Show resolved Hide resolved

estebank commented Oct 13, 2019

View reviewed changes

varkor reviewed Oct 20, 2019

View reviewed changes

estebank force-pushed the coherence branch from d21742f to 0f47ce5 Compare October 24, 2019 00:06

varkor reviewed Oct 24, 2019

View reviewed changes

varkor 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 Oct 25, 2019

estebank force-pushed the coherence branch from 0f47ce5 to dc41d6f Compare October 27, 2019 18:49

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 Oct 27, 2019

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 27, 2019

estebank added 4 commits October 28, 2019 09:56

Call out the types that are non local on E0117

adfe9a4

Use more targeted spans for orphan rule errors

9b4f811

Further tweak spans for better readability

56aa89c

Talk about specific types and remove lifetimes from output

daeafd8

estebank added 4 commits October 28, 2019 09:57

Account for tuples in explanation

db1bfbd

Do not display ADT type arguments and fix rebase

95364df

add comment

2cd28c1

Fix rebase

627691f

estebank force-pushed the coherence branch from dc41d6f to 627691f Compare October 28, 2019 17:43

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 Oct 28, 2019

Centril added a commit to Centril/rust that referenced this pull request Oct 29, 2019

Rollup merge of rust-lang#65318 - estebank:coherence, r=varkor

14a9a50

Call out the types that are non local on E0117 CC rust-lang#24745.

Centril mentioned this pull request Oct 29, 2019

Rollup of 8 pull requests #65915

Closed

Centril added a commit to Centril/rust that referenced this pull request Oct 29, 2019

Rollup merge of rust-lang#65318 - estebank:coherence, r=varkor

515dce5

Call out the types that are non local on E0117 CC rust-lang#24745.

Centril mentioned this pull request Oct 29, 2019

Rollup of 6 pull requests #65917

Closed

Centril added a commit to Centril/rust that referenced this pull request Oct 29, 2019

Rollup merge of rust-lang#65318 - estebank:coherence, r=varkor

b07e8ed

Call out the types that are non local on E0117 CC rust-lang#24745.

Centril mentioned this pull request Oct 29, 2019

Rollup of 5 pull requests #65919

Merged

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 29, 2019

bors merged commit 627691f into rust-lang:master Oct 29, 2019

estebank deleted the coherence branch November 9, 2023 05:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call out the types that are non local on E0117 #65318

Call out the types that are non local on E0117 #65318

estebank commented Oct 11, 2019

rust-highfive commented Oct 11, 2019

This comment has been minimized.

varkor Oct 11, 2019

estebank Oct 12, 2019

varkor Oct 12, 2019

estebank Oct 23, 2019 •

edited

Loading

varkor Oct 13, 2019

estebank Oct 13, 2019

estebank Oct 13, 2019

varkor Oct 20, 2019

varkor Oct 20, 2019

estebank Oct 23, 2019

varkor Oct 24, 2019 •

edited

Loading

estebank Oct 24, 2019 •

edited

Loading

varkor Oct 24, 2019

varkor Oct 24, 2019

varkor commented Oct 24, 2019

estebank commented Oct 27, 2019

bors commented Oct 27, 2019

bors commented Oct 27, 2019

estebank commented Oct 28, 2019

bors commented Oct 28, 2019

bors commented Oct 29, 2019

Call out the types that are non local on E0117 #65318

Call out the types that are non local on E0117 #65318

Conversation

estebank commented Oct 11, 2019

rust-highfive commented Oct 11, 2019

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Oct 23, 2019 • 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

varkor Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

estebank Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varkor commented Oct 24, 2019

estebank commented Oct 27, 2019

bors commented Oct 27, 2019

bors commented Oct 27, 2019

estebank commented Oct 28, 2019

bors commented Oct 28, 2019

bors commented Oct 29, 2019

estebank Oct 23, 2019 •

edited

Loading

varkor Oct 24, 2019 •

edited

Loading

estebank Oct 24, 2019 •

edited

Loading