Skip to content

Refactor and fix intra-doc link diagnostics, and fix links to primitives #76955

New issue

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

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

Already on GitHub? # to your account

Merged
merged 3 commits into from
Sep 27, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 20, 2020

Closes #76925, closes #76693, closes #76692.

Originally I only meant to fix #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 #76467 possible.

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

r? @euclio

@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 A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Sep 20, 2020
@jyn514 jyn514 force-pushed the refactor-diagnostics branch from b717d42 to 66a9cfa Compare September 20, 2020 07:10
Copy link
Contributor

@euclio euclio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with other comments addressed.

@jyn514 jyn514 force-pushed the refactor-diagnostics branch from 8c442fa to 2abf18c Compare September 23, 2020 23:50
@jyn514 jyn514 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 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
- Add `PrimTy::name` and `PrimTy::name_str`
- Use those new functions to distinguish between the name in scope and
the canonical name
- Fix diagnostics for primitive types
- Add tests for primitives
Now that `PrimTy::name()` exists, there's no need to carry around the
name of the primitive that failed to resolve. This removes the variants
special-casing primitives in favor of `NotResolved`.

- Remove `NoPrimitiveImpl` and `NoPrimitiveAssocItem`
- Remove hacky `has_primitive` check in `resolution_failure()`
- Fixup a couple tests that I forgot to `--bless` before
@jyn514 jyn514 force-pushed the refactor-diagnostics branch from 2abf18c to 049d29b Compare September 24, 2020 01:07
@jyn514
Copy link
Member Author

jyn514 commented Sep 24, 2020

@bors r=euclio

@bors
Copy link
Collaborator

bors commented Sep 24, 2020

📌 Commit 049d29b has been approved by euclio

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request 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
Copy link
Collaborator

bors commented Sep 27, 2020

⌛ Testing commit 049d29b with merge 71bdb84...

@bors
Copy link
Collaborator

bors commented Sep 27, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: euclio
Pushing 71bdb84 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 27, 2020
@bors bors merged commit 71bdb84 into rust-lang:master Sep 27, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 27, 2020
@jyn514 jyn514 deleted the refactor-diagnostics branch September 27, 2020 13:48
# for free to join this conversation on GitHub. Already have an account? # 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 merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
4 participants