Repository navigation
Move lint validation to attribute parsing #164918
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: main
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 |
|---|---|---|
|
|
@@ -45,13 +45,12 @@ impl LintCheckKind { | |
| pub struct LintCheck { | ||
| /// The lint's tool name, if present. | ||
| pub tool: Option<Ident>, | ||
| /// The lint's name, minus the tool name if any. | ||
| /// The full normalized lint name of an existing lint, including the tool name. | ||
| /// This may not be what we user wrote down, in case of renamed lints the new lint name is here. | ||
|
Member
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. Until #164607 this can also recover unknown lint names to a clippy lint even when it is not registered as renamed. |
||
| /// The above implies that `LintStore::find_lints` is guaranteed to find this lint using this symbol. | ||
| /// | ||
| /// For example: | ||
| /// - `clippy::blah` -> `blah` | ||
| /// - `unstable_imports` -> `unstable_imports` | ||
| /// - `custom_tool::blah::bleh` -> `blah::bleh` | ||
| pub lint_name: Symbol, | ||
| /// For example, `single_use_lifetime` is changed to `single_use_lifetimes`. | ||
| pub lint_name_normalized: Symbol, | ||
|
Member
Author
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. My goal for a follow-up PR is to change this field from a |
||
| /// The span of the lint name. | ||
| /// | ||
| /// For example `#[deny(foo, bar, reason = "reason")]` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,12 +2,17 @@ use rustc_ast::join_path_syms; | |
| use rustc_attr_ir::AttributeKind; | ||
| use rustc_attr_ir::lint::{LintCheck, LintCheckKind}; | ||
| use rustc_attr_ir::target::{AssocCtxt, MethodKind, Target}; | ||
| use rustc_lint_defs::builtin::UNUSED_ATTRIBUTES; | ||
| use rustc_span::{Symbol, sym}; | ||
| use rustc_lint_defs::builtin::{RENAMED_AND_REMOVED_LINTS, UNKNOWN_LINTS, UNUSED_ATTRIBUTES}; | ||
| use rustc_lint_defs::{CheckLintNameResult, LintId}; | ||
| use rustc_span::{DUMMY_SP, Ident, Span, Symbol, sym}; | ||
| use thin_vec::ThinVec; | ||
|
|
||
| use crate::attributes::{AcceptMapping, AttributeParser, AttributeStability}; | ||
| use crate::context::{AcceptContext, ExpectStringLiteral, FinalizeContext}; | ||
| use crate::diagnostics::{ | ||
| DeprecatedLintName, RemovedLint, RenamedLint, RenamedLintSuggestion, UnknownLint, | ||
| UnknownLintSuggestion, UnknownToolInScopedLint, | ||
| }; | ||
| use crate::parser::{ArgParser, MetaItemOrLitParser}; | ||
| use crate::target_checking::AllowedTargets; | ||
| use crate::target_checking::Policy::{Allow, Warn}; | ||
|
|
@@ -69,9 +74,18 @@ impl LintParser { | |
| ), | ||
| }; | ||
|
|
||
| let lint_name_normalized = if let Some((_lint_ids, new_lint_name)) = | ||
|
Member
Author
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. Note that we already have the |
||
| Self::check_lint_name(lint_name, tool, p.span(), cx) | ||
|
JonathanBrouwer marked this conversation as resolved.
|
||
| { | ||
| new_lint_name | ||
| } else { | ||
| // Discard lint attributes with unknown lints in them | ||
| continue; | ||
|
Member
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. Please cover this in a test, I think it would be best to have a hir pretty test showing that unknown lints and lints whose tool is inactive are dropped. |
||
| }; | ||
|
|
||
| self.lints.push(LintCheck { | ||
| tool, | ||
| lint_name, | ||
| lint_name_normalized, | ||
| lint_span: p.span(), | ||
| kind, | ||
| attr_id, | ||
|
|
@@ -130,6 +144,142 @@ impl LintParser { | |
| } | ||
| } | ||
| } | ||
|
|
||
| fn check_lint_name<'a>( | ||
| lint_name: Symbol, | ||
| tool: Option<Ident>, | ||
| lint_span: Span, | ||
| cx: &mut AcceptContext<'_, 'a>, | ||
| ) -> Option<(&'a [LintId], Symbol)> { | ||
| let lint_result = cx.sess.lint_store.as_ref().unwrap().check_lint_name( | ||
| lint_name.as_str(), | ||
| tool.map(|tool| tool.name), | ||
| cx.lint_tools.unwrap(), | ||
| ); | ||
|
|
||
| // FIXME(164898) Lint attributes are parsed twice, once on the AST and once on the HIR | ||
| // Both times emit diagnostics, for historic reasons | ||
| // We only want to emit these lints once, on the HIR. | ||
| // On the AST `target_span` will be `DUMMY_SP`, which we use to detect which instance we are. | ||
| // #164898 will fix this, after that PR this check can be removed | ||
| let should_lint = cx.target_span != DUMMY_SP; | ||
|
Member
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 don't think this is a good idea:
|
||
|
|
||
| let (ids, name) = match lint_result { | ||
| CheckLintNameResult::Ok(ids) => (ids, lint_name), | ||
|
|
||
| CheckLintNameResult::Tool(ids, new_lint_name) => { | ||
| let name = match new_lint_name { | ||
| None => { | ||
| let complete_name = &format!("{}::{lint_name}", tool.unwrap().name); | ||
| Symbol::intern(complete_name) | ||
| } | ||
| Some(new_lint_name) => { | ||
| let symbol = Symbol::intern(&new_lint_name); | ||
| if should_lint { | ||
| cx.emit_lint( | ||
| RENAMED_AND_REMOVED_LINTS, | ||
| DeprecatedLintName { | ||
| name: lint_name.to_string(), | ||
| suggestion: lint_span, | ||
| replace: new_lint_name, | ||
| }, | ||
| lint_span, | ||
| ); | ||
| } | ||
| symbol | ||
| } | ||
| }; | ||
| (ids, name) | ||
| } | ||
|
|
||
| CheckLintNameResult::MissingTool => { | ||
| // If `MissingTool` 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. | ||
| return None; | ||
| } | ||
|
|
||
| CheckLintNameResult::NoTool => { | ||
| if should_lint { | ||
| cx.emit_err(UnknownToolInScopedLint { | ||
| span: Some(tool.unwrap().span), | ||
| tool_name: tool.unwrap().name, | ||
| lint_name: lint_name.to_string(), | ||
| is_nightly_build: cx.sess.is_nightly_build(), | ||
| }); | ||
| } | ||
| return None; | ||
| } | ||
|
|
||
| CheckLintNameResult::Renamed(ref replace) => { | ||
| if should_lint { | ||
| let suggestion = RenamedLintSuggestion::WithSpan { | ||
| suggestion: lint_span, | ||
| replace: replace.clone(), | ||
| }; | ||
| let name = tool | ||
| .map(|Ident { name, .. }| format!("{name}::{lint_name}")) | ||
| .unwrap_or_else(|| lint_name.to_string()); | ||
| cx.emit_lint( | ||
| RENAMED_AND_REMOVED_LINTS, | ||
| RenamedLint { name, replace: replace.clone(), suggestion }, | ||
| lint_span, | ||
| ); | ||
| } | ||
|
|
||
| // If this lint was renamed, apply the new lint instead of ignoring the | ||
| // attribute. 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. | ||
| let CheckLintNameResult::Ok(ids) = cx | ||
| .sess | ||
| .lint_store | ||
| .as_ref() | ||
| .unwrap() | ||
| .check_lint_name(replace, None, cx.lint_tools.unwrap()) | ||
| else { | ||
| panic!("renamed lint does not exist: {replace}"); | ||
| }; | ||
|
|
||
| (ids, Symbol::intern(&replace)) | ||
| } | ||
|
|
||
| CheckLintNameResult::Removed(ref reason) => { | ||
| if should_lint { | ||
| let name = if let Some(Ident { name, .. }) = tool { | ||
| format!("{name}::{lint_name}") | ||
| } else { | ||
| lint_name.to_string() | ||
| }; | ||
| cx.emit_lint( | ||
| RENAMED_AND_REMOVED_LINTS, | ||
| RemovedLint { name, reason: reason.clone() }, | ||
| lint_span, | ||
| ); | ||
| } | ||
| return None; | ||
| } | ||
|
|
||
| CheckLintNameResult::NoLint(suggestion) => { | ||
| if should_lint { | ||
| let name = tool | ||
| .map(|Ident { name, .. }| format!("{name}::{lint_name}")) | ||
| .unwrap_or_else(|| lint_name.to_string()); | ||
| let suggestion = | ||
| suggestion.map(|(replace, from_rustc)| UnknownLintSuggestion::WithSpan { | ||
| suggestion: lint_span, | ||
| replace, | ||
| from_rustc, | ||
| }); | ||
| cx.emit_lint(UNKNOWN_LINTS, UnknownLint { name, suggestion }, lint_span); | ||
| } | ||
| return None; | ||
| } | ||
| }; | ||
| Some((ids, name)) | ||
| } | ||
| } | ||
|
|
||
| impl AttributeParser for LintParser { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,7 +118,7 @@ pub use attributes::cfg::{ | |
| pub use attributes::cfg_select::*; | ||
| pub use attributes::util::{is_builtin_attr, parse_version}; | ||
| pub use context::ShouldEmit; | ||
| pub use diagnostics::ParsedDescription; | ||
| pub use diagnostics::{ParsedDescription, RenamedLintSuggestion, UnknownLintSuggestion}; | ||
|
Member
Author
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 not sure if people find it cursed to export diagnostic structs such that they can be re-used. I like it
Member
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 don't, it can hide unused diagnostics 😅
Member
Author
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. While unused diagnostics are bad, the alternative is duplicating the suggestion structs which risks them diverging when someone edits one but not the other. I think that's worse |
||
| pub use interface::{AttributeParser, EmitAttribute}; | ||
| pub use rustc_parse::parser::Recovery; | ||
| pub use template::AttributeTemplate; | ||
Uh oh!
There was an error while loading. Please reload this page.