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

Make rustdoc --passes and rustdoc --no-defaults have no effect #91714

Closed
jyn514 opened this issue Dec 9, 2021 · 11 comments · Fixed by #91900
Closed

Make rustdoc --passes and rustdoc --no-defaults have no effect #91714

jyn514 opened this issue Dec 9, 2021 · 11 comments · Fixed by #91900
Assignees
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Dec 9, 2021

Currently, rustdoc --passes does one of two things:

  1. --passes list shows a list of available passes:
Available passes for running rustdoc:
check_doc_test_visibility - run various visibility-related lints on doctests
        strip-hidden - strips all `#[doc(hidden)]` items from the output
   unindent-comments - removes excess indentation on comments in order for markdown to like it
       strip-private - strips all private items from a crate which cannot be seen externally, implies strip-priv-imports
  strip-priv-imports - strips all private import statements (`use`, `extern crate`) from a crate
   propagate-doc-cfg - propagates `#[doc(cfg(...))]` to child items
collect-intra-doc-links - resolves intra-doc links
check-code-block-syntax - validates syntax inside Rust code blocks
 collect-trait-impls - retrieves trait impls for items in the crate
calculate-doc-coverage - counts the number of items with and without documentation
check-invalid-html-tags - detects invalid HTML tags in doc comments
     check-bare-urls - detects URLs that are not hyperlinks

Default passes for rustdoc:
 collect-trait-impls
   unindent-comments
check_doc_test_visibility
        strip-hidden  (when not --document-hidden-items)
       strip-private  (when not --document-private-items)
  strip-priv-imports  (when --document-private-items)
collect-intra-doc-links
check-code-block-syntax
check-invalid-html-tags
   propagate-doc-cfg
     check-bare-urls

Passes run with `--show-coverage`:
        strip-hidden  (when not --document-hidden-items)
       strip-private  (when not --document-private-items)
calculate-doc-coverage
  1. rustdoc --passes collect-trait-impls will add that specific pass to the list of passes rustdoc executes (I think this is intended to be used with --no-defaults, which is also deprecated).

Both of these are stable, so unfortunately we can't remove them; 2. is deprecated and 1. should be soon (after #91713 is fixed). However, this is not a very useful sort of stability, because the names and number of passes is not stable, and rustdoc is very likely to be buggy if you don't run all passes (there's no sort of test for this).

I propose not just deprecating --passes and --no-defaults but making them a no-op altogether, which means less complexity both for users and in the rustdoc codebase. I also suggest removing the CLI help at the same time (and just say "this flag is a no-op").

Note that the only other deprecated flag, --input-format, is also currently a no-op when used with --input-format rust, and a hard error otherwise. We could go further and make everything but --passes=list be a hard error, but that seems like more of a breaking change than necessary for the goal of reducing complexity.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Dec 9, 2021
@jyn514
Copy link
Member Author

jyn514 commented Dec 9, 2021

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 9, 2021

Team member @jyn514 has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 9, 2021
@jyn514
Copy link
Member Author

jyn514 commented Dec 9, 2021

Oh, I forgot to mention earlier - there's precedent for doing this for other deprecated flags:

$ rustdoc --help
        --plugin-path DIR
                        removed
$ rustdoc --plugin-path x
warning: the 'plugin-path' flag no longer functions
  |
  = warning: see CVE-2018-1000622

@camelid
Copy link
Member

camelid commented Dec 9, 2021

@rfcbot reviewed

+1. I've been wanting to get rid of these flags for a while.

Should we use a future-incompatibility warning so that it'd be more feasible to potentially remove the flags in the future?

We could go further and make everything but --passes=list be a hard error, but that seems like more of a breaking change than necessary for the goal of reducing complexity.

Why everything except --passes=list?

@jyn514
Copy link
Member Author

jyn514 commented Dec 9, 2021

Should we use a future-incompatibility warning so that it'd be more feasible to potentially remove the flags in the future?

I don't have any objection to doing that, but I don't think it's a big deal either way - the main advantage of future-incompat warnings is they're shown for dependencies, but there's no way to pass flags to a dependency without also passing them to the main crate.

@camelid
Copy link
Member

camelid commented Dec 9, 2021

the main advantage of future-incompat warnings is they're shown for dependencies

I meant more that it would signal that you really shouldn't be using these flags and thus make it easier to potentially remove in the future.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Dec 9, 2021
@rfcbot
Copy link

rfcbot commented Dec 9, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 9, 2021
@jyn514
Copy link
Member Author

jyn514 commented Dec 14, 2021

Mentoring instructions: remove manual_passes (and recursively, all code that uses it):

// Options that affect the documentation process
/// The selected default set of passes to use.
///
/// Be aware: This option can come both from the CLI and from crate attributes!
crate default_passes: DefaultPassOption,
/// Any passes manually selected by the user.
///
/// Be aware: This option can come both from the CLI and from crate attributes!
crate manual_passes: Vec<String>,

Also remove DefaultPassOption::None:
passes::DefaultPassOption::None

@jyn514 jyn514 added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Dec 14, 2021
@pitaj
Copy link
Contributor

pitaj commented Dec 14, 2021

@rustbot claim

@camelid
Copy link
Member

camelid commented Dec 15, 2021

cc #82824

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 19, 2021
@rfcbot
Copy link

rfcbot commented Dec 19, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@bors bors closed this as completed in 940a97a Dec 20, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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