From 4b466ee4f94d7c17b0634495dc959e6f5dd4cc5a Mon Sep 17 00:00:00 2001 From: flip1995 <9744647+flip1995@users.noreply.github.com> Date: Mon, 30 Jul 2018 11:20:11 +0200 Subject: [PATCH 1/4] Introduce the declare_tool_lint macro --- src/librustc/lint/mod.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index 3c1b2056208..6940826ba86 100644 --- a/src/librustc/lint/mod.rs +++ b/src/librustc/lint/mod.rs @@ -139,6 +139,26 @@ macro_rules! declare_lint { ); } +#[macro_export] +macro_rules! declare_tool_lint { + ($vis: vis $tool: ident ::$NAME: ident, $Level: ident, $desc: expr) => ( + declare_tool_lint!{$vis $tool::$NAME, $Level, $desc, false} + ); + ($vis: vis $tool: ident ::$NAME: ident, $Level: ident, $desc: expr, + report_in_external_macro: $rep: expr) => ( + declare_tool_lint!{$vis $tool::$NAME, $Level, $desc, $rep} + ); + ($vis: vis $tool: ident ::$NAME: ident, $Level: ident, $desc: expr, $external: expr) => ( + $vis static $NAME: &$crate::lint::Lint = &$crate::lint::Lint { + name: &concat!(stringify!($tool), "::", stringify!($NAME)), + default_level: $crate::lint::$Level, + desc: $desc, + edition_lint_opts: None, + report_in_external_macro: $external, + }; + ); +} + /// Declare a static `LintArray` and return it as an expression. #[macro_export] macro_rules! lint_array { From 57c77426ae6b4f9380a85a2aed80dc0900e7b93c Mon Sep 17 00:00:00 2001 From: flip1995 <9744647+flip1995@users.noreply.github.com> Date: Mon, 30 Jul 2018 11:29:23 +0200 Subject: [PATCH 2/4] Check if the lint_name is from a tool and if the tool_lint exists --- src/librustc/lint/context.rs | 64 ++++++++++++++++++++++++------------ src/librustc/lint/levels.rs | 36 +++++++++++++++----- 2 files changed, 71 insertions(+), 29 deletions(-) diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index 14cfaa81533..15630157722 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -42,7 +42,7 @@ use util::nodemap::FxHashMap; use std::default::Default as StdDefault; use syntax::ast; use syntax::edition; -use syntax_pos::{MultiSpan, Span}; +use syntax_pos::{MultiSpan, Span, symbol::LocalInternedString}; use errors::DiagnosticBuilder; use hir; use hir::def_id::LOCAL_CRATE; @@ -133,6 +133,12 @@ pub enum CheckLintNameResult<'a> { /// The lint is either renamed or removed. This is the warning /// message, and an optional new name (`None` if removed). Warning(String, Option), + /// The lint is from a tool. If the Option is None, then either + /// the lint does not exist in the tool or the code was not + /// compiled with the tool and therefore the lint was never + /// added to the `LintStore`. Otherwise the `LintId` will be + /// returned as if it where a rustc lint. + Tool(Option<&'a [LintId]>), } impl LintStore { @@ -288,7 +294,7 @@ impl LintStore { sess: &Session, lint_name: &str, level: Level) { - let db = match self.check_lint_name(lint_name) { + let db = match self.check_lint_name(lint_name, None) { CheckLintNameResult::Ok(_) => None, CheckLintNameResult::Warning(ref msg, _) => { Some(sess.struct_warn(msg)) @@ -296,6 +302,7 @@ impl LintStore { CheckLintNameResult::NoLint => { Some(struct_err!(sess, E0602, "unknown lint: `{}`", lint_name)) } + CheckLintNameResult::Tool(_) => unreachable!(), }; if let Some(mut db) = db { @@ -319,26 +326,41 @@ impl LintStore { /// it emits non-fatal warnings and there are *two* lint passes that /// inspect attributes, this is only run from the late pass to avoid /// printing duplicate warnings. - pub fn check_lint_name(&self, lint_name: &str) -> CheckLintNameResult { - match self.by_name.get(lint_name) { - Some(&Renamed(ref new_name, _)) => { - CheckLintNameResult::Warning( - format!("lint `{}` has been renamed to `{}`", lint_name, new_name), - Some(new_name.to_owned()) - ) - }, - Some(&Removed(ref reason)) => { - CheckLintNameResult::Warning( - format!("lint `{}` has been removed: `{}`", lint_name, reason), - None - ) - }, - None => { - match self.lint_groups.get(lint_name) { - None => CheckLintNameResult::NoLint, - Some(ids) => CheckLintNameResult::Ok(&ids.0), - } + pub fn check_lint_name( + &self, + lint_name: &str, + tool_name: Option, + ) -> CheckLintNameResult { + let complete_name = if let Some(tool_name) = tool_name { + format!("{}::{}", tool_name, lint_name) + } else { + lint_name.to_string() + }; + if let Some(_) = tool_name { + match self.by_name.get(&complete_name) { + None => match self.lint_groups.get(&*complete_name) { + None => return CheckLintNameResult::Tool(None), + Some(ids) => return CheckLintNameResult::Tool(Some(&ids.0)), + }, + Some(&Id(ref id)) => return CheckLintNameResult::Tool(Some(slice::from_ref(id))), + // If the lint was registered as removed or renamed by the lint tool, we don't need + // to treat tool_lints and rustc lints different and can use the code below. + _ => {} } + } + match self.by_name.get(&complete_name) { + Some(&Renamed(ref new_name, _)) => CheckLintNameResult::Warning( + format!("lint `{}` has been renamed to `{}`", lint_name, new_name), + Some(new_name.to_owned()), + ), + Some(&Removed(ref reason)) => CheckLintNameResult::Warning( + format!("lint `{}` has been removed: `{}`", lint_name, reason), + None, + ), + None => match self.lint_groups.get(&*complete_name) { + None => CheckLintNameResult::NoLint, + Some(ids) => CheckLintNameResult::Ok(&ids.0), + }, Some(&Id(ref id)) => CheckLintNameResult::Ok(slice::from_ref(id)), } } diff --git a/src/librustc/lint/levels.rs b/src/librustc/lint/levels.rs index b7b6aba970b..6a29b8c3e3e 100644 --- a/src/librustc/lint/levels.rs +++ b/src/librustc/lint/levels.rs @@ -227,8 +227,10 @@ impl<'a> LintLevelsBuilder<'a> { continue } }; - if let Some(lint_tool) = word.is_scoped() { - if !self.sess.features_untracked().tool_lints { + let tool_name = if let Some(lint_tool) = word.is_scoped() { + let gate_feature = !self.sess.features_untracked().tool_lints; + let known_tool = attr::is_known_lint_tool(lint_tool); + if gate_feature { feature_gate::emit_feature_err(&sess.parse_sess, "tool_lints", word.span, @@ -236,8 +238,7 @@ impl<'a> LintLevelsBuilder<'a> { &format!("scoped lint `{}` is experimental", word.ident)); } - - if !attr::is_known_lint_tool(lint_tool) { + if !known_tool { span_err!( sess, lint_tool.span, @@ -247,10 +248,16 @@ impl<'a> LintLevelsBuilder<'a> { ); } - continue - } + if gate_feature || !known_tool { + continue + } + + Some(lint_tool.as_str()) + } else { + None + }; let name = word.name(); - match store.check_lint_name(&name.as_str()) { + match store.check_lint_name(&name.as_str(), tool_name) { CheckLintNameResult::Ok(ids) => { let src = LintSource::Node(name, li.span); for id in ids { @@ -258,6 +265,19 @@ impl<'a> LintLevelsBuilder<'a> { } } + CheckLintNameResult::Tool(result) => { + if let Some(ids) = result { + let complete_name = &format!("{}::{}", tool_name.unwrap(), name); + let src = LintSource::Node(Symbol::intern(complete_name), li.span); + for id in ids { + specs.insert(*id, (level, src)); + } + } + //FIXME: if Tool(None) is returned than the lint either does not exist in + //the lint tool or the code doesn't get compiled with the lint tool and + //therefore the lint cannot exist. + } + _ if !self.warn_about_weird_lints => {} CheckLintNameResult::Warning(msg, renamed) => { @@ -298,7 +318,7 @@ impl<'a> LintLevelsBuilder<'a> { if name.as_str().chars().any(|c| c.is_uppercase()) { let name_lower = name.as_str().to_lowercase().to_string(); if let CheckLintNameResult::NoLint = - store.check_lint_name(&name_lower) { + store.check_lint_name(&name_lower, tool_name) { db.emit(); } else { db.span_suggestion_with_applicability( From d4ff949953141877743ee95123a492c4e48c3809 Mon Sep 17 00:00:00 2001 From: flip1995 <9744647+flip1995@users.noreply.github.com> Date: Mon, 30 Jul 2018 11:30:07 +0200 Subject: [PATCH 3/4] Add a test for the declare_tool_lint macro --- .../ui-fulldeps/auxiliary/lint_tool_test.rs | 48 +++++++++++++++++++ src/test/ui-fulldeps/lint_tool_test.rs | 24 ++++++++++ src/test/ui-fulldeps/lint_tool_test.stderr | 8 ++++ 3 files changed, 80 insertions(+) create mode 100644 src/test/ui-fulldeps/auxiliary/lint_tool_test.rs create mode 100644 src/test/ui-fulldeps/lint_tool_test.rs create mode 100644 src/test/ui-fulldeps/lint_tool_test.stderr diff --git a/src/test/ui-fulldeps/auxiliary/lint_tool_test.rs b/src/test/ui-fulldeps/auxiliary/lint_tool_test.rs new file mode 100644 index 00000000000..01fa2f3459e --- /dev/null +++ b/src/test/ui-fulldeps/auxiliary/lint_tool_test.rs @@ -0,0 +1,48 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(plugin_registrar)] +#![feature(box_syntax, rustc_private)] +#![feature(macro_vis_matcher)] +#![feature(macro_at_most_once_rep)] + +extern crate syntax; + +// Load rustc as a plugin to get macros +#[macro_use] +extern crate rustc; +extern crate rustc_plugin; + +use rustc::lint::{EarlyContext, LintContext, LintPass, EarlyLintPass, + LintArray}; +use rustc_plugin::Registry; +use syntax::ast; +declare_tool_lint!(pub clippy::TEST_LINT, Warn, "Warn about stuff"); + +struct Pass; + +impl LintPass for Pass { + fn get_lints(&self) -> LintArray { + lint_array!(TEST_LINT) + } +} + +impl EarlyLintPass for Pass { + fn check_item(&mut self, cx: &EarlyContext, it: &ast::Item) { + if it.ident.name == "lintme" { + cx.span_lint(TEST_LINT, it.span, "item is named 'lintme'"); + } + } +} + +#[plugin_registrar] +pub fn plugin_registrar(reg: &mut Registry) { + reg.register_early_lint_pass(box Pass); +} diff --git a/src/test/ui-fulldeps/lint_tool_test.rs b/src/test/ui-fulldeps/lint_tool_test.rs new file mode 100644 index 00000000000..ccdcd2df31b --- /dev/null +++ b/src/test/ui-fulldeps/lint_tool_test.rs @@ -0,0 +1,24 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// run-pass +// aux-build:lint_tool_test.rs +// ignore-stage1 +#![feature(plugin)] +#![feature(tool_lints)] +#![plugin(lint_tool_test)] +#![allow(dead_code)] + +fn lintme() { } //~ WARNING item is named 'lintme' + +#[allow(clippy::test_lint)] +pub fn main() { + fn lintme() { } +} diff --git a/src/test/ui-fulldeps/lint_tool_test.stderr b/src/test/ui-fulldeps/lint_tool_test.stderr new file mode 100644 index 00000000000..22d0f458e7d --- /dev/null +++ b/src/test/ui-fulldeps/lint_tool_test.stderr @@ -0,0 +1,8 @@ +warning: item is named 'lintme' + --> $DIR/lint_tool_test.rs:19:1 + | +LL | fn lintme() { } //~ WARNING item is named 'lintme' + | ^^^^^^^^^^^^^^^ + | + = note: #[warn(clippy::test_lint)] on by default + From 7b9388b7b5fcdbb2f7e7178dc0a533e3284184c5 Mon Sep 17 00:00:00 2001 From: flip1995 <9744647+flip1995@users.noreply.github.com> Date: Mon, 30 Jul 2018 16:07:00 +0200 Subject: [PATCH 4/4] Explain that the tool is responsible for unknown tool_lints --- src/librustc/lint/levels.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc/lint/levels.rs b/src/librustc/lint/levels.rs index 6a29b8c3e3e..483e2ea8a96 100644 --- a/src/librustc/lint/levels.rs +++ b/src/librustc/lint/levels.rs @@ -273,9 +273,10 @@ impl<'a> LintLevelsBuilder<'a> { specs.insert(*id, (level, src)); } } - //FIXME: if Tool(None) is returned than the lint either does not exist in - //the lint tool or the code doesn't get compiled with the lint tool and - //therefore the lint cannot exist. + // If Tool(None) is returned, then either the lint does not exist in the + // tool or the code was not compiled with the tool and therefore the lint + // was never added to the `LintStore`. To detect this is the responsibility + // of the lint tool. } _ if !self.warn_about_weird_lints => {}