From 82124f33b5b7820eceef0192bcc53f45cbea093f Mon Sep 17 00:00:00 2001 From: Chayim Refael Friedman Date: Thu, 12 Sep 2024 18:40:41 +0300 Subject: [PATCH] Handle lint attributes that are under `#[cfg_attr]` --- crates/hir-expand/src/cfg_process.rs | 21 +++-- crates/hir-expand/src/lib.rs | 1 + crates/hir/src/semantics.rs | 13 +++ .../src/handlers/incorrect_case.rs | 22 ++++++ crates/ide-diagnostics/src/lib.rs | 79 ++++++++++++++++--- 5 files changed, 120 insertions(+), 16 deletions(-) diff --git a/crates/hir-expand/src/cfg_process.rs b/crates/hir-expand/src/cfg_process.rs index 147cf912da..01a3103af8 100644 --- a/crates/hir-expand/src/cfg_process.rs +++ b/crates/hir-expand/src/cfg_process.rs @@ -6,7 +6,7 @@ use cfg::{CfgAtom, CfgExpr}; use intern::{sym, Symbol}; use rustc_hash::FxHashSet; use syntax::{ - ast::{self, Attr, HasAttrs, Meta, VariantList}, + ast::{self, Attr, HasAttrs, Meta, TokenTree, VariantList}, AstNode, NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode, T, }; use tracing::{debug, warn}; @@ -17,7 +17,7 @@ fn check_cfg(db: &dyn ExpandDatabase, attr: &Attr, krate: CrateId) -> Option Optio if !attr.simple_name().as_deref().map(|v| v == "cfg_attr")? { return None; } - let cfg_expr = parse_from_attr_meta(attr.meta()?)?; + check_cfg_attr_value(db, &attr.token_tree()?, krate) +} + +pub fn check_cfg_attr_value( + db: &dyn ExpandDatabase, + attr: &TokenTree, + krate: CrateId, +) -> Option { + let cfg_expr = parse_from_attr_token_tree(attr)?; let enabled = db.crate_graph()[krate].cfg_options.check(&cfg_expr) != Some(false); Some(enabled) } @@ -238,8 +246,7 @@ pub(crate) fn process_cfg_attrs( Some(remove) } /// Parses a `cfg` attribute from the meta -fn parse_from_attr_meta(meta: Meta) -> Option { - let tt = meta.token_tree()?; +fn parse_from_attr_token_tree(tt: &TokenTree) -> Option { let mut iter = tt .token_trees_and_tokens() .filter(is_not_whitespace) @@ -328,7 +335,7 @@ mod tests { use expect_test::{expect, Expect}; use syntax::{ast::Attr, AstNode, SourceFile}; - use crate::cfg_process::parse_from_attr_meta; + use crate::cfg_process::parse_from_attr_token_tree; fn check_dnf_from_syntax(input: &str, expect: Expect) { let parse = SourceFile::parse(input, span::Edition::CURRENT); @@ -342,7 +349,7 @@ mod tests { let node = node.clone_subtree(); assert_eq!(node.syntax().text_range().start(), 0.into()); - let cfg = parse_from_attr_meta(node.meta().unwrap()).unwrap(); + let cfg = parse_from_attr_token_tree(&node.meta().unwrap().token_tree().unwrap()).unwrap(); let actual = format!("#![cfg({})]", DnfExpr::new(&cfg)); expect.assert_eq(&actual); } diff --git a/crates/hir-expand/src/lib.rs b/crates/hir-expand/src/lib.rs index 7cf4fb5cef..9538097949 100644 --- a/crates/hir-expand/src/lib.rs +++ b/crates/hir-expand/src/lib.rs @@ -53,6 +53,7 @@ use crate::{ }; pub use crate::{ + cfg_process::check_cfg_attr_value, files::{AstId, ErasedAstId, FileRange, InFile, InMacroFile, InRealFile}, prettify_macro_expansion_::prettify_macro_expansion, }; diff --git a/crates/hir/src/semantics.rs b/crates/hir/src/semantics.rs index dcaff04442..fa14b53dbc 100644 --- a/crates/hir/src/semantics.rs +++ b/crates/hir/src/semantics.rs @@ -386,6 +386,19 @@ impl<'db> SemanticsImpl<'db> { Some(node) } + pub fn check_cfg_attr(&self, attr: &ast::TokenTree) -> Option { + let file_id = self.find_file(attr.syntax()).file_id; + let krate = match file_id.repr() { + HirFileIdRepr::FileId(file_id) => { + self.file_to_module_defs(file_id.file_id()).next()?.krate().id + } + HirFileIdRepr::MacroFile(macro_file) => { + self.db.lookup_intern_macro_call(macro_file.macro_call_id).krate + } + }; + hir_expand::check_cfg_attr_value(self.db.upcast(), attr, krate) + } + /// Expands the macro if it isn't one of the built-in ones that expand to custom syntax or dummy /// expansions. pub fn expand_allowed_builtins(&self, macro_call: &ast::MacroCall) -> Option { diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs index c4c2895317..83a1eb44a6 100644 --- a/crates/ide-diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide-diagnostics/src/handlers/incorrect_case.rs @@ -997,6 +997,28 @@ fn BAR() { ); } + #[test] + fn cfged_lint_attrs() { + check_diagnostics( + r#" +//- /lib.rs cfg:feature=cool_feature +#[cfg_attr(any(), allow(non_snake_case))] +fn FOO() {} +// ^^^ 💡 warn: Function `FOO` should have snake_case name, e.g. `foo` + +#[cfg_attr(non_existent, allow(non_snake_case))] +fn BAR() {} +// ^^^ 💡 warn: Function `BAR` should have snake_case name, e.g. `bar` + +#[cfg_attr(feature = "cool_feature", allow(non_snake_case))] +fn BAZ() {} + +#[cfg_attr(feature = "cool_feature", cfg_attr ( all ( ) , allow ( non_snake_case ) ) ) ] +fn QUX() {} + "#, + ); + } + #[test] fn allow_with_comment() { check_diagnostics( diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index d46d099f6f..45c723d09d 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -76,8 +76,9 @@ mod handlers { #[cfg(test)] mod tests; -use std::{collections::hash_map, sync::LazyLock}; +use std::{collections::hash_map, iter, sync::LazyLock}; +use either::Either; use hir::{db::ExpandDatabase, diagnostics::AnyDiagnostic, Crate, HirFileId, InFile, Semantics}; use ide_db::{ assists::{Assist, AssistId, AssistKind, AssistResolveStrategy}, @@ -92,7 +93,7 @@ use ide_db::{ use itertools::Itertools; use syntax::{ ast::{self, AstNode, HasAttrs}, - AstPtr, Edition, SmolStr, SyntaxNode, SyntaxNodePtr, TextRange, + AstPtr, Edition, NodeOrToken, SmolStr, SyntaxKind, SyntaxNode, SyntaxNodePtr, TextRange, T, }; // FIXME: Make this an enum @@ -647,6 +648,7 @@ fn find_outline_mod_lint_severity( let mut result = None; let lint_groups = lint_groups(&diag.code); lint_attrs( + sema, ast::AnyHasAttrs::cast(module_source_file.value).expect("SourceFile always has attrs"), edition, ) @@ -763,7 +765,7 @@ fn fill_lint_attrs( if let Some(ancestor) = ast::AnyHasAttrs::cast(ancestor) { // Insert this node's attributes into any outline descendant, including this node. - lint_attrs(ancestor, edition).for_each(|(lint, severity)| { + lint_attrs(sema, ancestor, edition).for_each(|(lint, severity)| { if diag_severity.is_none() && lint_groups(&diag.code).contains(&&*lint) { diag_severity = Some(severity); } @@ -793,7 +795,7 @@ fn fill_lint_attrs( cache_stack.pop(); return diag_severity; } else if let Some(ancestor) = ast::AnyHasAttrs::cast(ancestor) { - lint_attrs(ancestor, edition).for_each(|(lint, severity)| { + lint_attrs(sema, ancestor, edition).for_each(|(lint, severity)| { if diag_severity.is_none() && lint_groups(&diag.code).contains(&&*lint) { diag_severity = Some(severity); } @@ -809,20 +811,27 @@ fn fill_lint_attrs( } } -fn lint_attrs( +fn lint_attrs<'a>( + sema: &'a Semantics<'a, RootDatabase>, ancestor: ast::AnyHasAttrs, edition: Edition, -) -> impl Iterator { +) -> impl Iterator + 'a { ancestor .attrs_including_inner() .filter_map(|attr| { attr.as_simple_call().and_then(|(name, value)| match &*name { - "allow" | "expect" => Some((Severity::Allow, value)), - "warn" => Some((Severity::Warning, value)), - "forbid" | "deny" => Some((Severity::Error, value)), + "allow" | "expect" => Some(Either::Left(iter::once((Severity::Allow, value)))), + "warn" => Some(Either::Left(iter::once((Severity::Warning, value)))), + "forbid" | "deny" => Some(Either::Left(iter::once((Severity::Error, value)))), + "cfg_attr" => { + let mut lint_attrs = Vec::new(); + cfg_attr_lint_attrs(sema, &value, &mut lint_attrs); + Some(Either::Right(lint_attrs.into_iter())) + } _ => None, }) }) + .flatten() .flat_map(move |(severity, lints)| { parse_tt_as_comma_sep_paths(lints, edition).into_iter().flat_map(move |lints| { // Rejoin the idents with `::`, so we have no spaces in between. @@ -836,6 +845,58 @@ fn lint_attrs( }) } +fn cfg_attr_lint_attrs( + sema: &Semantics<'_, RootDatabase>, + value: &ast::TokenTree, + lint_attrs: &mut Vec<(Severity, ast::TokenTree)>, +) { + let prev_len = lint_attrs.len(); + + let mut iter = value.token_trees_and_tokens().filter(|it| match it { + NodeOrToken::Node(_) => true, + NodeOrToken::Token(it) => !it.kind().is_trivia(), + }); + + // Skip the condition. + for value in &mut iter { + if value.as_token().is_some_and(|it| it.kind() == T![,]) { + break; + } + } + + while let Some(value) = iter.next() { + if let Some(token) = value.as_token() { + if token.kind() == SyntaxKind::IDENT { + let severity = match token.text() { + "allow" | "expect" => Some(Severity::Allow), + "warn" => Some(Severity::Warning), + "forbid" | "deny" => Some(Severity::Error), + "cfg_attr" => { + if let Some(NodeOrToken::Node(value)) = iter.next() { + cfg_attr_lint_attrs(sema, &value, lint_attrs); + } + None + } + _ => None, + }; + if let Some(severity) = severity { + let lints = iter.next(); + if let Some(NodeOrToken::Node(lints)) = lints { + lint_attrs.push((severity, lints)); + } + } + } + } + } + + if prev_len != lint_attrs.len() { + if let Some(false) | None = sema.check_cfg_attr(value) { + // Discard the attributes when the condition is false. + lint_attrs.truncate(prev_len); + } + } +} + fn lint_groups(lint: &DiagnosticCode) -> &'static [&'static str] { match lint { DiagnosticCode::RustcLint(name) => {