Skip to content

Commit

Permalink
Auto merge of #18099 - ChayimFriedman2:diag-only-necessary, r=Veykril
Browse files Browse the repository at this point in the history 
Use more correct handling of lint attributes

The previous analysis was top-down, and worked on a single file (expanding macros). The new analysis is bottom-up, starting from the diagnostics and climbing up the syntax and module tree.

While this is more efficient (and in fact, efficiency was the motivating reason to work on this), unfortunately the code was already fast enough. But luckily, it also fixes a correctness problem: outline parent modules' attributes were not respected for the previous analysis. Case lints specifically did their own analysis to accommodate that, but it was limited to only them. The new analysis works on all kinds of lints, present and future.

It was basically impossible to fix the old analysis without rewriting it because navigating the module hierarchy must come bottom-up, and if we already have a bottom-up analysis (including syntax analysis because modules can be nested in other syntax elements, including macros), it makes sense to use only this kind of analysis.

Few other bugs (not fundamental to the previous analysis) are also fixed, e.g. overwriting of lint levels (i.e. `#[allow(lint)] mod foo { #[warn(lint)] mod bar; }`.

After this PR is merged I intend to work on an editor command that does workspace-wide diagnostics analysis (that is, `rust-analyzer diagnostics` but from your editor and without having to spawn a new process, which will have to analyze the workspace from scratch). This can be useful to users who do not want to enable check on save because of its overhead, but want to see workspace wide diagnostics from r-a (or to maintainers of rust-analyzer).

Closes #18086.
Closes #18081.
Fixes #18056.
  • Loading branch information
@bors
bors committed Sep 12, 2024
2 parents 3831b72 + a933329 commit 50b7185
Show file tree
Hide file tree
Showing 6 changed files with 403 additions and 279 deletions.
14 changes: 12 additions & 2 deletions src/tools/rust-analyzer/crates/hir-def/src/nameres.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ use la_arena::Arena;
use rustc_hash::{FxHashMap, FxHashSet};
use span::{Edition, EditionedFileId, FileAstId, FileId, ROOT_ERASED_FILE_AST_ID};
use stdx::format_to;
use syntax::{ast, SmolStr};
use syntax::{ast, AstNode, SmolStr, SyntaxNode};
use triomphe::Arc;
use tt::TextRange;

Expand Down Expand Up @@ -291,7 +291,7 @@ impl ModuleOrigin {

/// Returns a node which defines this module.
/// That is, a file or a `mod foo {}` with items.
fn definition_source(&self, db: &dyn DefDatabase) -> InFile<ModuleSource> {
pub fn definition_source(&self, db: &dyn DefDatabase) -> InFile<ModuleSource> {
match self {
&ModuleOrigin::File { definition, .. } | &ModuleOrigin::CrateRoot { definition } => {
let sf = db.parse(definition).tree();
Expand Down Expand Up @@ -728,6 +728,16 @@ pub enum ModuleSource {
BlockExpr(ast::BlockExpr),
}

impl ModuleSource {
pub fn node(&self) -> SyntaxNode {
match self {
ModuleSource::SourceFile(it) => it.syntax().clone(),
ModuleSource::Module(it) => it.syntax().clone(),
ModuleSource::BlockExpr(it) => it.syntax().clone(),
}
}
}

/// See `sub_namespace_match()`.
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum MacroSubNs {
Expand Down
152 changes: 11 additions & 141 deletions src/tools/rust-analyzer/crates/hir-ty/src/diagnostics/decl_check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,14 @@ mod case_conv;
use std::fmt;

use hir_def::{
data::adt::VariantData, db::DefDatabase, hir::Pat, src::HasSource, AdtId, AttrDefId, ConstId,
EnumId, EnumVariantId, FunctionId, HasModule, ItemContainerId, Lookup, ModuleDefId, ModuleId,
StaticId, StructId, TraitId, TypeAliasId,
data::adt::VariantData, db::DefDatabase, hir::Pat, src::HasSource, AdtId, ConstId, EnumId,
EnumVariantId, FunctionId, HasModule, ItemContainerId, Lookup, ModuleDefId, ModuleId, StaticId,
StructId, TraitId, TypeAliasId,
};
use hir_expand::{
name::{AsName, Name},
HirFileId, HirFileIdExt, MacroFileIdExt,
HirFileId, HirFileIdExt,
};
use intern::sym;
use stdx::{always, never};
use syntax::{
ast::{self, HasName},
Expand All @@ -36,14 +35,6 @@ use crate::db::HirDatabase;

use self::case_conv::{to_camel_case, to_lower_snake_case, to_upper_snake_case};

mod allow {
pub(super) const BAD_STYLE: &str = "bad_style";
pub(super) const NONSTANDARD_STYLE: &str = "nonstandard_style";
pub(super) const NON_SNAKE_CASE: &str = "non_snake_case";
pub(super) const NON_UPPER_CASE_GLOBAL: &str = "non_upper_case_globals";
pub(super) const NON_CAMEL_CASE_TYPES: &str = "non_camel_case_types";
}

pub fn incorrect_case(db: &dyn HirDatabase, owner: ModuleDefId) -> Vec<IncorrectCase> {
let _p = tracing::info_span!("incorrect_case").entered();
let mut validator = DeclValidator::new(db);
Expand Down Expand Up @@ -160,92 +151,7 @@ impl<'a> DeclValidator<'a> {
}
}

/// Checks whether not following the convention is allowed for this item.
fn allowed(&self, id: AttrDefId, allow_name: &str, recursing: bool) -> bool {
let is_allowed = |def_id| {
let attrs = self.db.attrs(def_id);
// don't bug the user about directly no_mangle annotated stuff, they can't do anything about it
(!recursing && attrs.by_key(&sym::no_mangle).exists())
|| attrs.by_key(&sym::allow).tt_values().any(|tt| {
let allows = tt.to_string();
allows.contains(allow_name)
|| allows.contains(allow::BAD_STYLE)
|| allows.contains(allow::NONSTANDARD_STYLE)
})
};
let db = self.db.upcast();
let file_id_is_derive = || {
match id {
AttrDefId::ModuleId(m) => {
m.def_map(db)[m.local_id].origin.file_id().map(Into::into)
}
AttrDefId::FunctionId(f) => Some(f.lookup(db).id.file_id()),
AttrDefId::StaticId(sid) => Some(sid.lookup(db).id.file_id()),
AttrDefId::ConstId(cid) => Some(cid.lookup(db).id.file_id()),
AttrDefId::TraitId(tid) => Some(tid.lookup(db).id.file_id()),
AttrDefId::TraitAliasId(taid) => Some(taid.lookup(db).id.file_id()),
AttrDefId::ImplId(iid) => Some(iid.lookup(db).id.file_id()),
AttrDefId::ExternBlockId(id) => Some(id.lookup(db).id.file_id()),
AttrDefId::ExternCrateId(id) => Some(id.lookup(db).id.file_id()),
AttrDefId::UseId(id) => Some(id.lookup(db).id.file_id()),
// These warnings should not explore macro definitions at all
AttrDefId::MacroId(_) => None,
AttrDefId::AdtId(aid) => match aid {
AdtId::StructId(sid) => Some(sid.lookup(db).id.file_id()),
AdtId::EnumId(eid) => Some(eid.lookup(db).id.file_id()),
// Unions aren't yet supported
AdtId::UnionId(_) => None,
},
AttrDefId::FieldId(_) => None,
AttrDefId::EnumVariantId(_) => None,
AttrDefId::TypeAliasId(_) => None,
AttrDefId::GenericParamId(_) => None,
}
.map_or(false, |file_id| {
matches!(file_id.macro_file(), Some(file_id) if file_id.is_custom_derive(db.upcast()) || file_id.is_builtin_derive(db.upcast()))
})
};

let parent = || {
match id {
AttrDefId::ModuleId(m) => m.containing_module(db).map(|v| v.into()),
AttrDefId::FunctionId(f) => Some(f.lookup(db).container.into()),
AttrDefId::StaticId(sid) => Some(sid.lookup(db).container.into()),
AttrDefId::ConstId(cid) => Some(cid.lookup(db).container.into()),
AttrDefId::TraitId(tid) => Some(tid.lookup(db).container.into()),
AttrDefId::TraitAliasId(taid) => Some(taid.lookup(db).container.into()),
AttrDefId::ImplId(iid) => Some(iid.lookup(db).container.into()),
AttrDefId::ExternBlockId(id) => Some(id.lookup(db).container.into()),
AttrDefId::ExternCrateId(id) => Some(id.lookup(db).container.into()),
AttrDefId::UseId(id) => Some(id.lookup(db).container.into()),
// These warnings should not explore macro definitions at all
AttrDefId::MacroId(_) => None,
AttrDefId::AdtId(aid) => match aid {
AdtId::StructId(sid) => Some(sid.lookup(db).container.into()),
AdtId::EnumId(eid) => Some(eid.lookup(db).container.into()),
// Unions aren't yet supported
AdtId::UnionId(_) => None,
},
AttrDefId::FieldId(_) => None,
AttrDefId::EnumVariantId(_) => None,
AttrDefId::TypeAliasId(_) => None,
AttrDefId::GenericParamId(_) => None,
}
.is_some_and(|mid| self.allowed(mid, allow_name, true))
};
is_allowed(id)
// FIXME: this is a hack to avoid false positives in derive macros currently
|| file_id_is_derive()
// go upwards one step or give up
|| parent()
}

fn validate_module(&mut self, module_id: ModuleId) {
// Check whether non-snake case identifiers are allowed for this module.
if self.allowed(module_id.into(), allow::NON_SNAKE_CASE, false) {
return;
}

// Check the module name.
let Some(module_name) = module_id.name(self.db.upcast()) else { return };
let Some(module_name_replacement) =
Expand All @@ -270,11 +176,6 @@ impl<'a> DeclValidator<'a> {
}

fn validate_trait(&mut self, trait_id: TraitId) {
// Check whether non-snake case identifiers are allowed for this trait.
if self.allowed(trait_id.into(), allow::NON_CAMEL_CASE_TYPES, false) {
return;
}

// Check the trait name.
let data = self.db.trait_data(trait_id);
self.create_incorrect_case_diagnostic_for_item_name(
Expand All @@ -292,11 +193,6 @@ impl<'a> DeclValidator<'a> {
return;
}

// Check whether non-snake case identifiers are allowed for this function.
if self.allowed(func.into(), allow::NON_SNAKE_CASE, false) {
return;
}

// Check the function name.
// Skipped if function is an associated item of a trait implementation.
if !self.is_trait_impl_container(container) {
Expand Down Expand Up @@ -389,28 +285,20 @@ impl<'a> DeclValidator<'a> {

fn validate_struct(&mut self, struct_id: StructId) {
// Check the structure name.
let non_camel_case_allowed =
self.allowed(struct_id.into(), allow::NON_CAMEL_CASE_TYPES, false);
if !non_camel_case_allowed {
let data = self.db.struct_data(struct_id);
self.create_incorrect_case_diagnostic_for_item_name(
struct_id,
&data.name,
CaseType::UpperCamelCase,
IdentType::Structure,
);
}
let data = self.db.struct_data(struct_id);
self.create_incorrect_case_diagnostic_for_item_name(
struct_id,
&data.name,
CaseType::UpperCamelCase,
IdentType::Structure,
);

// Check the field names.
self.validate_struct_fields(struct_id);
}

/// Check incorrect names for struct fields.
fn validate_struct_fields(&mut self, struct_id: StructId) {
if self.allowed(struct_id.into(), allow::NON_SNAKE_CASE, false) {
return;
}

let data = self.db.struct_data(struct_id);
let VariantData::Record(fields) = data.variant_data.as_ref() else {
return;
Expand Down Expand Up @@ -484,11 +372,6 @@ impl<'a> DeclValidator<'a> {
fn validate_enum(&mut self, enum_id: EnumId) {
let data = self.db.enum_data(enum_id);

// Check whether non-camel case names are allowed for this enum.
if self.allowed(enum_id.into(), allow::NON_CAMEL_CASE_TYPES, false) {
return;
}

// Check the enum name.
self.create_incorrect_case_diagnostic_for_item_name(
enum_id,
Expand Down Expand Up @@ -653,10 +536,6 @@ impl<'a> DeclValidator<'a> {
return;
}

if self.allowed(const_id.into(), allow::NON_UPPER_CASE_GLOBAL, false) {
return;
}

let data = self.db.const_data(const_id);
let Some(name) = &data.name else {
return;
Expand All @@ -676,10 +555,6 @@ impl<'a> DeclValidator<'a> {
return;
}

if self.allowed(static_id.into(), allow::NON_UPPER_CASE_GLOBAL, false) {
return;
}

self.create_incorrect_case_diagnostic_for_item_name(
static_id,
&data.name,
Expand All @@ -695,11 +570,6 @@ impl<'a> DeclValidator<'a> {
return;
}

// Check whether non-snake case identifiers are allowed for this type alias.
if self.allowed(type_alias_id.into(), allow::NON_CAMEL_CASE_TYPES, false) {
return;
}

// Check the type alias name.
let data = self.db.type_alias_data(type_alias_id);
self.create_incorrect_case_diagnostic_for_item_name(
Expand Down
45 changes: 43 additions & 2 deletions src/tools/rust-analyzer/crates/hir/src/semantics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use either::Either;
use hir_def::{
hir::Expr,
lower::LowerCtx,
nameres::MacroSubNs,
nameres::{MacroSubNs, ModuleOrigin},
path::ModPath,
resolver::{self, HasResolver, Resolver, TypeNs},
type_ref::Mutability,
Expand All @@ -32,7 +32,7 @@ use intern::Symbol;
use itertools::Itertools;
use rustc_hash::{FxHashMap, FxHashSet};
use smallvec::{smallvec, SmallVec};
use span::{EditionedFileId, FileId};
use span::{EditionedFileId, FileId, HirFileIdRepr};
use stdx::TupleExt;
use syntax::{
algo::skip_trivia_token,
Expand Down Expand Up @@ -323,6 +323,47 @@ impl<'db> SemanticsImpl<'db> {
tree
}

pub fn find_parent_file(&self, file_id: HirFileId) -> Option<InFile<SyntaxNode>> {
match file_id.repr() {
HirFileIdRepr::FileId(file_id) => {
let module = self.file_to_module_defs(file_id.file_id()).next()?;
let def_map = self.db.crate_def_map(module.krate().id);
match def_map[module.id.local_id].origin {
ModuleOrigin::CrateRoot { .. } => None,
ModuleOrigin::File { declaration, declaration_tree_id, .. } => {
let file_id = declaration_tree_id.file_id();
let in_file = InFile::new(file_id, declaration);
let node = in_file.to_node(self.db.upcast());
let root = find_root(node.syntax());
self.cache(root, file_id);
Some(in_file.with_value(node.syntax().clone()))
}
_ => unreachable!("FileId can only belong to a file module"),
}
}
HirFileIdRepr::MacroFile(macro_file) => {
let node = self
.db
.lookup_intern_macro_call(macro_file.macro_call_id)
.to_node(self.db.upcast());
let root = find_root(&node.value);
self.cache(root, node.file_id);
Some(node)
}
}
}

/// Returns the `SyntaxNode` of the module. If this is a file module, returns
/// the `SyntaxNode` of the *definition* file, not of the *declaration*.
pub fn module_definition_node(&self, module: Module) -> InFile<SyntaxNode> {
let def_map = module.id.def_map(self.db.upcast());
let definition = def_map[module.id.local_id].origin.definition_source(self.db.upcast());
let definition = definition.map(|it| it.node());
let root_node = find_root(&definition.value);
self.cache(root_node, definition.file_id);
definition
}

pub fn parse_or_expand(&self, file_id: HirFileId) -> SyntaxNode {
let node = self.db.parse_or_expand(file_id);
self.cache(node.clone(), file_id);
Expand Down
Loading

0 comments on commit 50b7185

Please sign in to comment.