Skip to content

Explain how to temporarily use paths instead of diagnostic items #15150

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
21 changes: 21 additions & 0 deletions clippy_utils/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,27 @@ path_macros! {
macro_path: PathNS::Macro,
}

// Paths in standard library (`alloc`/`core`/`std`, or against builtin scalar types)
// should be added as diagnostic items directly into the standard library, through a
// PR against the `rust-lang/rust` repository. If makes Clippy more robust in case
// the item is moved around, e.g. if the library structure is reorganized.
//
// If their use is required before the next sync (which happens every two weeks),
// they can be temporarily added below, prefixed with `DIAG_ITEM`, and commented
// with a reference to the PR adding the diagnostic item:
//
// ```rust
// // `sym::io_error_new` added in <https://github.com/rust-lang/rust/pull/142787>
// pub static DIAG_ITEM_IO_ERROR_NEW: PathLookup = value_path!(std::io::Error::new);
// ```
//
// During development, the "Added in …" comment is not required, but will make the
// test fail once the PR is submitted and becomes mandatory to ensure that a proper
// PR against `rust-lang/rust` has been created.
Copy link
Member

Choose a reason for hiding this comment

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

One thing that's unclear to me is what happens if a Clippy PR that defines one such path is ready to be merged but the nightly hasn't been bumped yet so we don't have the diagnostic item.

Would that be considered a blocker for the Clippy PR and the PR author will need to wait for that bump to happen, and then remove the path here?

Or can the clippy PR be merged with the temporary path in this file? If this is the case, who migrates it to a diagnostic item? It seems like we would essentially be back to the current situation of people having to go through the paths every once in a while asynchronously anyway independent of the PR and the test wouldn't actually enforce the use of diagnostic items, would it?

//
// You can request advice from the Clippy team members if you are not sure of how to
// add the diagnostic item to the standard library, or how to name it.

// Paths in external crates
pub static FUTURES_IO_ASYNCREADEXT: PathLookup = type_path!(futures_util::AsyncReadExt);
pub static FUTURES_IO_ASYNCWRITEEXT: PathLookup = type_path!(futures_util::AsyncWriteExt);
Expand Down
92 changes: 92 additions & 0 deletions tests/stdlib-diag-items.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// This tests checks that if a path is defined for an entity in the standard
// library, the proper prefix is used and a reference to a PR against
// `rust-lang/rust` is mentionned.
Comment on lines +1 to +3
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 personally not a fan of the idea of enforcing that a PR must be made to the r-l/r repository with a test like that. It's a rather huge hit in ergonomics and I imagine increases the barrier of entry for new contributors, rust-lang/rust is a huge project that takes a bit to set up, build the tools and maybe sometimes bless tests.

I do know that we generally prefer diagnostic items over hardcoded paths whenever possible, but when that isn't possible IMO the way it used to be done asynchronously (i.e., people add paths and every once in a while someone goes through them and adds diagnostic items in bulk) worked fine.

The argument that diagnostic items are more resilient against refactors in the stdlib is true but IMHO still isn't strong enough to warrant this. It's not like moving an item in the stdlib will silently break clippy, since clippy is built and tested in CI, so they'll just have to change the paths.rs file in clippy (which they might do anyway if they grep for the path across the codebase).

Then again, it's possible I'm alone on this (I know of #5393 and that it's taken a while to get to closing it). Might be something worth discussing in a meeting. I could be convinced that it's something we should do if more people think it's a good idea, or with more motivation


// This test is a no-op if run as part of the compiler test suite
// and will always succeed.

use itertools::Itertools;
use regex::Regex;
use std::io;

const PATHS_FILE: &str = "clippy_utils/src/paths.rs";

fn parse_content(content: &str) -> Vec<String> {
let comment_re = Regex::new(r"^// `sym::(.*)` added in <https://github.com/rust-lang/rust/pull/\d+>$").unwrap();
let path_re =
Regex::new(r"^pub static ([A-Z_]+): PathLookup = (?:macro|type|value)_path!\((([a-z]+)::.*)\);").unwrap();
let mut errors = vec![];
for (prev, line) in content.lines().tuple_windows() {
if let Some(caps) = path_re.captures(line) {
if ["alloc", "core", "std"].contains(&&caps[3]) && !caps[1].starts_with("DIAG_ITEM_") {
errors.push(format!(
"Path `{}` for `{}` should start with `DIAG_ITEM`",
&caps[1], &caps[2]
));
continue;
}
if let Some(upper) = caps[1].strip_prefix("DIAG_ITEM_") {
let Some(comment) = comment_re.captures(prev) else {
errors.push(format!(
"Definition for `{}` should be preceded by PR-related comment",
&caps[1]
));
continue;
};
let upper_sym = comment[1].to_uppercase();
if upper != upper_sym {
errors.push(format!(
"Path for symbol `{}` should be named `DIAG_ITEM_{}`",
&comment[1], upper_sym
));
}
}
}
}
errors
}

#[test]
fn stdlib_diag_items() -> Result<(), io::Error> {
if option_env!("RUSTC_TEST_SUITE").is_some() {
return Ok(());
}

let diagnostics = parse_content(&std::fs::read_to_string(PATHS_FILE)?);
if diagnostics.is_empty() {
Ok(())
} else {
eprintln!("Issues found in {PATHS_FILE}:");
for diag in diagnostics {
eprintln!("- {diag}");
}
Err(io::Error::other("problems found"))
}
}

#[test]
fn internal_diag_items_test() {
let content = r"
// Missing comment
pub static DIAG_ITEM_IO_ERROR_NEW: PathLookup = value_path!(std::io::Error::new);

// Wrong static name
// `sym::io_error` added in <https://github.com/rust-lang/rust/pull/142787>
pub static DIAG_ITEM_ERROR: PathLookup = value_path!(std::io::Error);

// Missing DIAG_ITEM
// `sym::io_foobar` added in <https://github.com/rust-lang/rust/pull/142787>
pub static IO_FOOBAR_PATH: PathLookup = value_path!(std::io);
";

let diags = parse_content(content);
let diags = diags.iter().map(String::as_str).collect::<Vec<_>>();
assert_eq!(
diags.as_slice(),
[
"Definition for `DIAG_ITEM_IO_ERROR_NEW` should be preceded by PR-related comment",
"Path for symbol `io_error` should be named `DIAG_ITEM_IO_ERROR`",
"Path `IO_FOOBAR_PATH` for `std::io` should start with `DIAG_ITEM`"
]
);
}