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

Cleanup rustdoc lint listing #80455

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ use rustc_middle::ty::TyCtxt;
use rustc_session::lint::builtin::{
BARE_TRAIT_OBJECTS, BROKEN_INTRA_DOC_LINKS, ELIDED_LIFETIMES_IN_PATHS,
EXPLICIT_OUTLIVES_REQUIREMENTS, INVALID_CODEBLOCK_ATTRIBUTES, INVALID_HTML_TAGS,
MISSING_DOC_CODE_EXAMPLES, NON_AUTOLINKS, PRIVATE_DOC_TESTS,
MISSING_CRATE_LEVEL_DOCS, MISSING_DOC_CODE_EXAMPLES, NON_AUTOLINKS, PRIVATE_DOC_TESTS,
};
use rustc_span::symbol::{Ident, Symbol};
use rustc_span::Span;
Expand Down Expand Up @@ -321,6 +321,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
INVALID_CODEBLOCK_ATTRIBUTES,
MISSING_DOC_CODE_EXAMPLES,
PRIVATE_DOC_TESTS,
MISSING_CRATE_LEVEL_DOCS,
Copy link
Member

Choose a reason for hiding this comment

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

This should not be mixed in with the cleanup; it's the same sort of change as #77364, which I'm still not convinced is the right change.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only emitted by rustdoc, so the context is a bit different there. This whole PR wouldn't be possible without this change because otherwise I'll need to still handle this lint differently than the others. However, because it was a bit out of the scope of this PR, I have put this change in a commit on its own (it's the first commit, you can check ;) ).

INVALID_HTML_TAGS
);

Expand Down
114 changes: 74 additions & 40 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use rustc_span::DUMMY_SP;

use std::cell::{Cell, RefCell};
use std::mem;
use std::path::PathBuf;
use std::rc::Rc;

use crate::clean;
Expand Down Expand Up @@ -273,6 +274,58 @@ where
(lint_opts, lint_caps)
}

crate fn get_rustdoc_lints(
cfg: FxHashSet<(String, Option<String>)>,
opts: config::Options,
) -> Vec<String> {
let config = interface::Config {
opts,
crate_cfg: cfg,
input: Input::File(PathBuf::new()),
input_path: None,
output_file: None,
output_dir: None,
file_loader: None,
diagnostic_output: DiagnosticOutput::Default,
stderr: None,
lint_caps: Default::default(),
register_lints: None,
override_queries: None,
make_codegen_backend: None,
registry: rustc_driver::diagnostics_registry(),
};
let missing_docs_lints = rustc_lint::builtin::MISSING_DOCS.name;
let renamed_and_removed_lints = rustc_lint::builtin::RENAMED_AND_REMOVED_LINTS.name;
let unknown_lints = rustc_lint::builtin::UNKNOWN_LINTS.name;

// Rustdoc silences all lints by default, only warning on lints in the `rustdoc` group.
// These lints aren't in the lint group, but we want to warn on them anyway.
let mut rustdoc_lints = vec![
missing_docs_lints.to_owned(),
renamed_and_removed_lints.to_owned(),
unknown_lints.to_owned(),
];

interface::run_compiler(config, |compiler| {
let sopts = &compiler.session().opts;
let lint_store = rustc_lint::new_lint_store(
sopts.debugging_opts.no_interleave_lints,
compiler.session().unstable_options(),
);
if let Some((_, lints, _)) =
lint_store.get_lint_groups().iter().find(|(name, _, _)| *name == "rustdoc")
{
for lint in lints {
// Funny thing about this: the lints in the compiler are capitalized but the lints
// you get through the `LintStore` are not, which means you have to capitalize them.
rustdoc_lints.push(lint.to_string().to_uppercase());
GuillaumeGomez marked this conversation as resolved.
Show resolved Hide resolved
}
}
});

rustdoc_lints
}

/// Parse, resolve, and typecheck the given crate.
crate fn create_config(
RustdocOptions {
Expand Down Expand Up @@ -301,51 +354,14 @@ crate fn create_config(
let cpath = Some(input.clone());
let input = Input::File(input);

let broken_intra_doc_links = lint::builtin::BROKEN_INTRA_DOC_LINKS.name;
let private_intra_doc_links = lint::builtin::PRIVATE_INTRA_DOC_LINKS.name;
let missing_docs = rustc_lint::builtin::MISSING_DOCS.name;
let missing_doc_example = rustc_lint::builtin::MISSING_DOC_CODE_EXAMPLES.name;
let private_doc_tests = rustc_lint::builtin::PRIVATE_DOC_TESTS.name;
let no_crate_level_docs = rustc_lint::builtin::MISSING_CRATE_LEVEL_DOCS.name;
let invalid_codeblock_attributes_name = rustc_lint::builtin::INVALID_CODEBLOCK_ATTRIBUTES.name;
let invalid_html_tags = rustc_lint::builtin::INVALID_HTML_TAGS.name;
let renamed_and_removed_lints = rustc_lint::builtin::RENAMED_AND_REMOVED_LINTS.name;
let non_autolinks = rustc_lint::builtin::NON_AUTOLINKS.name;
let unknown_lints = rustc_lint::builtin::UNKNOWN_LINTS.name;

// In addition to those specific lints, we also need to allow those given through
// command line, otherwise they'll get ignored and we don't want that.
let lints_to_show = vec![
broken_intra_doc_links.to_owned(),
private_intra_doc_links.to_owned(),
missing_docs.to_owned(),
missing_doc_example.to_owned(),
private_doc_tests.to_owned(),
no_crate_level_docs.to_owned(),
invalid_codeblock_attributes_name.to_owned(),
invalid_html_tags.to_owned(),
renamed_and_removed_lints.to_owned(),
unknown_lints.to_owned(),
non_autolinks.to_owned(),
];

let (lint_opts, lint_caps) = init_lints(lints_to_show, lint_opts, |lint| {
// FIXME: why is this necessary?
if lint.name == broken_intra_doc_links || lint.name == invalid_codeblock_attributes_name {
None
} else {
Some((lint.name_lower(), lint::Allow))
}
});

let crate_types =
if proc_macro_crate { vec![CrateType::ProcMacro] } else { vec![CrateType::Rlib] };
// plays with error output here!
let sessopts = config::Options {
let mut sessopts = config::Options {
maybe_sysroot,
search_paths: libs,
crate_types,
lint_opts: if !display_warnings { lint_opts } else { vec![] },
lint_opts: vec![],
Copy link
Member

Choose a reason for hiding this comment

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

This is a change in behavior (the same one from #73314). I think it's a good change, but it shouldn't be mixed in with a cleanup.

Copy link
Member Author

Choose a reason for hiding this comment

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

No it's not. Look just below: I set lint_opts after using the get_rustdoc_lints function. So it's the same but applied in two steps. I need this first one to do the least amount of things as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be exact, it's done here:

if !display_warnings {
    sessopts.lint_opts = lint_opts;
}

lint_cap,
cg: codegen_options,
externs,
Expand All @@ -360,9 +376,27 @@ crate fn create_config(
..Options::default()
};

let crate_cfg = interface::parse_cfgspecs(cfgs);

let invalid_codeblock_attributes_name = rustc_lint::builtin::INVALID_CODEBLOCK_ATTRIBUTES.name;
let broken_intra_doc_links = lint::builtin::BROKEN_INTRA_DOC_LINKS.name;

let lints_to_show = get_rustdoc_lints(crate_cfg.clone(), sessopts.clone());
let (lint_opts, lint_caps) = init_lints(lints_to_show, lint_opts, |lint| {
// FIXME: why is this necessary?
if lint.name == broken_intra_doc_links || lint.name == invalid_codeblock_attributes_name {
None
Comment on lines +386 to +388
Copy link
Member

Choose a reason for hiding this comment

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

If you remove this, does it still warn on those lints?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to update it in this PR to avoid mixing too many things together. We can take a look when this is merged if you want.

} else {
Some((lint.name_lower(), lint::Allow))
}
});
if !display_warnings {
sessopts.lint_opts = lint_opts;
}

interface::Config {
opts: sessopts,
crate_cfg: interface::parse_cfgspecs(cfgs),
crate_cfg,
input,
input_path: cpath,
output_file: None,
Expand Down
13 changes: 12 additions & 1 deletion src/test/rustdoc-ui/check.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,17 @@ warning: missing documentation for a function
LL | pub fn foo() {}
| ^^^^^^^^^^^^

warning: no documentation found for this crate's top-level module
|
note: the lint level is defined here
--> $DIR/check.rs:7:9
|
LL | #![warn(rustdoc)]
| ^^^^^^^
= note: `#[warn(missing_crate_level_docs)]` implied by `#[warn(rustdoc)]`
= help: The following guide may be of use:
https://doc.rust-lang.org/nightly/rustdoc/how-to-write-documentation.html

warning: missing code example in this documentation
--> $DIR/check.rs:4:1
|
Expand All @@ -45,5 +56,5 @@ warning: missing code example in this documentation
LL | pub fn foo() {}
| ^^^^^^^^^^^^^^^

warning: 4 warnings emitted
warning: 5 warnings emitted