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 diagnostics are wrong with a disambiguator that's not in the value namespace #76925

Closed
jyn514 opened this issue Sep 19, 2020 · 1 comment · Fixed by #76955
Closed
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints 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. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Sep 19, 2020

I tried this code:

/// [type@std::io::not::here]
pub fn f() {}

I expected to the same diagnostic as without a disambiguator:

warning: unresolved link to `std::io::not::here`
 --> tmp.rs:2:6
  |
2 | /// [std::io::not::here]
  |      ^^^^^^^^^^^^^^^^^^ the module `io` has no inner item named `not`

Instead, rustdoc gave a diagnostic that made no sense:

warning: unresolved link to `std::io::not::here`
 --> tmp.rs:1:6
  |
1 | /// [type@std::io::not::here]
  |      ^^^^^^^^^^^^^^^^^^^^^^^ the module `io` has no type named `std::io`

Meta

rustc --version --verbose:

rustdoc 1.48.0-nightly (bbc677480 2020-09-18)

Relevant code:

// `variant_field` looks at 3 different path segments in a row.
// But `NoAssocItem` assumes there are only 2. Check to see if there's
// an intermediate segment that resolves.
_ => {
let intermediate_path = format!("{}::{}", path, variant_name);
// NOTE: we have to be careful here, because we're already in `resolve`.
// We know this doesn't recurse forever because we use a shorter path each time.
// NOTE: this uses `TypeNS` because nothing else has a valid path segment after
let kind = if let Some(intermediate) = self.check_full_res(
TypeNS,
&intermediate_path,
module_id,
current_item,
extra_fragment,
) {
ResolutionFailure::NoAssocItem(intermediate, variant_field_name)
} else {
// Even with the shorter path, it didn't resolve, so say that.
ResolutionFailure::NoAssocItem(ty_res, variant_name)
};
Err(kind.into())
}

The issue is that variant_field is only called for the value namespace, but it's in charge of giving diagnostics for intermediate paths. That logic should instead be moved to fn resolution_failure.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. labels Sep 19, 2020
@jyn514 jyn514 self-assigned this Sep 19, 2020
@jyn514
Copy link
Member Author

jyn514 commented Sep 20, 2020

Another bug:

///  [f::A!]
pub fn f() {}
warning: unresolved link to `f::A`
 --> tmp.rs:3:7
  |
3 | ///  [f::A!]
  |       ^^^^^ no item named `f` in `tmp`

jyn514 added a commit to jyn514/rust that referenced this issue Sep 24, 2020
Previously, these were spread throughout the codebase. This had two
drawbacks:

1. It caused the fast path to be slower: even if a link resolved,
rustdoc would still perform various lookups for the error diagnostic.
2. It was inconsistent and didn't always give all diagnostics (rust-lang#76925)

Now, diagnostics only perform expensive lookups in the error case.
Additionally, the error handling is much more consistent, both in
wording and behavior.

- Remove `CannotHaveAssociatedItems`, `NotInScope`, `NoAssocItem`, and `NotAVariant`
  in favor of the more general `NotResolved`

  `resolution_failure` will now look up which of the four above
  categories is relevant, instead of requiring the rest of the code to
  be consistent and accurate in which it picked.

- Remove unnecessary lookups throughout the intra-doc link pass. These
are now done by `resolution_failure`.
  + Remove unnecessary `extra_fragment` argument to `variant_field()`;
    it was only used to do lookups on failure.
  + Remove various lookups related to associated items
  + Remove distinction between 'not in scope' and 'no associated item'

- Don't perform unnecessary copies
- Remove unused variables and code
- Update tests
- Note why looking at other namespaces is still necessary
- 'has no inner item' -> 'contains no item'

bless tests
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 25, 2020
Refactor and fix intra-doc link diagnostics, and fix links to primitives

Closes rust-lang#76925, closes rust-lang#76693, closes rust-lang#76692.

Originally I only meant to fix rust-lang#76925. But the hack with `has_primitive` was so bad it was easier to fix the primitive issues than to try and work around it.

Note that this still has one bug: `std::primitive::i32::MAX` does not resolve. However, this fixes the ICE so I'm fine with fixing the link in a later PR.

This is part of a series of refactors to make rust-lang#76467 possible.

This is best reviewed commit-by-commit; it has detailed commit messages.

r? @euclio
@bors bors closed this as completed in 71bdb84 Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints 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. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. 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.

1 participant