-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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`" | ||
] | ||
); | ||
} |
There was a problem hiding this comment.
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?