From eeb5bfcfab1c41e3ad80b9e8ce69d2865c42abc6 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 5 Feb 2021 16:09:45 +0100 Subject: Cleanup decl_check --- Cargo.lock | 4 +- crates/hir_ty/src/diagnostics.rs | 33 +++- crates/hir_ty/src/diagnostics/decl_check.rs | 279 ++++++++++++---------------- crates/stdx/Cargo.toml | 2 +- 4 files changed, 154 insertions(+), 164 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8f1a8401f..1ceae965f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -17,9 +17,9 @@ checksum = "ee2a4ec343196209d6594e19543ae87a39f96d5534d7174822a3ad825dd6ed7e" [[package]] name = "always-assert" -version = "0.1.1" +version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "727786f78c5bc0cda8011831616589f72084cb16b7df4213a997b78749b55a60" +checksum = "fbf688625d06217d5b1bb0ea9d9c44a1635fd0ee3534466388d18203174f4d11" dependencies = [ "log", ] diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs index 323c5f963..08483760c 100644 --- a/crates/hir_ty/src/diagnostics.rs +++ b/crates/hir_ty/src/diagnostics.rs @@ -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 // // 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 ident: AstPtr, pub expected_case: CaseType, - pub ident_type: String, + pub ident_type: IdentType, pub ident_text: String, pub suggested_text: String, } diff --git a/crates/hir_ty/src/diagnostics/decl_check.rs b/crates/hir_ty/src/diagnostics/decl_check.rs index eaeb6899f..6773ddea3 100644 --- a/crates/hir_ty/src/diagnostics/decl_check.rs +++ b/crates/hir_ty/src/diagnostics/decl_check.rs @@ -23,6 +23,7 @@ use hir_expand::{ diagnostics::DiagnosticSink, name::{AsName, Name}, }; +use stdx::{always, never}; use syntax::{ ast::{self, NameOwner}, AstNode, AstPtr, @@ -31,7 +32,7 @@ use test_utils::mark; use crate::{ db::HirDatabase, - diagnostics::{decl_check::case_conv::*, CaseType, IncorrectCase}, + diagnostics::{decl_check::case_conv::*, CaseType, IdentType, IncorrectCase}, }; mod allow { @@ -40,7 +41,7 @@ mod allow { 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, krate: CrateId, 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::EnumId(enum_id) => self.validate_enum(enum_id), 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. let function_name = data.name.to_string(); - let fn_name_replacement = if let Some(new_name) = to_lower_snake_case(&function_name) { - let replacement = Replacement { - current_name: data.name.clone(), - suggested_text: new_name, - expected_case: CaseType::LowerSnakeCase, - }; - Some(replacement) - } else { - None - }; + let fn_name_replacement = to_lower_snake_case(&function_name).map(|new_name| Replacement { + current_name: data.name.clone(), + suggested_text: new_name, + expected_case: CaseType::LowerSnakeCase, + }); // Check the param names. - let mut fn_param_replacements = Vec::new(); - - for pat_id in body.params.iter().cloned() { - let pat = &body[pat_id]; - - let param_name = match pat { - Pat::Bind { name, .. } => name, - _ => continue, - }; - - let name = param_name.to_string(); - if let Some(new_name) = to_lower_snake_case(&name) { - let replacement = Replacement { + let fn_param_replacements = body + .params + .iter() + .filter_map(|&id| match &body[id] { + Pat::Bind { name, .. } => Some(name), + _ => None, + }) + .filter_map(|param_name| { + Some(Replacement { current_name: param_name.clone(), - suggested_text: new_name, + suggested_text: to_lower_snake_case(¶m_name.to_string())?, expected_case: CaseType::LowerSnakeCase, - }; - fn_param_replacements.push(replacement); - } - } + }) + }) + .collect(); // Check the patterns inside the function body. - let mut pats_replacements = Vec::new(); - - for (pat_idx, pat) in body.pats.iter() { - if body.params.contains(&pat_idx) { - // We aren't interested in function parameters, we've processed them above. - continue; - } - - let bind_name = match pat { - Pat::Bind { name, .. } => name, - _ => continue, - }; - - let name = bind_name.to_string(); - if let Some(new_name) = to_lower_snake_case(&name) { - let replacement = Replacement { - current_name: bind_name.clone(), - suggested_text: new_name, - expected_case: CaseType::LowerSnakeCase, - }; - pats_replacements.push((pat_idx, replacement)); - } - } + let pats_replacements = body + .pats + .iter() + // We aren't interested in function parameters, we've processed them above. + .filter(|(pat_idx, _)| !body.params.contains(&pat_idx)) + .filter_map(|(id, pat)| match pat { + Pat::Bind { name, .. } => Some((id, name)), + _ => None, + }) + .filter_map(|(id, bind_name)| { + Some(( + id, + Replacement { + current_name: bind_name.clone(), + suggested_text: to_lower_snake_case(&bind_name.to_string())?, + expected_case: CaseType::LowerSnakeCase, + }, + )) + }) + .collect(); // 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( @@ -199,8 +187,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let ast_ptr = match fn_src.value.name() { Some(name) => name, None => { - // We don't want rust-analyzer to panic over this, but it is definitely some kind of error in the logic. - log::error!( + never!( "Replacement ({:?}) was generated for a function without a name: {:?}", replacement, fn_src @@ -211,7 +198,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let diagnostic = IncorrectCase { file: fn_src.file_id, - ident_type: "Function".to_string(), + ident_type: IdentType::Function, ident: AstPtr::new(&ast_ptr).into(), expected_case: replacement.expected_case, 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() { Some(params) => params, None => { - if !fn_param_replacements.is_empty() { - log::error!( - "Replacements ({:?}) were generated for a function parameters which had no parameters list: {:?}", - fn_param_replacements, fn_src - ); - } + always!( + fn_param_replacements.is_empty(), + "Replacements ({:?}) were generated for a function parameters which had no parameters list: {:?}", + fn_param_replacements, + fn_src + ); 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. let ast_ptr: ast::Name = loop { match fn_params_iter.next() { - Some(element) - if pat_equals_to_name(element.pat(), ¶m_to_rename.current_name) => - { - if let ast::Pat::IdentPat(pat) = element.pat().unwrap() { - break pat.name().unwrap(); - } else { - // This is critical. If we consider this parameter the expected one, - // it **must** have a name. - panic!( - "Pattern {:?} equals to expected replacement {:?}, but has no name", - element, param_to_rename - ); + Some(element) => { + if let Some(ast::Pat::IdentPat(pat)) = element.pat() { + if pat.to_string() == param_to_rename.current_name.to_string() { + if let Some(name) = pat.name() { + break name; + } + // This is critical. If we consider this parameter the expected one, + // it **must** have a name. + never!( + "Pattern {:?} equals to expected replacement {:?}, but has no name", + element, + param_to_rename + ); + return; + } } } - Some(_) => {} None => { - log::error!( + never!( "Replacement ({:?}) was generated for a function parameter which was not found: {:?}", param_to_rename, fn_src ); @@ -267,7 +256,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let diagnostic = IncorrectCase { file: fn_src.file_id, - ident_type: "Argument".to_string(), + ident_type: IdentType::Argument, ident: AstPtr::new(&ast_ptr).into(), expected_case: param_to_rename.expected_case, 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, // because e.g. match arms are patterns as well. // In other words, we check that it's a named variable binding. - let is_binding = ast::LetStmt::cast(parent.clone()).is_some() - || (ast::MatchArm::cast(parent).is_some() + let is_binding = ast::LetStmt::can_cast(parent.kind()) + || (ast::MatchArm::can_cast(parent.kind()) && ident_pat.at_token().is_some()); if !is_binding { // 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 { file: source_ptr.file_id, - ident_type: "Variable".to_string(), + ident_type: IdentType::Variable, ident: AstPtr::new(&name_ast).into(), expected_case: replacement.expected_case, ident_text: replacement.current_name.to_string(), @@ -341,17 +330,12 @@ impl<'a, 'b> DeclValidator<'a, 'b> { // Check the structure name. let struct_name = data.name.to_string(); - let struct_name_replacement = if let Some(new_name) = to_camel_case(&struct_name) { - let replacement = Replacement { + let struct_name_replacement = if !non_camel_case_allowed { + to_camel_case(&struct_name).map(|new_name| Replacement { current_name: data.name.clone(), suggested_text: new_name, expected_case: CaseType::UpperCamelCase, - }; - if non_camel_case_allowed { - None - } else { - Some(replacement) - } + }) } else { None }; @@ -403,8 +387,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let ast_ptr = match struct_src.value.name() { Some(name) => name, None => { - // We don't want rust-analyzer to panic over this, but it is definitely some kind of error in the logic. - log::error!( + never!( "Replacement ({:?}) was generated for a structure without a name: {:?}", replacement, struct_src @@ -415,7 +398,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let diagnostic = IncorrectCase { file: struct_src.file_id, - ident_type: "Structure".to_string(), + ident_type: IdentType::Structure, ident: AstPtr::new(&ast_ptr).into(), expected_case: replacement.expected_case, 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() { Some(ast::FieldList::RecordFieldList(fields)) => fields, _ => { - if !struct_fields_replacements.is_empty() { - log::error!( - "Replacements ({:?}) were generated for a structure fields which had no fields list: {:?}", - struct_fields_replacements, struct_src - ); - } + always!( + struct_fields_replacements.is_empty(), + "Replacements ({:?}) were generated for a structure fields which had no fields list: {:?}", + struct_fields_replacements, + struct_src + ); 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 // actual params list, but just some of them (ones that named correctly) are skipped. let ast_ptr = loop { - match struct_fields_iter.next() { - Some(element) if names_equal(element.name(), &field_to_rename.current_name) => { - break element.name().unwrap() + match struct_fields_iter.next().and_then(|field| field.name()) { + Some(field_name) => { + if field_name.as_name() == field_to_rename.current_name { + break field_name; + } } - Some(_) => {} None => { - log::error!( + never!( "Replacement ({:?}) was generated for a structure field which was not found: {:?}", field_to_rename, struct_src ); @@ -459,7 +443,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let diagnostic = IncorrectCase { file: struct_src.file_id, - ident_type: "Field".to_string(), + ident_type: IdentType::Field, ident: AstPtr::new(&ast_ptr).into(), expected_case: field_to_rename.expected_case, ident_text: field_to_rename.current_name.to_string(), @@ -480,31 +464,24 @@ impl<'a, 'b> DeclValidator<'a, 'b> { // Check the enum name. let enum_name = data.name.to_string(); - let enum_name_replacement = if let Some(new_name) = to_camel_case(&enum_name) { - let replacement = Replacement { - current_name: data.name.clone(), - suggested_text: new_name, - expected_case: CaseType::UpperCamelCase, - }; - Some(replacement) - } else { - None - }; + let enum_name_replacement = to_camel_case(&enum_name).map(|new_name| Replacement { + current_name: data.name.clone(), + suggested_text: new_name, + expected_case: CaseType::UpperCamelCase, + }); // Check the field names. - let mut enum_fields_replacements = Vec::new(); - - for (_, variant) in data.variants.iter() { - let variant_name = variant.name.to_string(); - if let Some(new_name) = to_camel_case(&variant_name) { - let replacement = Replacement { + let enum_fields_replacements = data + .variants + .iter() + .filter_map(|(_, variant)| { + Some(Replacement { current_name: variant.name.clone(), - suggested_text: new_name, + suggested_text: to_camel_case(&variant.name.to_string())?, 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. 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() { Some(name) => name, None => { - // We don't want rust-analyzer to panic over this, but it is definitely some kind of error in the logic. - log::error!( + never!( "Replacement ({:?}) was generated for a enum without a name: {:?}", replacement, enum_src @@ -546,7 +522,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let diagnostic = IncorrectCase { file: enum_src.file_id, - ident_type: "Enum".to_string(), + ident_type: IdentType::Enum, ident: AstPtr::new(&ast_ptr).into(), expected_case: replacement.expected_case, 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() { Some(variants) => variants, _ => { - if !enum_variants_replacements.is_empty() { - log::error!( - "Replacements ({:?}) were generated for a enum variants which had no fields list: {:?}", - enum_variants_replacements, enum_src - ); - } + always!( + enum_variants_replacements.is_empty(), + "Replacements ({:?}) were generated for a enum variants which had no fields list: {:?}", + enum_variants_replacements, + enum_src + ); 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 // actual params list, but just some of them (ones that named correctly) are skipped. let ast_ptr = loop { - match enum_variants_iter.next() { - Some(variant) - if names_equal(variant.name(), &variant_to_rename.current_name) => - { - break variant.name().unwrap() + match enum_variants_iter.next().and_then(|v| v.name()) { + Some(variant_name) => { + if variant_name.as_name() == variant_to_rename.current_name { + break variant_name; + } } - Some(_) => {} None => { - log::error!( + never!( "Replacement ({:?}) was generated for a enum variant which was not found: {:?}", variant_to_rename, enum_src ); @@ -592,7 +567,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let diagnostic = IncorrectCase { file: enum_src.file_id, - ident_type: "Variant".to_string(), + ident_type: IdentType::Variant, ident: AstPtr::new(&ast_ptr).into(), expected_case: variant_to_rename.expected_case, ident_text: variant_to_rename.current_name.to_string(), @@ -637,7 +612,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let diagnostic = IncorrectCase { file: const_src.file_id, - ident_type: "Constant".to_string(), + ident_type: IdentType::Constant, ident: AstPtr::new(&ast_ptr).into(), expected_case: replacement.expected_case, ident_text: replacement.current_name.to_string(), @@ -685,7 +660,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let diagnostic = IncorrectCase { file: static_src.file_id, - ident_type: "Static variable".to_string(), + ident_type: IdentType::StaticVariable, ident: AstPtr::new(&ast_ptr).into(), expected_case: replacement.expected_case, ident_text: replacement.current_name.to_string(), @@ -696,22 +671,6 @@ impl<'a, 'b> DeclValidator<'a, 'b> { } } -fn names_equal(left: Option, right: &Name) -> bool { - if let Some(left) = left { - &left.as_name() == right - } else { - false - } -} - -fn pat_equals_to_name(pat: Option, name: &Name) -> bool { - if let Some(ast::Pat::IdentPat(ident)) = pat { - ident.to_string() == name.to_string() - } else { - false - } -} - #[cfg(test)] mod tests { use test_utils::mark; diff --git a/crates/stdx/Cargo.toml b/crates/stdx/Cargo.toml index 5866c0a28..d28b5e658 100644 --- a/crates/stdx/Cargo.toml +++ b/crates/stdx/Cargo.toml @@ -11,7 +11,7 @@ doctest = false [dependencies] 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 [features] -- cgit v1.2.3