The compiler should report publicly exported type names if possible

Opened by Patrick Walton at 2022-03-02 19:01:43

If a private type is reexported at some public location, the Rust compiler will report the private location when referring to the type. It'd be more helpful if the compiler reported the public alias for it.

This was reported by Coda Hale on Twitter.

Update by pnkfelix: here is a standalone bit of code illustrating the problem. (play):

mod a {
    mod m { pub struct PrivateName; }
    pub use m::PrivateName as PublicName;
}

fn main() {
    // a::m::PrivateName; // would error, "module `m` is private"
    a::PublicName;
    a::PublicName.clone();
}

As of nightly 2019-06-17, this issues the diagnostic:

> error[E0599]: no method named `clone` found for type `a::m::PrivateName` [...]
                                                        ^^^^^^^^^^^^^^^^^
                                                                 |
             (author of `fn main` wants to see `a::PublicName` here)

Felix S Klock II at 2019-06-21 14:23:35

This happens a lot with core/std.
Steve Klabnik at 2015-02-04 23:21:23

Triage: it seems like a private type can no longer be exported as a public type at all

pub mod A {
    struct Foo;
    pub use A::Foo as Bar;
}

fn main() {
    let b = A::Bar;
}

gives

hello.rs:5:13: 5:26 error: `Foo` is private, and cannot be reexported [E0364]
hello.rs:5     pub use A::Foo as Bar;
                       ^~~~~~~~~~~~~
hello.rs:5:13: 5:26 help: run `rustc --explain E0364` to see a detailed explanation
hello.rs:5:13: 5:26 note: consider marking `Foo` as `pub` in the imported module
hello.rs:5     pub use A::Foo as Bar;
                       ^~~~~~~~~~~~~

So I believe this is effectively fixed? My core/std comment is more about reexports where both are public:

pub mod A {
    pub struct Foo;
    pub use A::Foo as Bar;
}

fn main() {
    let b: i32 = A::Bar;
}

which gives

hello.rs:9:18: 9:24 error: mismatched types:
 expected `i32`,
    found `A::Foo`
(expected i32,
    found struct `A::Foo`) [E0308]
hello.rs:9     let b: i32 = A::Bar;
                            ^~~~~~
hello.rs:9:18: 9:24 help: run `rustc --explain E0308` to see a detailed explanation
error: aborting due to previous error

which is covered by another ticket whose number escapes me.

Steve Klabnik at 2016-03-04 16:53:36

The issue is relevant to "unnameable" pub types which are still allowed.
```
mod m {
    pub struct PrivateName;
}

pub use m::PrivateName as PublicName.
```
m::PrivateName is still used in error messages, while it's preferable to use PublicName.
Vadim Petrochenkov at 2016-03-04 17:08:26

It also reports names which don't exist:

Consider this library:

// lib.rs of crate foo

mod a {
    pub trait Foo {}
}

pub use a::Foo as Afoo;

pub fn foo<T: Afoo>() {}

And some binary crate using it:

extern crate foo;

use foo::foo;

fn main() {
    foo::<()>(); // Afoo is not implemented for `()`
}

The error message is:

error[E0277]: the trait bound `(): foo::Foo` is not satisfied
 --> src/main.rs:6:5
  |
6 |     foo::<()>();
  |     ^^^^^^^^^ the trait `foo::Foo` is not implemented for `()`
  |
  = note: required by `foo::foo`

So it does not report the private name but a name that does not exist.

Thomas Schaller at 2017-02-19 17:47:19

Notes from my attempt at solving this:
- The module_exports query is helpful as it returns a list of Exports in a module, the ident field is the name or rename.
- Modifying item_path.rs directly is problematic because it is used in many different parts of the compiler. It's unclear where exactly the logic should go.
Also linking relevant internal's thread.
Leonardo Yvens at 2018-01-16 19:51:03
I don't know if this deserves a P-high priority for the T-compiler team.

I definitely know that its a foot-gun, but maybe there's some stop-gap measure we can provide (e.g. an attribute saying what path to use in diagnostic output?)

Nominated for discussion there, so that the group can decide how this should be prioritized.
Felix S Klock II at 2019-06-21 14:40:49
I think this should be P-high for WG-diagnostics if it isn't for the compiler team.

For many people, the problem here represents one of the worsts aspects of the compiler and makes type mismatch diagnostics inscrutable. Also, if you fix this issue, it may take off some of the pressure to make highly specialized diagnostics because of a general improvement across the board.
Mazdak Farrokhzad at 2019-06-25 14:20:12
discussed at T-compiler meeting. Main issue is probably finding someone to do the work. Assigning to @estebank for now to take point on either being the champion or finding a champion. Assigning to self to make sure it doesn't fall through cracks. Removing nomination.

Update: in case its not clear, another important step, after finding a champion, is to actually propose the plan to fix it and get general approval from the team. It might not be worth an RFC; it might not even be worth a slot at the weekly steering/design meeting (though that's debatable) but it should probably not be something that someone solves in isolation without double-checking with @rust-lang/compiler team.
Felix S Klock II at 2019-07-04 14:46:29
As a random note, there could be some additional incentive to build parts of the necessary infrastructure for trying to incrementalize name resolution, and also help other diagnostics/rustdoc/rls (when they want to reason about scopes and the definitions that can be accessed through them).

The other side of the necessary infrastructure could be built independently, with much simpler information, and maybe not turned on by default if wildly inaccurate.

Quick sketch of how that might work:
- track the "current scope" somehow, ideally partially automated
  - maybe ok approximation: "queries that take a DefId have that as the scope"
- build a "reverse index of defs in scope" ((Namespace, DefId) -> Name)
  - keyed by whatever the "current scope" is
  - walk up to the enclosing module and use its children (incl. imports)
  - this may overlap a bit with the existing visible_parent_map
- when printing a path (see ty::print::pretty), check the scope
  - if the definition of a path is in scope, use that name
  - e.g. use crate::foo as x; would cause crate::foo::... to become x::...
  - can print self::Foo instead of just Foo to indicate module-relative
EDIT: can also use Spans, that would probably be better (and easier to get, since you can probably hook it up to e.g. struct_span_err!, and have it used specifically for formatting the types/etc. included in that message), but it might be harder to map from the Spans back to scopes (use binary search?).
Eduard-Mihai Burtescu at 2019-07-04 15:15:49
Another intersection I forgot to mention is with @rust-lang/wg-diagnostics: if we end up with a type-rich diagnostic system, that should let us be more precise about contextual things like the scope for all the paths in the message (or even better, only require that contextual information if we want to put any types or other entities that have paths, into the message).
Eduard-Mihai Burtescu at 2019-07-04 15:19:18
I can see that figuring out what name the user would prefer to see for a type is a difficult problem that requires threading a lot of information through to keep enough context information to make it possible.

However it feels to me that a large number of cases would be solved by a rather simple workaround: try to come up with a canonical path for every type outside of the crate currently being compiled. The canonical path would be one that is fully visible outside of the defining crate, and if there's multiple, the shortest one could be chosen. If it's still ambiguous, just choose a random one.

This requires no context information for look-ups because the canonical path can be figured out based on just the type itself. This isn't a perfect solution, but it gets rid of the worst problem in my opinion - reporting a private path that is not accessible to the user and not visible in any documentation.
Matti Virkkunen at 2019-08-27 12:21:32
just choose a random one.

Please don't introduce non-determinism. Instead use for example the first/last when lexically sorted.
bjorn3 at 2019-08-27 12:25:02
@bjorn3 I guess what I wanted to say is, "it doesn't have to be perfect, an educated guess is OK". Of course having an actually random output isn't very good. First one when sorted sounds good.
Matti Virkkunen at 2019-08-27 13:12:27
However it feels to me that a large number of cases would be solved by a rather simple workaround: try to come up with a canonical path for every type outside of the crate currently being compiled. The canonical path would be one that is fully visible outside of the defining crate, and if there's multiple, the shortest one could be chosen. If it's still ambiguous, just choose a random one.

I'm pretty sure this is literally what we do today?

This issue ~~is about~~ remains open for paths in the crate currently being compiled. We could do something similar to find a global path visible outside the current crate, but that still leaves unexported paths - a proper solution for requires context.
Eduard-Mihai Burtescu at 2019-08-27 15:20:31
@eddyb If there's a mention somewhere that visibility across crates is out of scope, I didn't see it. I did read through more of the referenced issues now though, and found issue #56175 which is more specific. It seems that what I described is indeed implemented, but only when using extern crate which is optional in the current edition. That's why I didn't notice it when testing this.
Matti Virkkunen at 2019-08-27 17:37:58
Sorry, I meant that this issue is left open for the intra-crate part of it.

And yeah, #56175 is one of the unfortunate "Rust2018 features needing more polish" cases.
Eduard-Mihai Burtescu at 2019-08-29 07:54:24
I and @estebank talked a bit about this on Twitter. This is very similar to @eddyb's approach above: https://github.com/rust-lang/rust/issues/21934#issuecomment-508515813 , with less magical query tracking.

Ideally, we should be able to do this without bloating memory usage. Some approaches for this involve storing local import information on Defs or Tys.

For https://github.com/rust-lang/rust/issues/43466 , we're probably going to need some kind of ScopeMap for each module, similar to the existing ExportMap except it also contains local scope info.

This is basically a map that lets you query a module (and hopefully, a function) defid and get a list of non-variable names in scope along with their defids. If we have scope information when emitting errors, we can resolve all emitted DefIds against it, looking for ones in scope already. We could go one level further so that we can suggest things like io::BufRead if io is in scope but not BufRead.

Now, we already print errors at spans. These spans are where the user is going to fix the code, and thus they're actually the most relevant bit of scope information for resolving paths. But the compiler doesn't really use spans that way so we can't use those -- we need a way to get the scope defid from the span. The most straightforward approach would be to stash this in the span or have some kind of map, but that would be super expensive.

But here's the thing: most if not all spans come from some AST/HIR node anyway! There isn't really any other major place we store spans, we usually extract them from some node and then pass them around on the stack. And these nodes have IDs which can be traced back to find their scope.

What we do is we introduce a new SpanAndScope type (produced by node.span_and_scope()) that's a (Span, HirId), which has a .scope(&tcx) method allowing you to obtain the scope of the HirId, which can then be used to clean up paths in diagnostics.

As far as I can tell this wouldn't impact heap memory at all since all the bloat is on the stack. The ScopeMap would cause some bloat, but maybe not too much if we make the ExportMap a shim over the ScopeMap, and we need ScopeMap for intra-doc-links anyway.

This can be done incrementally, you can choose any error and make it require a SpanAndScope instead of a Span, and trace it back up the call stack until you find the HIR node the Span was obtained from, and change .span to .span_and_scope(). Easy :smile: (in practice, i suspect there will be hiccups)

As a bonus, this gets even easier to do with https://github.com/rust-lang/rust/issues/21934, which can simply auto-resolve paths against any SpanAndScope it may have. I'm hoping to hack on that soon.
Manish Goregaokar at 2019-11-11 07:15:29
cc https://internals.rust-lang.org/t/pre-rfc-nicer-types-in-diagnostics/11139
Mazdak Farrokhzad at 2019-11-11 07:29:23
cc @da-x, looks like you've had a similar plan :smile:
Manish Goregaokar at 2019-11-11 07:34:41
@Manishearth, yes, though as a rustc newcomer my draft implementation is different. Not sure yet which one is better though.

I'm hoping to draft the RFC this or next month. Based on feedback in the pre-RFC, it would address three concerns:
1. Shorten paths only for types imported at error scope, such that all types paths printed conform to either of the two: uniform or shorted according to imports at error scope (no reachable by as previously presented).
2. Type compression via invented type variables (i.e. with where). This would be the tough bit, but I'm hopeful that it's doable.
3. Crate version disambiguity only where needed.
Impressed upon me is that without [2] the use case in #66269 and some of the use cases presented in the pre-RFC would still be unreadable.
Dan Aloni at 2019-11-11 08:38:46
Do we need an RFC for this? My impression is that this is purely an implementation thing, and doesn't need to go through the actual RFC process. We could do that if we wanted to add configurability (It's not clear to me that that is necessary!), but I think the right way to go would be to implement it behind an env var first and then see what the compiler team thinks.

Like, this is something most folks working on the compiler have wanted for ages, the question is just about how to implement it, which doesn't need an RFC.
Manish Goregaokar at 2019-11-11 08:49:26
@Manishearth, previously, I was requested an RFC over a smaller change - #49898, and submitted it. This one has even a larger user impact, so it only made sense to go through some RFC process, even if most of the work is implementation.
Dan Aloni at 2019-11-11 12:08:41
This one has a larger user impact, but a smaller design one: that one was changing the actual UI layout which has been very carefully designed. Full paths in diagnostics, on the other hand, are not an intentional part of the design, and everyone has wanted to fix it forever. I'd start experimenting first and make an rfc if the compiler team asks you to, not the other way around.

(I'll point out that @estebank is also experimenting with this without making an RFC and is on the compiler team)

On Mon, Nov 11, 2019, 4:09 AM Dan Aloni notifications@github.com wrote:

@Manishearth https://github.com/Manishearth, previously, I was requested an RFC over a smaller change - #49898 https://github.com/rust-lang/rust/pull/49898, and submitted it https://github.com/rust-lang/rfcs/pull/2777. This one has even a larger user impact, so it only made sense to go through some RFC process, even if most of the work is implementation.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/21934?email_source=notifications&email_token=AAMK6SHGXQUU7542S4EGI7DQTFDRBA5CNFSM4A3OUJW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDWT45Q#issuecomment-552418934, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMK6SDUY7MFNL6TITBUG5TQTFDRBANCNFSM4A3OUJWQ .

Manish Goregaokar at 2019-11-11 16:16:36
Like @Manishearth , I do not think an RFC is required here.

It might be nice to have a T-compiler design meeting for it (but even that might be overkill here; at the very least it would require synchronous participation from the proposer, which again may not be necessary here.)
Felix S Klock II at 2019-11-14 13:21:39
It's ok, as I prefer working on code than on RFCs anyway :)

I can try fitting the draft implementation to whatever is decided there.

On Thu, Nov 14, 2019, 15:22 Felix S Klock II notifications@github.com wrote:

Like @Manishearth https://github.com/Manishearth , I do not think an RFC is required here.

It might be nice to have a T-compiler design meeting https://rust-lang.github.io/compiler-team/about/steering-meeting/ for it (but even that might be overkill here; at the very least it would require synchronous participation from the proposer, which again may not be necessary here.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/21934?email_source=notifications&email_token=AACON6IFXOX7JF6O2WO53CDQTVGKFA5CNFSM4A3OUJW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEBZ2RA#issuecomment-553884996, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACON6J2OTV5OQEK5SZXQVTQTVGKFANCNFSM4A3OUJWQ .

Dan Aloni at 2019-11-14 13:27:57
triage: I am downgrading this to P-medium. We have too large of a backlog of high priority items and this one does not warrant being visited every week at triage.
Felix S Klock II at 2019-11-28 14:27:04
There was an idea I noted down in a pull request: https://github.com/rust-lang/rust/pull/70911#issuecomment-611102912. In short, it involves shortening paths to a name that is unique to them (e.g. if there's only one Vec ever), and potentially doing some prelude-specific logic (but not necessarily, I guess?).

I don't know if #73996 is based on that comment or not, but it's really cool that it's happening, and I'm sorry I didn't leave a comment here when I suggested the idea in the str librarification PR.
Eduard-Mihai Burtescu at 2020-07-13 08:42:00
Rustdoc would benefit from work on this; see https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/To.20alias.20or.20not.20to.20alias.
Noah Lev at 2022-03-02 19:01:43