On privacy error caused by private reexport, use spans to show the `use` chain #67951

estebank · 2020-01-06T19:49:00Z

Fix #63942, fix #57619. Partially address #42909.

rust-highfive · 2020-01-06T19:49:04Z

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

src/librustc/infer/error_reporting/mod.rs

estebank · 2020-01-06T19:50:46Z

src/librustc_resolve/lib.rs

+                                                self.definitions
+                                                    .def_path(def_id.index)
+                                                    .to_string_no_crate(),


@petrochenkov would you have some advice on how to get a better suited source for a usable path string in resolve?

There are zero guarantees that a direct path won't have private segments, especially given that module structures with facades are pretty popular.
So trying to guess and use full paths here means inviting false positives.

I'd rather keep the status quo and say only the name.
(In this case the changes in src/librustc/hir/map/definitions.rs can be removed as well, those strings are not user-facing otherwise and are used only for debugging.)

estebank · 2020-01-06T19:51:55Z

src/librustc_resolve/lib.rs

+                                    // FIXME: we should verify that this def is actually
+                                    // reachable from the user's crate, as the parent modules
+                                    // of this ADT might be private.


@petrochenkov would you have some advice on how to properly check the visibility of a given ADT here? I'm guessing I would have to ferry some scope information in PrivacyError.

This issue doesn't apply to the simpler version of this PR (#67951 (comment)) which I'd like to see implemented first.
I'm not sure we'll need anything beyond that.

src/test/ui/imports/issue-55884-2.stderr

src/test/ui/export-tag-variant.stderr

src/librustc_resolve/lib.rs

src/test/ui/imports/issue-55884-2.stderr

varkor

I really like these more informative errors! They could just do with a little tweaking for readability.

src/librustc_resolve/lib.rs

src/test/ui/error-codes/E0603.stderr

src/test/ui/extern/extern-crate-visibility.stderr

src/test/ui/imports/issue-55884-2.stderr

petrochenkov · 2020-01-11T14:24:27Z

r? @petrochenkov

src/librustc_resolve/lib.rs

src/test/ui/extern/extern-crate-visibility.rs

src/test/ui/xcrate/xcrate-private-by-default.stderr

petrochenkov · 2020-01-11T18:39:52Z

I'm not sure that reporting the full reexports chains is useful.
The important thing to report here is that the import we are trying to access is private.

So, I'd replace this:

error[E0603]: module `bar` is private
  --> $DIR/shadowed-use-visibility.rs:9:14
   |
LL |     use foo::bar::f as g;
   |              ^^^ this module is private
   |
note: used restricted re-export of `bar`
  --> $DIR/shadowed-use-visibility.rs:4:9
   |
LL |     use foo as bar;
   |         ^^^^^^^^^^
note: re-exported module `bar` is declared here
  --> $DIR/shadowed-use-visibility.rs:1:1
   |
LL | / mod foo {
LL | |     pub fn f() {}
LL | |
LL | |     use foo as bar;
LL | |     pub use self::f as bar;
LL | | }
   | |_^

with this

error[E0603]: module import `bar` is private
  --> $DIR/shadowed-use-visibility.rs:9:14
   |
LL |     use foo::bar::f as g;
   |              ^^^ this import is private
   |
note: the module import `bar` is declared here
  --> $DIR/shadowed-use-visibility.rs:4:9
   |
LL |     use foo as bar;
   |         ^^^^^^^^^^

This would be a pretty minimal change to the code existing on master.
This PR is trying to be too smart an do too much in a single step, so it's hard to see what is essential and what can be removed to keep the code simple.

estebank · 2020-01-12T01:03:22Z

The important thing to report here is that the import we are trying to access is private.

The concern I have is that by only stating "you're importing something private, and this is the private re-export" and nothing else we don't give users a chance of changing their use statement. By pointing the whole chain of re-exports all the way to the original definition, we might be giving a lot of unnecessary information, but we're giving the user a good idea of whether they want to import the original struct directly, or one of the public reexports. An option would be to only show the chain of public re-export and hide the private ones, but that "inconsistency" on what we show could be confusing.

petrochenkov · 2020-01-12T13:23:01Z

I'd still want to see the diff between the basic version and what this PR does, so I made #68153.

petrochenkov · 2020-01-12T13:23:28Z

Blocked on #68153 and #67951 (comment).

@estebank

resolve: Point at the private item definitions in privacy errors A basic version of rust-lang#67951. r? @estebank

@estebank

resolve: Point at the private item definitions in privacy errors A basic version of rust-lang#67951. r? @estebank

@estebank

resolve: Point at the private item definitions in privacy errors A basic version of rust-lang#67951. r? @estebank

Dylan-DPC-zz · 2020-02-15T20:48:51Z

@estebank this is unblocked now

…se` chain Use full path for direct `use` of ADT instead of private re-export Point at enum defs and modules on private re-exports Use span notes to denote order Account for `use` of private `extern crate foo;`

estebank · 2020-02-18T00:42:46Z

@Dylan-DPC thanks for the ping.

@petrochenkov rebased and squashed on top of latest master, the diff is much smaller now.

estebank · 2020-02-18T00:44:40Z

src/test/ui/imports/issue-55884-2.stderr

+  --> $DIR/issue-55884-2.rs:6:13
+   |
+LL |     pub use options::*;
+   |             ^^^^^^^^^^


I think this clarification in particular will be incredibly useful when trying to understand these errors.

petrochenkov · 2020-03-01T10:51:08Z

I will get to this next weekend.

petrochenkov · 2020-03-07T21:50:51Z

I'll close this in favor of #69811 which does roughly the same thing.

@estebank

resolve: Print import chains on privacy errors A part of rust-lang#67951 that doesn't require hacks. r? @estebank

@estebank

resolve: Print import chains on privacy errors A part of rust-lang#67951 that doesn't require hacks. r? @estebank

@estebank

resolve: Print import chains on privacy errors A part of rust-lang#67951 that doesn't require hacks. r? @estebank

@estebank

resolve: Print import chains on privacy errors A part of rust-lang#67951 that doesn't require hacks. r? @estebank

rust-highfive assigned zackmdavis Jan 6, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2020

estebank commented Jan 6, 2020

View reviewed changes

Centril reviewed Jan 6, 2020

View reviewed changes

src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved

petrochenkov self-assigned this Jan 8, 2020

varkor reviewed Jan 10, 2020

View reviewed changes

src/test/ui/imports/issue-55884-2.stderr Outdated Show resolved Hide resolved

varkor reviewed Jan 10, 2020

View reviewed changes

estebank force-pushed the private-reexport branch from 45e2df4 to 883a86b Compare January 10, 2020 03:24

rust-highfive unassigned zackmdavis Jan 11, 2020

petrochenkov reviewed Jan 11, 2020

View reviewed changes

src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Jan 11, 2020

View reviewed changes

src/test/ui/extern/extern-crate-visibility.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Jan 11, 2020

View reviewed changes

src/test/ui/xcrate/xcrate-private-by-default.stderr Outdated Show resolved Hide resolved

petrochenkov 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 Jan 11, 2020

petrochenkov mentioned this pull request Jan 12, 2020

resolve: Point at the private item definitions in privacy errors #68153

Merged

petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2020

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 16, 2020

Rollup merge of rust-lang#68153 - petrochenkov:privdiag, r=estebank

d349273

resolve: Point at the private item definitions in privacy errors A basic version of rust-lang#67951. r? @estebank

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 16, 2020

Rollup merge of rust-lang#68153 - petrochenkov:privdiag, r=estebank

b515e1f

resolve: Point at the private item definitions in privacy errors A basic version of rust-lang#67951. r? @estebank

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 17, 2020

Rollup merge of rust-lang#68153 - petrochenkov:privdiag, r=estebank

ecf42a3

resolve: Point at the private item definitions in privacy errors A basic version of rust-lang#67951. r? @estebank

This comment has been minimized.

Sign in to view

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jan 27, 2020

On privacy error caused by private reexport, use spans to show the `u…

b3a554d

…se` chain Use full path for direct `use` of ADT instead of private re-export Point at enum defs and modules on private re-exports Use span notes to denote order Account for `use` of private `extern crate foo;`

estebank force-pushed the private-reexport branch from 65c8532 to b3a554d Compare February 18, 2020 00:40

estebank commented Feb 18, 2020

View reviewed changes

petrochenkov 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 Feb 18, 2020

petrochenkov mentioned this pull request Mar 7, 2020

resolve: Print import chains on privacy errors #69811

Merged

petrochenkov closed this Mar 7, 2020

Centril added a commit to Centril/rust that referenced this pull request Mar 10, 2020

Rollup merge of rust-lang#69811 - petrochenkov:privdiag2, r=estebank

6a77a75

resolve: Print import chains on privacy errors A part of rust-lang#67951 that doesn't require hacks. r? @estebank

Centril added a commit to Centril/rust that referenced this pull request Mar 16, 2020

Rollup merge of rust-lang#69811 - petrochenkov:privdiag2, r=estebank

9b26188

resolve: Print import chains on privacy errors A part of rust-lang#67951 that doesn't require hacks. r? @estebank

Centril added a commit to Centril/rust that referenced this pull request Mar 17, 2020

Rollup merge of rust-lang#69811 - petrochenkov:privdiag2, r=estebank

b691145

resolve: Print import chains on privacy errors A part of rust-lang#67951 that doesn't require hacks. r? @estebank

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 17, 2020

Rollup merge of rust-lang#69811 - petrochenkov:privdiag2, r=estebank

f6703ef

resolve: Print import chains on privacy errors A part of rust-lang#67951 that doesn't require hacks. r? @estebank

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On privacy error caused by private reexport, use spans to show the `use` chain #67951

On privacy error caused by private reexport, use spans to show the `use` chain #67951

estebank commented Jan 6, 2020 •

edited

Loading

rust-highfive commented Jan 6, 2020

estebank Jan 6, 2020

petrochenkov Jan 11, 2020

estebank Jan 6, 2020

petrochenkov Jan 11, 2020

varkor left a comment

petrochenkov commented Jan 11, 2020

petrochenkov commented Jan 11, 2020

estebank commented Jan 12, 2020

petrochenkov commented Jan 12, 2020

petrochenkov commented Jan 12, 2020

This comment has been minimized.

Dylan-DPC-zz commented Feb 15, 2020

estebank commented Feb 18, 2020

estebank Feb 18, 2020

petrochenkov commented Mar 1, 2020

petrochenkov commented Mar 7, 2020

On privacy error caused by private reexport, use spans to show the use chain #67951

On privacy error caused by private reexport, use spans to show the use chain #67951

Conversation

estebank commented Jan 6, 2020 • edited Loading

rust-highfive commented Jan 6, 2020

estebank Jan 6, 2020

Choose a reason for hiding this comment

petrochenkov Jan 11, 2020

Choose a reason for hiding this comment

estebank Jan 6, 2020

Choose a reason for hiding this comment

petrochenkov Jan 11, 2020

Choose a reason for hiding this comment

varkor left a comment

Choose a reason for hiding this comment

petrochenkov commented Jan 11, 2020

petrochenkov commented Jan 11, 2020

estebank commented Jan 12, 2020

petrochenkov commented Jan 12, 2020

petrochenkov commented Jan 12, 2020

This comment has been minimized.

Dylan-DPC-zz commented Feb 15, 2020

estebank commented Feb 18, 2020

estebank Feb 18, 2020

Choose a reason for hiding this comment

petrochenkov commented Mar 1, 2020

petrochenkov commented Mar 7, 2020

On privacy error caused by private reexport, use spans to show the `use` chain #67951

On privacy error caused by private reexport, use spans to show the `use` chain #67951

estebank commented Jan 6, 2020 •

edited

Loading