Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

intra-doc: [type@char] point to module page instead of primitive page #74063

Closed
tesuji opened this issue Jul 5, 2020 · 14 comments · Fixed by #74078
Closed

intra-doc: [type@char] point to module page instead of primitive page #74063

tesuji opened this issue Jul 5, 2020 · 14 comments · Fixed by #74078
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@tesuji
Copy link
Contributor

tesuji commented Jul 5, 2020

I tried to document this code:

pub use std::char;

/// Foooooooo
///
/// [`char`] [`type@char`] [`std::char`]
pub struct Foo;

I expected to see this happen:

  • [`char`] and [`std::char`] points to module page of char
  • [`type@char`] points to char primitive page

Instead, this happened:

  • [`type@char`] points to module page of char

Meta

rustc --version --verbose:

rustc 1.46.0-nightly (0cd7ff7dd 2020-07-04)
binary: rustc
commit-hash: 0cd7ff7ddfb75a38dca81ad3e76b1e984129e939
commit-date: 2020-07-04
host: x86_64-unknown-linux-gnu
release: 1.46.0-nightly
LLVM version: 10.0
@tesuji tesuji added the C-bug Category: This is a bug. label Jul 5, 2020
@jonas-schievink jonas-schievink added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 5, 2020
@ehuss
Copy link
Contributor

ehuss commented Jul 5, 2020

I think the problem is that modules are in the type namespace, so type@char is doing exactly what you are asking it for (the char module, since it has higher priority). Since the built-in types scope is the lowest precedence, perhaps there needs to be a way to access it directly? Maybe something like prim@char or builtin@char?

Or maybe type@ should skip over modules, and keep looking deeper in the scope hierarchy? (cc #58699)

@jyn514
Copy link
Member

jyn514 commented Jul 5, 2020

Oh hmm I see what you mean. There are only three namespaces: Types, values, and macros. Since modules and types are in the same namespace, it uses the first one it finds.

Maybe we could work around this by first resolving in an empty module (so it has nothing in scope), then only falling back to the current scope if we don't find something there?

@jyn514
Copy link
Member

jyn514 commented Jul 5, 2020

I'm surprised this hasn't caused trouble before now. Does the root of std never use the char primitive?

@ehuss
Copy link
Contributor

ehuss commented Jul 5, 2020

I don't know how it works, but the resolver is smart enough to know that char in a type context won't match a module (like let x: char = 'c', the char won't match a module named "char". Maybe it has something to do with late-resolution smart_resolve_path which has a PathSource to give it some context?

@jyn514
Copy link
Member

jyn514 commented Jul 5, 2020

Adding that would be more of @Manishearth's area ... I can give it a shot though.

@jyn514
Copy link
Member

jyn514 commented Jul 5, 2020

It looks like smart_resolve_path doesn't even return a resolution and only records it for later use ... I'm going to go with the empty module workaround and we can fix it later if it turns out to cause issues.

@Manishearth
Copy link
Member

The double resolve seems hacky. We could probably make type@ specifically not resolve to a module, perhaps?

Also, note that modules are in all namespaces

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

The double resolve seems hacky. We could probably make type@ specifically not resolve to a module, perhaps?

How would that work? We could skip any module resolutions, but since modules have precedence over types I don't know how we'd get the primitive type to resolve afterwards.

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

Oh hold on we have a is_primitive function already. We can look at PRIMITIVES directly.

@Manishearth
Copy link
Member

This doesn't have anything to do with primitives, this has to do with glob imports vs non-glob imports. You can get the same issue with the following:

pub mod ch {}

pub mod inner {

    pub struct ch {
        x: bool
    }
}

use inner::*;

/// [`type@ch`] [`mod@ch`] [`ch`]
pub fn foo() {

}

@Manishearth
Copy link
Member

This also doesn't have to do with modules being "resolved first" or anything. Things that are non-glob in-scope are resolved first.

@Manishearth
Copy link
Member

Actually, my bad: This does have to do with primitives in the sense that primitives are the only thing that still work when shadowed because of some special code in smart_resolve_path checking the primitive tables.

I think when the string matches one of the primitive table entries for TypeNS lookups that were explicitly annotated with type@ and resolve to a module we can hack this in.

In the future we might want to tighten up this code so that e.g. struct@foo can't resolve to an enum, etc.

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

@Manishearth do you mind if I fix #58699 at the same time and resolve primitives even before modules? That seems like the less surprising behavior to me, the module can still be disambiguated with std::char.

@Manishearth
Copy link
Member

@jyn514 I'm a bit wary of doing this because having the char module in scope is not a common situation, and it will get weirder for people who have a different char module.

I think we should resolve to whatever is in scope, but if type@ is explicitly mentioned we can then force resolve to the primitive.

Manishearth added a commit to Manishearth/rust that referenced this issue Jul 6, 2020
Always resolve type@primitive as a primitive, not a module

Previously, if there were a module in scope with the same name as the
primitive, that would take precedence. Coupled with
rust-lang#58699, this made it impossible
to link to the primitive when that module was in scope.

This approach could be extended so that `struct@foo` would no longer resolve
to any type, etc. However, it could not be used for glob imports:

```rust
pub mod foo {
  pub struct Bar;
}

pub enum Bar {}
use foo::*;

// This is expected to link to `inner::Bar`, but instead it will link to the enum.
/// Link to [struct@Bar]
pub struct MyDocs;
```

The reason for this is that this change does not affect the resolution
algorithm of rustc_resolve at all. The only reason we could special-case
primitives is because we have a list of all possible primitives ahead of time.

Closes rust-lang#74063

r? @Manishearth
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 6, 2020
Always resolve type@primitive as a primitive, not a module

Previously, if there were a module in scope with the same name as the
primitive, that would take precedence. Coupled with
rust-lang#58699, this made it impossible
to link to the primitive when that module was in scope.

This approach could be extended so that `struct@foo` would no longer resolve
to any type, etc. However, it could not be used for glob imports:

```rust
pub mod foo {
  pub struct Bar;
}

pub enum Bar {}
use foo::*;

// This is expected to link to `inner::Bar`, but instead it will link to the enum.
/// Link to [struct@Bar]
pub struct MyDocs;
```

The reason for this is that this change does not affect the resolution
algorithm of rustc_resolve at all. The only reason we could special-case
primitives is because we have a list of all possible primitives ahead of time.

Closes rust-lang#74063

r? @Manishearth
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 6, 2020
Always resolve type@primitive as a primitive, not a module

Previously, if there were a module in scope with the same name as the
primitive, that would take precedence. Coupled with
rust-lang#58699, this made it impossible
to link to the primitive when that module was in scope.

This approach could be extended so that `struct@foo` would no longer resolve
to any type, etc. However, it could not be used for glob imports:

```rust
pub mod foo {
  pub struct Bar;
}

pub enum Bar {}
use foo::*;

// This is expected to link to `inner::Bar`, but instead it will link to the enum.
/// Link to [struct@Bar]
pub struct MyDocs;
```

The reason for this is that this change does not affect the resolution
algorithm of rustc_resolve at all. The only reason we could special-case
primitives is because we have a list of all possible primitives ahead of time.

Closes rust-lang#74063

r? @Manishearth
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jul 6, 2020
Always resolve type@primitive as a primitive, not a module

Previously, if there were a module in scope with the same name as the
primitive, that would take precedence. Coupled with
rust-lang#58699, this made it impossible
to link to the primitive when that module was in scope.

This approach could be extended so that `struct@foo` would no longer resolve
to any type, etc. However, it could not be used for glob imports:

```rust
pub mod foo {
  pub struct Bar;
}

pub enum Bar {}
use foo::*;

// This is expected to link to `inner::Bar`, but instead it will link to the enum.
/// Link to [struct@Bar]
pub struct MyDocs;
```

The reason for this is that this change does not affect the resolution
algorithm of rustc_resolve at all. The only reason we could special-case
primitives is because we have a list of all possible primitives ahead of time.

Closes rust-lang#74063

r? @Manishearth
@bors bors closed this as completed in ecc6f56 Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants