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

Doc alias checks: ensure only items appearing in search index can use it #74098

Merged
merged 7 commits into from
Aug 19, 2020

Conversation

GuillaumeGomez
Copy link
Member

Following the discussion in #73721, I added checks to ensure that only items appearing in the search are allowed to have doc alias.

r? @ollie27

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2020
@ollie27
Copy link
Member

ollie27 commented Jul 7, 2020

The search index also doesn't include associated types and consts in trait impls so we'll need to error on those too. I'm not actually sure why they're not included though.

@GuillaumeGomez
Copy link
Member Author

I think it's in case of conflicts between two implementations. I'll add them as well, good catch!

@GuillaumeGomez GuillaumeGomez force-pushed the doc-alias-checks branch 2 times, most recently from 9e93555 to 1cc5b45 Compare July 23, 2020 19:07
@GuillaumeGomez
Copy link
Member Author

@ollie27 Updated!

@ollie27 ollie27 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 24, 2020
@GuillaumeGomez
Copy link
Member Author

cc @rust-lang/rustdoc

@ollie27
Copy link
Member

ollie27 commented Jul 29, 2020

Ideally this check would be done inside rustc with the other checks. Unfortunately I think #73566 meant that the checks moved in #74148 no longer run in rustdoc. That should have bee caught by CI but the test was moved rather than copied.

Assuming it's possible to get the check_mod_attrs pass running in rustdoc again then we should do that and move the checks from this PR there.

@GuillaumeGomez
Copy link
Member Author

Good point! I'll move those checks then.

@jyn514
Copy link
Member

jyn514 commented Jul 29, 2020

Assuming it's possible to get the check_mod_attrs pass running in rustdoc again then we should do that and move the checks from this PR there.

It should be possible to add it back as long as it doesn't need typeck. See https://github.com/rust-lang/rust/blob/master/src/librustdoc/core.rs#L460 for an example.

@jyn514
Copy link
Member

jyn514 commented Jul 30, 2020

So the discussion is in one place: @GuillaumeGomez tried this and found that checking for unused attributes is in the same pass as checking for unused code. So it currently requires typeck:

 Documenting std v0.0.0 (/home/imperio/rust/rust/src/libstd)
error[E0599]: no method named `into_handle` found for struct `sys::unix::thread::Thread` in the current scope
  --> src/libstd/sys/windows/ext/thread.rs:19:27
   |
19 |         self.into_inner().into_handle().into_raw() as *mut _
   |                           ^^^^^^^^^^^ method not found in `sys::unix::thread::Thread`
   | 
  ::: src/libstd/sys/unix/thread.rs:14:1
   |
14 | pub struct Thread {
   | ----------------- method `into_handle` not found for this

error: internal compiler error: src/librustc_passes/dead.rs:122:13: no type-dependent def for method

It will have to be split into two passes for this to work: one for unused attrs, and the other for unreachable expressions.

@ollie27
Copy link
Member

ollie27 commented Jul 30, 2020

check_mod_attrs is called here:

tcx.ensure().check_mod_attrs(local_def_id);

so I tested adding:

for &module in tcx.hir().krate().modules.keys() {
    let local_def_id = tcx.hir().local_def_id(module);
    tcx.ensure().check_mod_attrs(local_def_id);
};

just after

rust/src/librustdoc/core.rs

Lines 460 to 462 in 2186722

sess.time("missing_docs", || {
rustc_lint::check_crate(tcx, rustc_lint::builtin::MissingDoc::new);
});
and it seems to work just fine. I don't think that requires typeck but I could be wrong.

@GuillaumeGomez GuillaumeGomez force-pushed the doc-alias-checks branch 2 times, most recently from 17b2e1d to c0bb98d Compare August 1, 2020 15:38
@GuillaumeGomez
Copy link
Member Author

Updated! I added the equivalent tests both in test/rustdoc-ui and in test/ui to be sure that the pass is run in rustdoc. I also moved the checks in rustc directly.

@GuillaumeGomez
Copy link
Member Author

ping @rust-lang/rustdoc

@Manishearth
Copy link
Member

Do you want review from others? Seems like jyn and Ollie are reviewing this PR

@GuillaumeGomez
Copy link
Member Author

You can provide one as well if you want. But it was more like a reminder for the actual reviewers. :)

@Manishearth
Copy link
Member

Nah, I don't have many opinions on doc(alias) and I trust the others

@bors
Copy link
Contributor

bors commented Aug 8, 2020

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

@GuillaumeGomez
Copy link
Member Author

Rebased.

@GuillaumeGomez
Copy link
Member Author

ping @ollie27

@GuillaumeGomez
Copy link
Member Author

@ollie27 The impl const items are now allowed to have a #[doc(alias)]. :)

@GuillaumeGomez
Copy link
Member Author

Forgot that we tested in both test/ui and test/rustdoc-ui...

@GuillaumeGomez
Copy link
Member Author

ping @ollie27

Copy link
Member

@ollie27 ollie27 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

As I've said associated consts in trait impls also don't appear in the search index so should also produce errors.

@@ -5,7 +5,7 @@ const QUERY = [
'StructFieldItem',
'StructMethodItem',
'ImplTraitItem',
Copy link
Member

Choose a reason for hiding this comment

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

ImplTraitItem should be removed from here as well now it's been removed from doc-alias.rs.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why the search for it is returning nothing below. The goal was to check that nothing was returned.

Copy link
Member

Choose a reason for hiding this comment

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

ImplTraitItem doesn't appear anywhere in the source file anymore so of course it's not going to produce any search results. This isn't a particularly useful test.

Copy link
Member Author

Choose a reason for hiding this comment

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

But that's the point of the test: it's supposed to not return anything, so we ensure that it doesn't.

@GuillaumeGomez
Copy link
Member Author

I guess we're good to go then! \o/

@bors: r=ollie27

@bors
Copy link
Contributor

bors commented Aug 19, 2020

📌 Commit fc6fb3f has been approved by ollie27

@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 Aug 19, 2020
@bors
Copy link
Contributor

bors commented Aug 19, 2020

⌛ Testing commit fc6fb3f with merge e7f6ed1...

@bors
Copy link
Contributor

bors commented Aug 19, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2020
@bors bors merged commit e7f6ed1 into rust-lang:master Aug 19, 2020
@jyn514 jyn514 added the A-doc-alias Area: #[doc(alias)] label Sep 27, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doc-alias Area: #[doc(alias)] 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.

None yet

7 participants