From 65b28a987bfce11c37634fff2be129ffc16096d5 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 25 Jun 2021 14:28:03 -0700 Subject: [PATCH 1/5] Add ui test for command line lints with tool names This adds a ui test to make sure rustc accepts lint arguments such as `-A clippy::foo` when clippy is disabled. --- src/test/ui/lint/command-line-register-lint-tool.rs | 7 +++++++ .../lint/command-line-register-unknown-lint-tool.rs | 4 ++++ .../command-line-register-unknown-lint-tool.stderr | 11 +++++++++++ 3 files changed, 22 insertions(+) create mode 100644 src/test/ui/lint/command-line-register-lint-tool.rs create mode 100644 src/test/ui/lint/command-line-register-unknown-lint-tool.rs create mode 100644 src/test/ui/lint/command-line-register-unknown-lint-tool.stderr diff --git a/src/test/ui/lint/command-line-register-lint-tool.rs b/src/test/ui/lint/command-line-register-lint-tool.rs new file mode 100644 index 0000000000000..d6e95fd3ec403 --- /dev/null +++ b/src/test/ui/lint/command-line-register-lint-tool.rs @@ -0,0 +1,7 @@ +// compile-flags: -A known_tool::foo +// check-pass + +#![feature(register_tool)] +#![register_tool(known_tool)] + +fn main() {} diff --git a/src/test/ui/lint/command-line-register-unknown-lint-tool.rs b/src/test/ui/lint/command-line-register-unknown-lint-tool.rs new file mode 100644 index 0000000000000..435e951c809ba --- /dev/null +++ b/src/test/ui/lint/command-line-register-unknown-lint-tool.rs @@ -0,0 +1,4 @@ +// compile-flags: -A unknown_tool::foo +// check-fail + +fn main() {} diff --git a/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr b/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr new file mode 100644 index 0000000000000..9e0177c47da20 --- /dev/null +++ b/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr @@ -0,0 +1,11 @@ +error[E0602]: unknown lint: `unknown_tool::foo` + | + = note: requested on the command line with `-A unknown_tool::foo` + +error[E0602]: unknown lint: `unknown_tool::foo` + | + = note: requested on the command line with `-A unknown_tool::foo` + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0602`. From 1e0db4cfedcb9ecdc8df62aac22bec00a5c3c1b5 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 24 Jun 2021 16:38:32 -0700 Subject: [PATCH 2/5] Parse tool name for command line lint options --- compiler/rustc_lint/src/context.rs | 20 +++++++++++++++++--- compiler/rustc_lint/src/lib.rs | 3 +++ compiler/rustc_lint/src/tests.rs | 24 ++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 compiler/rustc_lint/src/tests.rs diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index 00869ac3c9a2b..76ac4cfdf84e5 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/compiler/rustc_lint/src/context.rs @@ -334,9 +334,16 @@ impl LintStore { } } - /// Checks the validity of lint names derived from the command line - pub fn check_lint_name_cmdline(&self, sess: &Session, lint_name: &str, level: Level) { - let db = match self.check_lint_name(lint_name, None) { + /// Checks the validity of lint names derived from the command line. Returns + /// true if the lint is valid, false otherwise. + pub fn check_lint_name_cmdline( + &self, + sess: &Session, + lint_name: &str, + level: Option, + ) -> bool { + let (tool_name, lint_name) = parse_lint_and_tool_name(lint_name); + let db = match self.check_lint_name(lint_name, tool_name) { CheckLintNameResult::Ok(_) => None, CheckLintNameResult::Warning(ref msg, _) => Some(sess.struct_warn(msg)), CheckLintNameResult::NoLint(suggestion) => { @@ -1018,3 +1025,10 @@ impl<'tcx> LayoutOf for LateContext<'tcx> { self.tcx.layout_of(self.param_env.and(ty)) } } + +pub fn parse_lint_and_tool_name(lint_name: &str) -> (Option, &str) { + match lint_name.split_once("::") { + Some((tool_name, lint_name)) => (Some(Symbol::intern(tool_name)), lint_name), + None => (None, lint_name), + } +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 89f9809d643e0..6f0cedca90c00 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -494,3 +494,6 @@ fn register_internals(store: &mut LintStore) { ], ); } + +#[cfg(test)] +mod tests; diff --git a/compiler/rustc_lint/src/tests.rs b/compiler/rustc_lint/src/tests.rs new file mode 100644 index 0000000000000..a50c88aa0f736 --- /dev/null +++ b/compiler/rustc_lint/src/tests.rs @@ -0,0 +1,24 @@ +use crate::context::parse_lint_and_tool_name; +use rustc_span::{with_default_session_globals, Symbol}; + +#[test] +fn parse_lint_no_tool() { + with_default_session_globals(|| assert_eq!(parse_lint_and_tool_name("foo"), (None, "foo"))); +} + +#[test] +fn parse_lint_with_tool() { + with_default_session_globals(|| { + assert_eq!(parse_lint_and_tool_name("clippy::foo"), (Some(Symbol::intern("clippy")), "foo")) + }); +} + +#[test] +fn parse_lint_multiple_path() { + with_default_session_globals(|| { + assert_eq!( + parse_lint_and_tool_name("clippy::foo::bar"), + (Some(Symbol::intern("clippy")), "foo::bar") + ) + }); +} From 8b4f538320cbb40643bfb94f28313d4137126b01 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 6 Jul 2021 17:19:20 -0700 Subject: [PATCH 3/5] Unify lint tool and lint name checking This shares a little more code between checking command line and attribute lint specifications. --- compiler/rustc_lint/src/context.rs | 44 ++++++++++--- compiler/rustc_lint/src/levels.rs | 61 ++++++++++--------- ...and-line-register-unknown-lint-tool.stderr | 8 +-- 3 files changed, 73 insertions(+), 40 deletions(-) diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index 76ac4cfdf84e5..263ed83ed5a21 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/compiler/rustc_lint/src/context.rs @@ -16,7 +16,7 @@ use self::TargetLint::*; -use crate::levels::LintLevelsBuilder; +use crate::levels::{is_known_lint_tool, LintLevelsBuilder}; use crate::passes::{EarlyLintPassObject, LateLintPassObject}; use rustc_ast as ast; use rustc_data_structures::fx::FxHashMap; @@ -129,6 +129,8 @@ pub enum CheckLintNameResult<'a> { Ok(&'a [LintId]), /// Lint doesn't exist. Potentially contains a suggestion for a correct lint name. NoLint(Option), + /// The lint refers to a tool that has not been registered. + NoTool, /// The lint is either renamed or removed. This is the warning /// message, and an optional new name (`None` if removed). Warning(String, Option), @@ -334,16 +336,17 @@ impl LintStore { } } - /// Checks the validity of lint names derived from the command line. Returns - /// true if the lint is valid, false otherwise. + /// Checks the validity of lint names derived from the command line. pub fn check_lint_name_cmdline( &self, sess: &Session, lint_name: &str, - level: Option, - ) -> bool { + level: Level, + crate_attrs: &[ast::Attribute], + ) { let (tool_name, lint_name) = parse_lint_and_tool_name(lint_name); - let db = match self.check_lint_name(lint_name, tool_name) { + + let db = match self.check_lint_and_tool_name(sess, tool_name, lint_name, crate_attrs) { CheckLintNameResult::Ok(_) => None, CheckLintNameResult::Warning(ref msg, _) => Some(sess.struct_warn(msg)), CheckLintNameResult::NoLint(suggestion) => { @@ -365,6 +368,13 @@ impl LintStore { ))), _ => None, }, + CheckLintNameResult::NoTool => Some(struct_span_err!( + sess, + DUMMY_SP, + E0602, + "unknown lint tool: `{}`", + tool_name.unwrap() + )), }; if let Some(mut db) = db { @@ -398,6 +408,22 @@ impl LintStore { } } + pub fn check_lint_and_tool_name( + &self, + sess: &Session, + tool_name: Option, + lint_name: &str, + crate_attrs: &[ast::Attribute], + ) -> CheckLintNameResult<'_> { + if let Some(tool_name) = tool_name { + if !is_known_lint_tool(tool_name, sess, crate_attrs) { + return CheckLintNameResult::NoTool; + } + } + + self.check_lint_name(lint_name, tool_name) + } + /// Checks the name of a lint for its existence, and whether it was /// renamed or removed. Generates a DiagnosticBuilder containing a /// warning for renamed and removed lints. This is over both lint @@ -1028,7 +1054,11 @@ impl<'tcx> LayoutOf for LateContext<'tcx> { pub fn parse_lint_and_tool_name(lint_name: &str) -> (Option, &str) { match lint_name.split_once("::") { - Some((tool_name, lint_name)) => (Some(Symbol::intern(tool_name)), lint_name), + Some((tool_name, lint_name)) => { + let tool_name = Symbol::intern(tool_name); + + (Some(tool_name), lint_name) + } None => (None, lint_name), } } diff --git a/compiler/rustc_lint/src/levels.rs b/compiler/rustc_lint/src/levels.rs index c0a059b92aa9f..30ee8c9b6ae1b 100644 --- a/compiler/rustc_lint/src/levels.rs +++ b/compiler/rustc_lint/src/levels.rs @@ -88,7 +88,7 @@ impl<'s> LintLevelsBuilder<'s> { self.sets.lint_cap = sess.opts.lint_cap.unwrap_or(Level::Forbid); for &(ref lint_name, level) in &sess.opts.lint_opts { - store.check_lint_name_cmdline(sess, &lint_name, level); + store.check_lint_name_cmdline(sess, &lint_name, level, self.crate_attrs); let orig_level = level; // If the cap is less than this specified level, e.g., if we've got @@ -110,7 +110,7 @@ impl<'s> LintLevelsBuilder<'s> { } for lint_name in &sess.opts.force_warns { - store.check_lint_name_cmdline(sess, lint_name, Level::ForceWarn); + store.check_lint_name_cmdline(sess, lint_name, Level::ForceWarn, self.crate_attrs); let lints = store .find_lints(lint_name) .unwrap_or_else(|_| bug!("A valid lint failed to produce a lint ids")); @@ -321,33 +321,15 @@ impl<'s> LintLevelsBuilder<'s> { continue; } }; - let tool_name = if meta_item.path.segments.len() > 1 { - let tool_ident = meta_item.path.segments[0].ident; - if !is_known_lint_tool(tool_ident.name, sess, &self.crate_attrs) { - let mut err = struct_span_err!( - sess, - tool_ident.span, - E0710, - "unknown tool name `{}` found in scoped lint: `{}`", - tool_ident.name, - pprust::path_to_string(&meta_item.path), - ); - if sess.is_nightly_build() { - err.help(&format!( - "add `#![register_tool({})]` to the crate root", - tool_ident.name - )); - } - err.emit(); - continue; - } - - Some(meta_item.path.segments.remove(0).ident.name) + let tool_ident = if meta_item.path.segments.len() > 1 { + Some(meta_item.path.segments.remove(0).ident) } else { None }; + let tool_name = tool_ident.map(|ident| ident.name); let name = pprust::path_to_string(&meta_item.path); - let lint_result = store.check_lint_name(&name, tool_name); + let lint_result = + store.check_lint_and_tool_name(sess, tool_name, &name, self.crate_attrs); match &lint_result { CheckLintNameResult::Ok(ids) => { let src = LintLevelSource::Node( @@ -364,7 +346,8 @@ impl<'s> LintLevelsBuilder<'s> { CheckLintNameResult::Tool(result) => { match *result { Ok(ids) => { - let complete_name = &format!("{}::{}", tool_name.unwrap(), name); + let complete_name = + &format!("{}::{}", tool_ident.unwrap().name, name); let src = LintLevelSource::Node( Symbol::intern(complete_name), sp, @@ -419,6 +402,26 @@ impl<'s> LintLevelsBuilder<'s> { } } + &CheckLintNameResult::NoTool => { + let mut err = struct_span_err!( + sess, + tool_ident.map_or(DUMMY_SP, |ident| ident.span), + E0710, + "unknown tool name `{}` found in scoped lint: `{}::{}`", + tool_name.unwrap(), + tool_name.unwrap(), + pprust::path_to_string(&meta_item.path), + ); + if sess.is_nightly_build() { + err.help(&format!( + "add `#![register_tool({})]` to the crate root", + tool_name.unwrap() + )); + } + err.emit(); + continue; + } + _ if !self.warn_about_weird_lints => {} CheckLintNameResult::Warning(msg, renamed) => { @@ -450,8 +453,8 @@ impl<'s> LintLevelsBuilder<'s> { let (level, src) = self.sets.get_lint_level(lint, self.cur, Some(&specs), self.sess); struct_lint_level(self.sess, lint, level, src, Some(sp.into()), |lint| { - let name = if let Some(tool_name) = tool_name { - format!("{}::{}", tool_name, name) + let name = if let Some(tool_ident) = tool_ident { + format!("{}::{}", tool_ident.name, name) } else { name.to_string() }; @@ -578,7 +581,7 @@ impl<'s> LintLevelsBuilder<'s> { } } -fn is_known_lint_tool(m_item: Symbol, sess: &Session, attrs: &[ast::Attribute]) -> bool { +pub fn is_known_lint_tool(m_item: Symbol, sess: &Session, attrs: &[ast::Attribute]) -> bool { if [sym::clippy, sym::rustc, sym::rustdoc].contains(&m_item) { return true; } diff --git a/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr b/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr index 9e0177c47da20..b7c5893a0ea86 100644 --- a/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr +++ b/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr @@ -1,10 +1,10 @@ -error[E0602]: unknown lint: `unknown_tool::foo` +error[E0602]: unknown lint tool: `unknown_tool` | - = note: requested on the command line with `-A unknown_tool::foo` + = note: requested on the command line with `-A foo` -error[E0602]: unknown lint: `unknown_tool::foo` +error[E0602]: unknown lint tool: `unknown_tool` | - = note: requested on the command line with `-A unknown_tool::foo` + = note: requested on the command line with `-A foo` error: aborting due to 2 previous errors From 5413d2e52981924bd42a5497ea4a0bf76f2a572f Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 7 Jul 2021 10:28:35 -0700 Subject: [PATCH 4/5] Fix test case Previously this was using a `.stderr` file, but that does not seem to work for all cases. This change uses `// error-pattern:` instead. --- .../lint/command-line-register-unknown-lint-tool.rs | 2 +- .../command-line-register-unknown-lint-tool.stderr | 11 ----------- 2 files changed, 1 insertion(+), 12 deletions(-) delete mode 100644 src/test/ui/lint/command-line-register-unknown-lint-tool.stderr diff --git a/src/test/ui/lint/command-line-register-unknown-lint-tool.rs b/src/test/ui/lint/command-line-register-unknown-lint-tool.rs index 435e951c809ba..59fc020009507 100644 --- a/src/test/ui/lint/command-line-register-unknown-lint-tool.rs +++ b/src/test/ui/lint/command-line-register-unknown-lint-tool.rs @@ -1,4 +1,4 @@ // compile-flags: -A unknown_tool::foo -// check-fail +// error-pattern: unknown lint tool: `unknown_tool` fn main() {} diff --git a/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr b/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr deleted file mode 100644 index b7c5893a0ea86..0000000000000 --- a/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error[E0602]: unknown lint tool: `unknown_tool` - | - = note: requested on the command line with `-A foo` - -error[E0602]: unknown lint tool: `unknown_tool` - | - = note: requested on the command line with `-A foo` - -error: aborting due to 2 previous errors - -For more information about this error, try `rustc --explain E0602`. From 4a83a93e9afd29e7494af3cc2a33c44ec32b0303 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Wed, 7 Jul 2021 10:50:50 -0700 Subject: [PATCH 5/5] Cleanup: unify lint name checking This change merges `check_lint_and_tool_name` into `check_lint_name` in order to avoid having two very similar functions. Also adds the `.stderr` file back for the test case, since apparently it is still needed. --- compiler/rustc_lint/src/context.rs | 28 +++++++------------ compiler/rustc_lint/src/levels.rs | 7 +++-- ...and-line-register-unknown-lint-tool.stderr | 11 ++++++++ 3 files changed, 25 insertions(+), 21 deletions(-) create mode 100644 src/test/ui/lint/command-line-register-unknown-lint-tool.stderr diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index 263ed83ed5a21..b6ea89b50743c 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/compiler/rustc_lint/src/context.rs @@ -344,9 +344,9 @@ impl LintStore { level: Level, crate_attrs: &[ast::Attribute], ) { - let (tool_name, lint_name) = parse_lint_and_tool_name(lint_name); + let (tool_name, lint_name_only) = parse_lint_and_tool_name(lint_name); - let db = match self.check_lint_and_tool_name(sess, tool_name, lint_name, crate_attrs) { + let db = match self.check_lint_name(sess, lint_name_only, tool_name, crate_attrs) { CheckLintNameResult::Ok(_) => None, CheckLintNameResult::Warning(ref msg, _) => Some(sess.struct_warn(msg)), CheckLintNameResult::NoLint(suggestion) => { @@ -408,22 +408,6 @@ impl LintStore { } } - pub fn check_lint_and_tool_name( - &self, - sess: &Session, - tool_name: Option, - lint_name: &str, - crate_attrs: &[ast::Attribute], - ) -> CheckLintNameResult<'_> { - if let Some(tool_name) = tool_name { - if !is_known_lint_tool(tool_name, sess, crate_attrs) { - return CheckLintNameResult::NoTool; - } - } - - self.check_lint_name(lint_name, tool_name) - } - /// Checks the name of a lint for its existence, and whether it was /// renamed or removed. Generates a DiagnosticBuilder containing a /// warning for renamed and removed lints. This is over both lint @@ -433,9 +417,17 @@ impl LintStore { /// printing duplicate warnings. pub fn check_lint_name( &self, + sess: &Session, lint_name: &str, tool_name: Option, + crate_attrs: &[ast::Attribute], ) -> CheckLintNameResult<'_> { + if let Some(tool_name) = tool_name { + if !is_known_lint_tool(tool_name, sess, crate_attrs) { + return CheckLintNameResult::NoTool; + } + } + let complete_name = if let Some(tool_name) = tool_name { format!("{}::{}", tool_name, lint_name) } else { diff --git a/compiler/rustc_lint/src/levels.rs b/compiler/rustc_lint/src/levels.rs index 30ee8c9b6ae1b..b603483414fd7 100644 --- a/compiler/rustc_lint/src/levels.rs +++ b/compiler/rustc_lint/src/levels.rs @@ -328,8 +328,7 @@ impl<'s> LintLevelsBuilder<'s> { }; let tool_name = tool_ident.map(|ident| ident.name); let name = pprust::path_to_string(&meta_item.path); - let lint_result = - store.check_lint_and_tool_name(sess, tool_name, &name, self.crate_attrs); + let lint_result = store.check_lint_name(sess, &name, tool_name, self.crate_attrs); match &lint_result { CheckLintNameResult::Ok(ids) => { let src = LintLevelSource::Node( @@ -477,7 +476,9 @@ impl<'s> LintLevelsBuilder<'s> { if let CheckLintNameResult::Warning(_, Some(new_name)) = lint_result { // Ignore any errors or warnings that happen because the new name is inaccurate // NOTE: `new_name` already includes the tool name, so we don't have to add it again. - if let CheckLintNameResult::Ok(ids) = store.check_lint_name(&new_name, None) { + if let CheckLintNameResult::Ok(ids) = + store.check_lint_name(sess, &new_name, None, self.crate_attrs) + { let src = LintLevelSource::Node(Symbol::intern(&new_name), sp, reason); for &id in ids { self.check_gated_lint(id, attr.span); diff --git a/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr b/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr new file mode 100644 index 0000000000000..c9a2aff2137a7 --- /dev/null +++ b/src/test/ui/lint/command-line-register-unknown-lint-tool.stderr @@ -0,0 +1,11 @@ +error[E0602]: unknown lint tool: `unknown_tool` + | + = note: requested on the command line with `-A unknown_tool::foo` + +error[E0602]: unknown lint tool: `unknown_tool` + | + = note: requested on the command line with `-A unknown_tool::foo` + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0602`.