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

Clean up in intra-doc link collector #77743

Merged
merged 6 commits into from
Oct 11, 2020
Merged

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 9, 2020

This PR makes the following changes in intra-doc links:

  • clean up hard to follow closure-based logic in check_full_res
  • fix a FIXME comment by figuring out that true and false need to be resolved separately
  • refactor path resolution by extracting common code to a helper method and trying to reduce the number of unnecessary early returns
  • primitive types are now defined by their symbols, not their name strings
  • re-enables a commented-out test case

Closes #77267 (cc @Stupremee)

r? @jyn514

Copy link
Contributor Author

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

cc @jyn514

I've found some parts of the code are hard to follow so I've done some cleanups, but it's not a thorough one. I don't really want to change anything big to not cause conflicts with ongoing PRs.

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

Can you remove the first commit? I don't think this depends on #77700 in any way.

@jyn514 jyn514 added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 9, 2020
@jyn514 jyn514 self-assigned this Oct 9, 2020
src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
@bugadani
Copy link
Contributor Author

bugadani commented Oct 9, 2020

@jyn514 your test case can't compile, because true is a keyword.

/// [true] //~ ERROR ambiguous
pub mod true {}

Rustc recommends me r#-ing it, but then the test case passes. No you are right, it doesn't report. I need to double check what I paste.

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

When you say passes, you mean it warns that it's an ambiguous link?

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

For context, this is teh current error on master:

warning: `true` is both a module and a builtin type
 --> tmp.rs:1:6
  |
1 | /// [true] ~ERROR ambiguous
  |      ^^^^ ambiguous link

@bugadani bugadani force-pushed the idl-cleanup branch 2 times, most recently from 31291db to 5574fa0 Compare October 9, 2020 16:20
@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

Hmm, now try adding the disambiguator:

/// [primitive@true]
pub mod r#true {}

That goes through is_primitive, I think it will break after your change: https://github.com/rust-lang/rust/blob/5574fa0529401a8ff781e4017161f228b879c739/src/librustdoc/passes/collect_intra_doc_links.rs#L1011

@bugadani
Copy link
Contributor Author

bugadani commented Oct 9, 2020

Well I added code to test for true and false, it's just uses is_bool_value now, so the new test case should resolve just fine.

@jyn514
Copy link
Member

jyn514 commented Oct 9, 2020

I don't think we should separate is_primitive from is_true_and_false. I don't see any benefit to doing that, all it does is make you remember to call is_true_and_false() wherever you call is_primitive().

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this :)

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
Res::Def(DefKind::AssocTy, _) => false,
if let Ok(res) = result {
match res {
Res::Def(DefKind::AssocFn | DefKind::AssocConst, _) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm starting to wonder if traits_implemented_by is actually necessary ... do we already have the info here?
Related: #74489 (comment). Maybe rustc_resolve handles inherent impls but not anything else? @petrochenkov might know.

Anyway, not your problem to solve.

Comment on lines 1894 to 1998
fn is_bool_value(path_str: &str, ns: Namespace) -> Option<(&'static str, Res)> {
if ns == TypeNS && (path_str == "true" || path_str == "false") {
Some(("bool", Res::PrimTy(hir::PrimTy::Bool)))
} else {
None
}
Copy link
Member

Choose a reason for hiding this comment

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

This looks a little out of place now that it's in its own function ... I'm starting to wonder if ollie was right in #75101 (comment).

Anyway, not your problem, the implementation looks fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should support docs for keywords? :) /s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second (and third) thought, is it correct to handle true and false as an alias of bool? They are values, not types, up until the point when somebody defines something else called true. But in that case, couldn't the built-in resolution handle them?

Copy link
Member

Choose a reason for hiding this comment

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

That's a separate policy decision that shouldn't happen here. I don't have a strong opinion either way, but if you want to change it please open a separate issue so we can discuss it separate from the implementation.

Copy link
Member

Choose a reason for hiding this comment

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

couldn't the built-in resolution handle them?

They're not really values, they're keywords. Typeck handles them specially IIRC.

src/test/rustdoc-ui/intra-links-ambiguity.rs Outdated Show resolved Hide resolved
@bugadani bugadani force-pushed the idl-cleanup branch 4 times, most recently from e532eb5 to 5c916d0 Compare October 10, 2020 11:42
@bugadani
Copy link
Contributor Author

bugadani commented Oct 10, 2020

The idea behind the last 3 "Path resolution refactor", phase 1-3 commits is to make it clear that the resolution has two phases (not to be confused with the 3 part commit naming), and makes the phases as similar as possible.

I did not do this using purely equivalent transformations - instead of checking if we have a primitive, we first try to resolve the path instead, and only check primitiveness if we found a mod. I'm not sure if this can cause issues, but if it does, then the test suite does not cover it.

@bugadani bugadani force-pushed the idl-cleanup branch 2 times, most recently from 0b50831 to 4a22a57 Compare October 10, 2020 12:57
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Last few nits :) overall looks good

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
Comment on lines +368 to +415
let ty_res = if let Some(ty_res) = is_primitive(&path_root, TypeNS)
.map(|(_, res)| res)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
Copy link
Member

Choose a reason for hiding this comment

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

After my spiel about separating refactors and changes, I don't want to make the change here, but I don't think these are both necessary:

Suggested change
let ty_res = if let Some(ty_res) = is_primitive(&path_root, TypeNS)
.map(|(_, res)| res)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
// FIXME: are these both necessary?
let ty_res = if let Some(ty_res) = is_primitive(&path_root, TypeNS)
.map(|(_, res)| res)
.or_else(|| self.resolve_path(&path_root, TypeNS, module_id))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the strictest sense, they are, because removing is_primitive breaks tests, because modules aren't getting resolved. But even adding is_primitive back, it's only the modules that are called the same as the primitives that are... I wonder if this is because otherwise we'd end up returning early out of resolve()?

Copy link
Member

Choose a reason for hiding this comment

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

Let's fix this in a follow-up.

@jyn514
Copy link
Member

jyn514 commented Oct 10, 2020

nit: Can you squash the commits a little more? No need to bring it down to one, but it would be nice not to have ' Undo the behaviour change while keeping most of the stylistic ones ' in the git history.

Either way, r=me.

@bors delegate=bugadani

@bors
Copy link
Contributor

bors commented Oct 10, 2020

✌️ @bugadani can now approve this pull request

@bugadani
Copy link
Contributor Author

bugadani commented Oct 10, 2020

I agree, but that is the trickiest commit to squash in the whole bunch because the changes are mixed with the str->symbol ones :) I'll see what I can do

@jyn514
Copy link
Member

jyn514 commented Oct 10, 2020

Ok, no need then.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 10, 2020

📌 Commit d4c51b88f558ce4f38f0a4aae09a8fd8a1f055ce has been approved by jyn514

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 10, 2020
@bugadani
Copy link
Contributor Author

bugadani commented Oct 10, 2020

Sorry for the last force push but it doesn't hurt if the patch builds. Also squashed the nasty bits. 2 in 1, but should be fine now!

@jyn514
Copy link
Member

jyn514 commented Oct 10, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 10, 2020

📌 Commit 46f183bf523d7ee2ead49b0c6f3e51290a63ff8e has been approved by jyn514

@bors
Copy link
Contributor

bors commented Oct 10, 2020

☔ The latest upstream changes (presumably #76934) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 10, 2020
@bugadani
Copy link
Contributor Author

Rebased

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2020

📌 Commit 725e3d1 has been approved by bugadani

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 11, 2020
@bors
Copy link
Contributor

bors commented Oct 11, 2020

⌛ Testing commit 725e3d1 with merge c38f001...

@bors
Copy link
Contributor

bors commented Oct 11, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: bugadani
Pushing c38f001 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 11, 2020
@bors bors merged commit c38f001 into rust-lang:master Oct 11, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 11, 2020
@bugadani bugadani deleted the idl-cleanup branch October 11, 2020 12:03
@jyn514
Copy link
Member

jyn514 commented Oct 11, 2020

@bugadani small note - r+ marks you as the approver, please use r=jyn514 next time. Not a big deal though.

@bugadani
Copy link
Contributor Author

bugadani commented Oct 11, 2020

Whoops, sorry about that. Is there a bors users' manual somewhere? :)

Actually, I found it here: https://buildbot2.rust-lang.org/homu/

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-cleanup Category: PRs that clean code up or issues documenting cleanup. 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
Development

Successfully merging this pull request may close these issues.

Intra-doc links don't work for associated items on re-exported primitives
4 participants