8000 Make the tool_lints actually usable by flip1995 · Pull Request #52851 · rust-lang/rust · GitHub
[go: up one dir, main page]
Skip to content

Make the tool_lints actually usable #52851

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 1, 2018
Merged

Make the tool_lints actually usable #52851

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b466ee
Introduce the declare_tool_lint macro
flip1995 Jul 30, 2018
57c7742
Check if the lint_name is from a tool and if the tool_lint exists
flip1995 Jul 30, 2018
d4ff949
Add a test for the declare_tool_lint macro
flip1995 Jul 30, 2018
7b9388b
Explain that the tool is responsible for unknown tool_lints
flip1995 Jul 30, 2018
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
Prev Previous commit
Next Next commit
Check if the lint_name is from a tool and if the tool_lint exists
  • Loading branch information
@flip1995
flip1995 committed Jul 30, 2018
commit 57c77426ae6b4f9380a85a2aed80dc0900e7b93c
64 changes: 43 additions & 21 deletions src/librustc/lint/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String>),
/// 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 {
Expand Down Expand Up @@ -288,14 +294,15 @@ 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))
},
CheckLintNameResult::NoLint => {
Some(struct_err!(sess, E0602, "unknown lint: `{}`", lint_name))
}
CheckLintNameResult::Tool(_) => unreachable!(),
};

if let Some(mut db) = db {
Expand All @@ -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<LocalInternedString>,
) -> 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)),
}
}
Expand Down
36 changes: 28 additions & 8 deletions src/librustc/lint/levels.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,17 +227,18 @@ 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,
feature_gate::GateIssue::Language,
&format!("scoped lint `{}` is experimental",
word.ident));
}

if !attr::is_known_lint_tool(lint_tool) {
if !known_tool {
span_err!(
sess,
lint_tool.span,
Expand All @@ -247,17 +248,36 @@ 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 {
specs.insert(*id, (level, src));
}
}

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.
Copy link
Member Author
@flip1995 flip1995 Jul 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure what to do here... Can we somehow detect if we currently use the clippy_driver or the rustc_driver to find out if the lint does not exist or wasn't even added in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

The driver could register itself as a tool. Not sure where that information belongs, maybe a new field in the Session?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think tool attributes are required to be checked by the tool. So maybe instead we leave the "unknown lint" error to clippy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving it to the tool/Clippy was also my idea. We basically need a lint, which looks for allow(clippy::_)/... and checks if the lint exists, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

So... I guess just turn the FIXME comment into a comment explaining that

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok will do. And I will squash these commits while doing this.

}

_ if !self.warn_about_weird_lints => {}

CheckLintNameResult::Warning(msg, renamed) => {
Expand Down Expand Up @@ -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(
Expand Down
3055
0