squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Cleanup decl_check

Browse source
This commit is contained in:
Lukas Wirth 2021-02-05 16:09:45 +01:00
parent ac5958485e
commit eeb5bfcfab
4 changed files with 154 additions and 164 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
Cargo.lock generated
View file

@ -17,9 +17,9 @@ checksum = "ee2a4ec343196209d6594e19543ae87a39f96d5534d7174822a3ad825dd6ed7e"
[[package]] [[package]]
name = "always-assert" name = "always-assert"
version = "0.1.1" version = "0.1.2"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "727786f78c5bc0cda8011831616589f72084cb16b7df4213a997b78749b55a60" checksum = "fbf688625d06217d5b1bb0ea9d9c44a1635fd0ee3534466388d18203174f4d11"
dependencies = [ dependencies = [
"log", "log",
] ]

33
crates/hir_ty/src/diagnostics.rs
View file

@ -345,6 +345,37 @@ impl fmt::Display for CaseType {
} }
} }
#[derive(Debug)]
pub enum IdentType {
Argument,
Constant,
Enum,
Field,
Function,
StaticVariable,
Structure,
Variable,
Variant,
}
impl fmt::Display for IdentType {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let repr = match self {
IdentType::Argument => "Argument",
IdentType::Constant => "Constant",
IdentType::Enum => "Enum",
IdentType::Field => "Field",
IdentType::Function => "Function",
IdentType::StaticVariable => "Static variable",
IdentType::Structure => "Structure",
IdentType::Variable => "Variable",
IdentType::Variant => "Variant",
};
write!(f, "{}", repr)
}
}
// Diagnostic: incorrect-ident-case // Diagnostic: incorrect-ident-case
// //
// This diagnostic is triggered if an item name doesn't follow https://doc.rust-lang.org/1.0.0/style/style/naming/README.html[Rust naming convention]. // This diagnostic is triggered if an item name doesn't follow https://doc.rust-lang.org/1.0.0/style/style/naming/README.html[Rust naming convention].
@ -353,7 +384,7 @@ pub struct IncorrectCase {
pub file: HirFileId, pub file: HirFileId,
pub ident: AstPtr<ast::Name>, pub ident: AstPtr<ast::Name>,
pub expected_case: CaseType, pub expected_case: CaseType,
pub ident_type: String, pub ident_type: IdentType,
pub ident_text: String, pub ident_text: String,
pub suggested_text: String, pub suggested_text: String,
} }

279
crates/hir_ty/src/diagnostics/decl_check.rs
View file

@ -23,6 +23,7 @@ use hir_expand::{
diagnostics::DiagnosticSink, diagnostics::DiagnosticSink,
name::{AsName, Name}, name::{AsName, Name},
}; };
use stdx::{always, never};
use syntax::{ use syntax::{
ast::{self, NameOwner}, ast::{self, NameOwner},
AstNode, AstPtr, AstNode, AstPtr,
@ -31,7 +32,7 @@ use test_utils::mark;
use crate::{ use crate::{
db::HirDatabase, db::HirDatabase,
diagnostics::{decl_check::case_conv::*, CaseType, IncorrectCase}, diagnostics::{decl_check::case_conv::*, CaseType, IdentType, IncorrectCase},
}; };
mod allow { mod allow {
@ -40,7 +41,7 @@ mod allow {
pub(super) const NON_CAMEL_CASE_TYPES: &str = "non_camel_case_types"; pub(super) const NON_CAMEL_CASE_TYPES: &str = "non_camel_case_types";
} }
pub(super) struct DeclValidator<'a, 'b: 'a> { pub(super) struct DeclValidator<'a, 'b> {
db: &'a dyn HirDatabase, db: &'a dyn HirDatabase,
krate: CrateId, krate: CrateId,
sink: &'a mut DiagnosticSink<'b>, sink: &'a mut DiagnosticSink<'b>,
@ -77,7 +78,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
AdtId::StructId(struct_id) => self.validate_struct(struct_id), AdtId::StructId(struct_id) => self.validate_struct(struct_id),
AdtId::EnumId(enum_id) => self.validate_enum(enum_id), AdtId::EnumId(enum_id) => self.validate_enum(enum_id),
AdtId::UnionId(_) => { AdtId::UnionId(_) => {
// Unions aren't yet supported by this validator. // FIXME: Unions aren't yet supported by this validator.
} }
} }
} }
@ -111,63 +112,50 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
// Check the function name. // Check the function name.
let function_name = data.name.to_string(); let function_name = data.name.to_string();
let fn_name_replacement = if let Some(new_name) = to_lower_snake_case(&function_name) { let fn_name_replacement = to_lower_snake_case(&function_name).map(|new_name| Replacement {
let replacement = Replacement { current_name: data.name.clone(),
current_name: data.name.clone(), suggested_text: new_name,
suggested_text: new_name, expected_case: CaseType::LowerSnakeCase,
expected_case: CaseType::LowerSnakeCase, });
};
Some(replacement)
} else {
None
};
// Check the param names. // Check the param names.
let mut fn_param_replacements = Vec::new(); let fn_param_replacements = body
.params
for pat_id in body.params.iter().cloned() { .iter()
let pat = &body[pat_id]; .filter_map(|&id| match &body[id] {
Pat::Bind { name, .. } => Some(name),
let param_name = match pat { _ => None,
Pat::Bind { name, .. } => name, })
_ => continue, .filter_map(|param_name| {
}; Some(Replacement {
let name = param_name.to_string();
if let Some(new_name) = to_lower_snake_case(&name) {
let replacement = Replacement {
current_name: param_name.clone(), current_name: param_name.clone(),
suggested_text: new_name, suggested_text: to_lower_snake_case(&param_name.to_string())?,
expected_case: CaseType::LowerSnakeCase, expected_case: CaseType::LowerSnakeCase,
}; })
fn_param_replacements.push(replacement); })
} .collect();
}
// Check the patterns inside the function body. // Check the patterns inside the function body.
let mut pats_replacements = Vec::new(); let pats_replacements = body
.pats
for (pat_idx, pat) in body.pats.iter() { .iter()
if body.params.contains(&pat_idx) { // We aren't interested in function parameters, we've processed them above.
// We aren't interested in function parameters, we've processed them above. .filter(|(pat_idx, _)| !body.params.contains(&pat_idx))
continue; .filter_map(|(id, pat)| match pat {
} Pat::Bind { name, .. } => Some((id, name)),
_ => None,
let bind_name = match pat { })
Pat::Bind { name, .. } => name, .filter_map(|(id, bind_name)| {
_ => continue, Some((
}; id,
Replacement {
let name = bind_name.to_string(); current_name: bind_name.clone(),
if let Some(new_name) = to_lower_snake_case(&name) { suggested_text: to_lower_snake_case(&bind_name.to_string())?,
let replacement = Replacement { expected_case: CaseType::LowerSnakeCase,
current_name: bind_name.clone(), },
suggested_text: new_name, ))
expected_case: CaseType::LowerSnakeCase, })
}; .collect();
pats_replacements.push((pat_idx, replacement));
}
}
// If there is at least one element to spawn a warning on, go to the source map and generate a warning. // If there is at least one element to spawn a warning on, go to the source map and generate a warning.
self.create_incorrect_case_diagnostic_for_func( self.create_incorrect_case_diagnostic_for_func(
@ -199,8 +187,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let ast_ptr = match fn_src.value.name() { let ast_ptr = match fn_src.value.name() {
Some(name) => name, Some(name) => name,
None => { None => {
// We don't want rust-analyzer to panic over this, but it is definitely some kind of error in the logic. never!(
log::error!(
"Replacement ({:?}) was generated for a function without a name: {:?}", "Replacement ({:?}) was generated for a function without a name: {:?}",
replacement, replacement,
fn_src fn_src
@ -211,7 +198,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: fn_src.file_id, file: fn_src.file_id,
ident_type: "Function".to_string(), ident_type: IdentType::Function,
ident: AstPtr::new(&ast_ptr).into(), ident: AstPtr::new(&ast_ptr).into(),
expected_case: replacement.expected_case, expected_case: replacement.expected_case,
ident_text: replacement.current_name.to_string(), ident_text: replacement.current_name.to_string(),
@ -225,12 +212,12 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let fn_params_list = match fn_src.value.param_list() { let fn_params_list = match fn_src.value.param_list() {
Some(params) => params, Some(params) => params,
None => { None => {
if !fn_param_replacements.is_empty() { always!(
log::error!( fn_param_replacements.is_empty(),
"Replacements ({:?}) were generated for a function parameters which had no parameters list: {:?}", "Replacements ({:?}) were generated for a function parameters which had no parameters list: {:?}",
fn_param_replacements, fn_src fn_param_replacements,
); fn_src
} );
return; return;
} }
}; };
@ -240,23 +227,25 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
// actual params list, but just some of them (ones that named correctly) are skipped. // actual params list, but just some of them (ones that named correctly) are skipped.
let ast_ptr: ast::Name = loop { let ast_ptr: ast::Name = loop {
match fn_params_iter.next() { match fn_params_iter.next() {
Some(element) Some(element) => {
if pat_equals_to_name(element.pat(), &param_to_rename.current_name) => if let Some(ast::Pat::IdentPat(pat)) = element.pat() {
{ if pat.to_string() == param_to_rename.current_name.to_string() {
if let ast::Pat::IdentPat(pat) = element.pat().unwrap() { if let Some(name) = pat.name() {
break pat.name().unwrap(); break name;
} else { }
// This is critical. If we consider this parameter the expected one, // This is critical. If we consider this parameter the expected one,
// it **must** have a name. // it **must** have a name.
panic!( never!(
"Pattern {:?} equals to expected replacement {:?}, but has no name", "Pattern {:?} equals to expected replacement {:?}, but has no name",
element, param_to_rename element,
); param_to_rename
);
return;
}
} }
} }
Some(_) => {}
None => { None => {
log::error!( never!(
"Replacement ({:?}) was generated for a function parameter which was not found: {:?}", "Replacement ({:?}) was generated for a function parameter which was not found: {:?}",
param_to_rename, fn_src param_to_rename, fn_src
); );
@ -267,7 +256,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: fn_src.file_id, file: fn_src.file_id,
ident_type: "Argument".to_string(), ident_type: IdentType::Argument,
ident: AstPtr::new(&ast_ptr).into(), ident: AstPtr::new(&ast_ptr).into(),
expected_case: param_to_rename.expected_case, expected_case: param_to_rename.expected_case,
ident_text: param_to_rename.current_name.to_string(), ident_text: param_to_rename.current_name.to_string(),
@ -309,8 +298,8 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
// We have to check that it's either `let var = ...` or `var @ Variant(_)` statement, // We have to check that it's either `let var = ...` or `var @ Variant(_)` statement,
// because e.g. match arms are patterns as well. // because e.g. match arms are patterns as well.
// In other words, we check that it's a named variable binding. // In other words, we check that it's a named variable binding.
let is_binding = ast::LetStmt::cast(parent.clone()).is_some() let is_binding = ast::LetStmt::can_cast(parent.kind())
|| (ast::MatchArm::cast(parent).is_some() || (ast::MatchArm::can_cast(parent.kind())
&& ident_pat.at_token().is_some()); && ident_pat.at_token().is_some());
if !is_binding { if !is_binding {
// This pattern is not an actual variable declaration, e.g. `Some(val) => {..}` match arm. // This pattern is not an actual variable declaration, e.g. `Some(val) => {..}` match arm.
@ -319,7 +308,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: source_ptr.file_id, file: source_ptr.file_id,
ident_type: "Variable".to_string(), ident_type: IdentType::Variable,
ident: AstPtr::new(&name_ast).into(), ident: AstPtr::new(&name_ast).into(),
expected_case: replacement.expected_case, expected_case: replacement.expected_case,
ident_text: replacement.current_name.to_string(), ident_text: replacement.current_name.to_string(),
@ -341,17 +330,12 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
// Check the structure name. // Check the structure name.
let struct_name = data.name.to_string(); let struct_name = data.name.to_string();
let struct_name_replacement = if let Some(new_name) = to_camel_case(&struct_name) { let struct_name_replacement = if !non_camel_case_allowed {
let replacement = Replacement { to_camel_case(&struct_name).map(|new_name| Replacement {
current_name: data.name.clone(), current_name: data.name.clone(),
suggested_text: new_name, suggested_text: new_name,
expected_case: CaseType::UpperCamelCase, expected_case: CaseType::UpperCamelCase,
}; })
if non_camel_case_allowed {
None
} else {
Some(replacement)
}
} else { } else {
None None
}; };
@ -403,8 +387,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let ast_ptr = match struct_src.value.name() { let ast_ptr = match struct_src.value.name() {
Some(name) => name, Some(name) => name,
None => { None => {
// We don't want rust-analyzer to panic over this, but it is definitely some kind of error in the logic. never!(
log::error!(
"Replacement ({:?}) was generated for a structure without a name: {:?}", "Replacement ({:?}) was generated for a structure without a name: {:?}",
replacement, replacement,
struct_src struct_src
@ -415,7 +398,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: struct_src.file_id, file: struct_src.file_id,
ident_type: "Structure".to_string(), ident_type: IdentType::Structure,
ident: AstPtr::new(&ast_ptr).into(), ident: AstPtr::new(&ast_ptr).into(),
expected_case: replacement.expected_case, expected_case: replacement.expected_case,
ident_text: replacement.current_name.to_string(), ident_text: replacement.current_name.to_string(),
@ -428,12 +411,12 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let struct_fields_list = match struct_src.value.field_list() { let struct_fields_list = match struct_src.value.field_list() {
Some(ast::FieldList::RecordFieldList(fields)) => fields, Some(ast::FieldList::RecordFieldList(fields)) => fields,
_ => { _ => {
if !struct_fields_replacements.is_empty() { always!(
log::error!( struct_fields_replacements.is_empty(),
"Replacements ({:?}) were generated for a structure fields which had no fields list: {:?}", "Replacements ({:?}) were generated for a structure fields which had no fields list: {:?}",
struct_fields_replacements, struct_src struct_fields_replacements,
); struct_src
} );
return; return;
} }
}; };
@ -442,13 +425,14 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
// We assume that parameters in replacement are in the same order as in the // We assume that parameters in replacement are in the same order as in the
// actual params list, but just some of them (ones that named correctly) are skipped. // actual params list, but just some of them (ones that named correctly) are skipped.
let ast_ptr = loop { let ast_ptr = loop {
match struct_fields_iter.next() { match struct_fields_iter.next().and_then(|field| field.name()) {
Some(element) if names_equal(element.name(), &field_to_rename.current_name) => { Some(field_name) => {
break element.name().unwrap() if field_name.as_name() == field_to_rename.current_name {
break field_name;
}
} }
Some(_) => {}
None => { None => {
log::error!( never!(
"Replacement ({:?}) was generated for a structure field which was not found: {:?}", "Replacement ({:?}) was generated for a structure field which was not found: {:?}",
field_to_rename, struct_src field_to_rename, struct_src
); );
@ -459,7 +443,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: struct_src.file_id, file: struct_src.file_id,
ident_type: "Field".to_string(), ident_type: IdentType::Field,
ident: AstPtr::new(&ast_ptr).into(), ident: AstPtr::new(&ast_ptr).into(),
expected_case: field_to_rename.expected_case, expected_case: field_to_rename.expected_case,
ident_text: field_to_rename.current_name.to_string(), ident_text: field_to_rename.current_name.to_string(),
@ -480,31 +464,24 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
// Check the enum name. // Check the enum name.
let enum_name = data.name.to_string(); let enum_name = data.name.to_string();
let enum_name_replacement = if let Some(new_name) = to_camel_case(&enum_name) { let enum_name_replacement = to_camel_case(&enum_name).map(|new_name| Replacement {
let replacement = Replacement { current_name: data.name.clone(),
current_name: data.name.clone(), suggested_text: new_name,
suggested_text: new_name, expected_case: CaseType::UpperCamelCase,
expected_case: CaseType::UpperCamelCase, });
};
Some(replacement)
} else {
None
};
// Check the field names. // Check the field names.
let mut enum_fields_replacements = Vec::new(); let enum_fields_replacements = data
.variants
for (_, variant) in data.variants.iter() { .iter()
let variant_name = variant.name.to_string(); .filter_map(|(_, variant)| {
if let Some(new_name) = to_camel_case(&variant_name) { Some(Replacement {
let replacement = Replacement {
current_name: variant.name.clone(), current_name: variant.name.clone(),
suggested_text: new_name, suggested_text: to_camel_case(&variant.name.to_string())?,
expected_case: CaseType::UpperCamelCase, expected_case: CaseType::UpperCamelCase,
}; })
enum_fields_replacements.push(replacement); })
} .collect();
}
// If there is at least one element to spawn a warning on, go to the source map and generate a warning. // If there is at least one element to spawn a warning on, go to the source map and generate a warning.
self.create_incorrect_case_diagnostic_for_enum( self.create_incorrect_case_diagnostic_for_enum(
@ -534,8 +511,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let ast_ptr = match enum_src.value.name() { let ast_ptr = match enum_src.value.name() {
Some(name) => name, Some(name) => name,
None => { None => {
// We don't want rust-analyzer to panic over this, but it is definitely some kind of error in the logic. never!(
log::error!(
"Replacement ({:?}) was generated for a enum without a name: {:?}", "Replacement ({:?}) was generated for a enum without a name: {:?}",
replacement, replacement,
enum_src enum_src
@ -546,7 +522,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: enum_src.file_id, file: enum_src.file_id,
ident_type: "Enum".to_string(), ident_type: IdentType::Enum,
ident: AstPtr::new(&ast_ptr).into(), ident: AstPtr::new(&ast_ptr).into(),
expected_case: replacement.expected_case, expected_case: replacement.expected_case,
ident_text: replacement.current_name.to_string(), ident_text: replacement.current_name.to_string(),
@ -559,12 +535,12 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let enum_variants_list = match enum_src.value.variant_list() { let enum_variants_list = match enum_src.value.variant_list() {
Some(variants) => variants, Some(variants) => variants,
_ => { _ => {
if !enum_variants_replacements.is_empty() { always!(
log::error!( enum_variants_replacements.is_empty(),
"Replacements ({:?}) were generated for a enum variants which had no fields list: {:?}", "Replacements ({:?}) were generated for a enum variants which had no fields list: {:?}",
enum_variants_replacements, enum_src enum_variants_replacements,
); enum_src
} );
return; return;
} }
}; };
@ -573,15 +549,14 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
// We assume that parameters in replacement are in the same order as in the // We assume that parameters in replacement are in the same order as in the
// actual params list, but just some of them (ones that named correctly) are skipped. // actual params list, but just some of them (ones that named correctly) are skipped.
let ast_ptr = loop { let ast_ptr = loop {
match enum_variants_iter.next() { match enum_variants_iter.next().and_then(|v| v.name()) {
Some(variant) Some(variant_name) => {
if names_equal(variant.name(), &variant_to_rename.current_name) => if variant_name.as_name() == variant_to_rename.current_name {
{ break variant_name;
break variant.name().unwrap() }
} }
Some(_) => {}
None => { None => {
log::error!( never!(
"Replacement ({:?}) was generated for a enum variant which was not found: {:?}", "Replacement ({:?}) was generated for a enum variant which was not found: {:?}",
variant_to_rename, enum_src variant_to_rename, enum_src
); );
@ -592,7 +567,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: enum_src.file_id, file: enum_src.file_id,
ident_type: "Variant".to_string(), ident_type: IdentType::Variant,
ident: AstPtr::new(&ast_ptr).into(), ident: AstPtr::new(&ast_ptr).into(),
expected_case: variant_to_rename.expected_case, expected_case: variant_to_rename.expected_case,
ident_text: variant_to_rename.current_name.to_string(), ident_text: variant_to_rename.current_name.to_string(),
@ -637,7 +612,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: const_src.file_id, file: const_src.file_id,
ident_type: "Constant".to_string(), ident_type: IdentType::Constant,
ident: AstPtr::new(&ast_ptr).into(), ident: AstPtr::new(&ast_ptr).into(),
expected_case: replacement.expected_case, expected_case: replacement.expected_case,
ident_text: replacement.current_name.to_string(), ident_text: replacement.current_name.to_string(),
@ -685,7 +660,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
let diagnostic = IncorrectCase { let diagnostic = IncorrectCase {
file: static_src.file_id, file: static_src.file_id,
ident_type: "Static variable".to_string(), ident_type: IdentType::StaticVariable,
ident: AstPtr::new(&ast_ptr).into(), ident: AstPtr::new(&ast_ptr).into(),
expected_case: replacement.expected_case, expected_case: replacement.expected_case,
ident_text: replacement.current_name.to_string(), ident_text: replacement.current_name.to_string(),
@ -696,22 +671,6 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
} }
} }
fn names_equal(left: Option<ast::Name>, right: &Name) -> bool {
if let Some(left) = left {
&left.as_name() == right
} else {
false
}
}
fn pat_equals_to_name(pat: Option<ast::Pat>, name: &Name) -> bool {
if let Some(ast::Pat::IdentPat(ident)) = pat {
ident.to_string() == name.to_string()
} else {
false
}
}
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use test_utils::mark; use test_utils::mark;

2
crates/stdx/Cargo.toml
View file

@ -11,7 +11,7 @@ doctest = false
[dependencies] [dependencies]
backtrace = { version = "0.3.44", optional = true } backtrace = { version = "0.3.44", optional = true }
always-assert = { version = "0.1.1", features = ["log"] } always-assert = { version = "0.1.2", features = ["log"] }
# Think twice before adding anything here # Think twice before adding anything here
[features] [features]