diff --git a/Arena.toml b/Arena.toml index 4201a84a2..5d043ef67 100644 --- a/Arena.toml +++ b/Arena.toml @@ -2065,6 +2065,11 @@ document = "stjudecloud/workflows:/workflows/dnaseq/dnaseq-standard-fastq.wdl" message = "dnaseq-standard-fastq.wdl:126:21: note[TrailingComma]: item missing trailing comma" permalink = "https://github.com/stjudecloud/workflows/blob/8377299f9d171af316f06570eedcd12e39600d72/workflows/dnaseq/dnaseq-standard-fastq.wdl/#L126" +[[diagnostics]] +document = "stjudecloud/workflows:/workflows/dnaseq/dnaseq-standard-fastq.wdl" +message = "dnaseq-standard-fastq.wdl:133:20: note[DisallowedDeclarationName]: declaration identifier 'array_lengths' contains type name 'Array'" +permalink = "https://github.com/stjudecloud/workflows/blob/8377299f9d171af316f06570eedcd12e39600d72/workflows/dnaseq/dnaseq-standard-fastq.wdl/#L133" + [[diagnostics]] document = "stjudecloud/workflows:/workflows/dnaseq/dnaseq-standard-fastq.wdl" message = "dnaseq-standard-fastq.wdl:162:20: note[ContainerValue]: container URI uses a mutable tag" @@ -2135,6 +2140,11 @@ document = "stjudecloud/workflows:/workflows/reference/qc-reference.wdl" message = "qc-reference.wdl:51:32: note[TrailingComma]: item missing trailing comma" permalink = "https://github.com/stjudecloud/workflows/blob/8377299f9d171af316f06570eedcd12e39600d72/workflows/reference/qc-reference.wdl/#L51" +[[diagnostics]] +document = "stjudecloud/workflows:/workflows/rnaseq/rnaseq-core.wdl" +message = "rnaseq-core.wdl:147:25: note[DisallowedDeclarationName]: declaration identifier 'htseq_strandedness_map' contains type name 'Map'" +permalink = "https://github.com/stjudecloud/workflows/blob/8377299f9d171af316f06570eedcd12e39600d72/workflows/rnaseq/rnaseq-core.wdl/#L147" + [[diagnostics]] document = "stjudecloud/workflows:/workflows/rnaseq/rnaseq-core.wdl" message = "rnaseq-core.wdl:49:22: note[TrailingComma]: item missing trailing comma" diff --git a/wdl-lint/CHANGELOG.md b/wdl-lint/CHANGELOG.md index 9a8482705..5cb018f62 100644 --- a/wdl-lint/CHANGELOG.md +++ b/wdl-lint/CHANGELOG.md @@ -10,9 +10,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added * Added suggestion for similar rule names when encountering unknown lint rules ([#334](https://github.com/stjude-rust-labs/wdl/pull/334)). +* Added `DisallowedDeclarationName` rule ([#343](https://github.com/stjude-rust-labs/wdl/pull/343)). ### Changed +* Added `InputSectionNode` and `OutputSectionNode` to `SnakeCase` `exceptable_nodes()` ([#343](https://github.com/stjude-rust-labs/wdl/pull/343)). * Updated to use new `wdl-ast` API ([#355](https://github.com/stjude-rust-labs/wdl/pull/355)). * Updated to Rust 2024 edition ([#353](https://github.com/stjude-rust-labs/wdl/pull/353)). * Relaxed `CommentWhitespace` rule so that it doesn't fire when a comment has extra spaces before it ([#314](https://github.com/stjude-rust-labs/wdl/pull/314)). diff --git a/wdl-lint/RULES.md b/wdl-lint/RULES.md index 4ac5098ae..fb3af2e64 100644 --- a/wdl-lint/RULES.md +++ b/wdl-lint/RULES.md @@ -16,6 +16,7 @@ be out of sync with released packages. | `DeprecatedObject` | Deprecated | Ensures that the deprecated `Object` construct is not used. | | `DeprecatedPlaceholderOption` | Deprecated | Ensures that the deprecated placeholder options construct is not used. | | `DescriptionMissing` | Completeness | Ensures that each meta section has a description key. | +| `DisallowedDeclarationName` | Naming | Ensures that declaration names do not contain their type information. | | `DisallowedInputName` | Naming | Ensures that input names are meaningful. | | `DisallowedOutputName` | Naming | Ensures that output names are meaningful. | | `DoubleQuotes` | Clarity, Style | Ensures that strings are defined using double quotes. | diff --git a/wdl-lint/src/lib.rs b/wdl-lint/src/lib.rs index 8655b88ae..473dceaf5 100644 --- a/wdl-lint/src/lib.rs +++ b/wdl-lint/src/lib.rs @@ -118,6 +118,7 @@ pub fn rules() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), Box::::default(), Box::::default(), Box::::default(), @@ -134,7 +135,7 @@ pub fn rules() -> Vec> { use convert_case::Case; use convert_case::Casing; let mut set = std::collections::HashSet::new(); - for r in rules.iter() { + for r in &rules { if r.id().to_case(Case::Pascal) != r.id() { panic!("lint rule id `{id}` is not pascal case", id = r.id()); } diff --git a/wdl-lint/src/rules.rs b/wdl-lint/src/rules.rs index 564f4aef4..d2bbf74b0 100644 --- a/wdl-lint/src/rules.rs +++ b/wdl-lint/src/rules.rs @@ -8,6 +8,7 @@ mod container_value; mod deprecated_object; mod deprecated_placeholder_option; mod description_missing; +mod disallowed_declaration_name; mod disallowed_input_name; mod disallowed_output_name; mod double_quotes; @@ -51,6 +52,7 @@ pub use container_value::*; pub use deprecated_object::*; pub use deprecated_placeholder_option::*; pub use description_missing::*; +pub use disallowed_declaration_name::*; pub use disallowed_input_name::*; pub use disallowed_output_name::*; pub use double_quotes::*; diff --git a/wdl-lint/src/rules/disallowed_declaration_name.rs b/wdl-lint/src/rules/disallowed_declaration_name.rs new file mode 100644 index 000000000..6722f790a --- /dev/null +++ b/wdl-lint/src/rules/disallowed_declaration_name.rs @@ -0,0 +1,159 @@ +//! A lint rule that disallows declaration names with type information. + +use wdl_ast::AstToken; +use wdl_ast::Diagnostic; +use wdl_ast::Diagnostics; +use wdl_ast::Document; +use wdl_ast::Span; +use wdl_ast::SupportedVersion; +use wdl_ast::SyntaxKind; +use wdl_ast::VisitReason; +use wdl_ast::Visitor; +use wdl_ast::v1::BoundDecl; +use wdl_ast::v1::Decl; +use wdl_ast::v1::PrimitiveTypeKind; +use wdl_ast::v1::Type; +use wdl_ast::v1::UnboundDecl; + +use crate::Rule; +use crate::Tag; +use crate::TagSet; + +/// A rule that identifies declaration names that include their type names. +#[derive(Debug, Default)] +pub struct DisallowedDeclarationNameRule; + +/// Create a diagnostic for a declaration identifier that contains its type +/// name. +fn decl_identifier_with_type(span: Span, decl_name: &str, type_name: &str) -> Diagnostic { + Diagnostic::note(format!( + "declaration identifier '{decl_name}' contains type name '{type_name}'", + )) + .with_rule("DisallowedDeclarationName") + .with_highlight(span) + .with_fix("rename the identifier to not include the type name") +} + +impl Rule for DisallowedDeclarationNameRule { + fn id(&self) -> &'static str { + "DisallowedDeclarationName" + } + + fn description(&self) -> &'static str { + "Disallows declaration names that include their type name." + } + + fn explanation(&self) -> &'static str { + "Declaration names should not include their type. This makes the code more verbose and \ + often redundant. For example, use 'counter' instead of 'counter_int' or 'is_active' \ + instead of 'is_active_bool'. Exceptions are made for String, File, and user-defined \ + struct types, which are not flagged by this rule." + } + + fn tags(&self) -> TagSet { + TagSet::new(&[Tag::Style, Tag::Clarity]) + } + + fn exceptable_nodes(&self) -> Option<&'static [SyntaxKind]> { + Some(&[ + SyntaxKind::VersionStatementNode, + SyntaxKind::InputSectionNode, + SyntaxKind::OutputSectionNode, + SyntaxKind::BoundDeclNode, + SyntaxKind::UnboundDeclNode, + SyntaxKind::TaskDefinitionNode, + SyntaxKind::WorkflowDefinitionNode, + ]) + } +} + +impl Visitor for DisallowedDeclarationNameRule { + type State = Diagnostics; + + fn document( + &mut self, + _: &mut Self::State, + reason: VisitReason, + _: &Document, + _: SupportedVersion, + ) { + if reason == VisitReason::Exit { + return; + } + + // Reset the visitor upon document entry + *self = Default::default(); + } + + fn bound_decl(&mut self, state: &mut Self::State, reason: VisitReason, decl: &BoundDecl) { + if reason == VisitReason::Enter { + check_decl_name(state, &Decl::Bound(decl.clone()), &self.exceptable_nodes()); + } + } + + fn unbound_decl(&mut self, state: &mut Self::State, reason: VisitReason, decl: &UnboundDecl) { + if reason == VisitReason::Enter { + check_decl_name( + state, + &Decl::Unbound(decl.clone()), + &self.exceptable_nodes(), + ); + } + } +} + +/// Check declaration name for type suffixes. +fn check_decl_name( + state: &mut Diagnostics, + decl: &Decl, + exceptable_nodes: &Option<&'static [SyntaxKind]>, +) { + let (type_name, alt_type_name) = match decl.ty() { + Type::Ref(_) => return, // Skip type reference types (user-defined structs) + Type::Primitive(ty) => { + match ty.kind() { + // Skip File and String types as they cause too many false positives + PrimitiveTypeKind::File | PrimitiveTypeKind::String => return, + PrimitiveTypeKind::Boolean => ("Boolean", Some("Bool")), + PrimitiveTypeKind::Integer => ("Int", Some("Integer")), + PrimitiveTypeKind::Float => ("Float", None), + PrimitiveTypeKind::Directory => ("Directory", Some("Dir")), + } + } + Type::Array(_) => ("Array", None), + Type::Map(_) => ("Map", None), + Type::Pair(_) => ("Pair", None), + Type::Object(_) => ("Object", None), + }; + + let ident = decl.name(); + let name = ident.text(); + let name_lower = name.to_lowercase(); + + for type_name in [type_name].into_iter().chain(alt_type_name) { + let type_lower = type_name.to_lowercase(); + + // Special handling for short type names (3 characters or less). + // These require word-based checks to avoid false positives. + if type_lower.len() <= 3 { + let words = convert_case::split(&name, &convert_case::Boundary::defaults()); + if words.into_iter().any(|w| w == type_lower) { + let diagnostic = decl_identifier_with_type(ident.span(), name, type_name); + state.exceptable_add( + diagnostic, + rowan::NodeOrToken::Node(decl.inner().to_owned()), + exceptable_nodes, + ); + return; + } + } else if name_lower.contains(&type_lower) { + let diagnostic = decl_identifier_with_type(ident.span(), name, type_name); + state.exceptable_add( + diagnostic, + rowan::NodeOrToken::Node(decl.inner().to_owned()), + exceptable_nodes, + ); + return; + } + } +} diff --git a/wdl-lint/src/rules/snake_case.rs b/wdl-lint/src/rules/snake_case.rs index 485af774a..a2457a8cc 100644 --- a/wdl-lint/src/rules/snake_case.rs +++ b/wdl-lint/src/rules/snake_case.rs @@ -140,6 +140,8 @@ impl Rule for SnakeCaseRule { SyntaxKind::StructDefinitionNode, SyntaxKind::TaskDefinitionNode, SyntaxKind::WorkflowDefinitionNode, + SyntaxKind::InputSectionNode, + SyntaxKind::OutputSectionNode, SyntaxKind::BoundDeclNode, SyntaxKind::UnboundDeclNode, ]) diff --git a/wdl-lint/tests/lints/deprecated-object/source.wdl b/wdl-lint/tests/lints/deprecated-object/source.wdl index 8b43a1dd0..95a792a19 100644 --- a/wdl-lint/tests/lints/deprecated-object/source.wdl +++ b/wdl-lint/tests/lints/deprecated-object/source.wdl @@ -2,7 +2,7 @@ version 1.1 -#@ except: MissingMetas, NonmatchingOutput +#@ except: MissingMetas, NonmatchingOutput, DisallowedDeclarationName workflow test { #@ except: DescriptionMissing meta {} diff --git a/wdl-lint/tests/lints/disallowed-declaration-name/source.errors b/wdl-lint/tests/lints/disallowed-declaration-name/source.errors new file mode 100644 index 000000000..fb4f4db29 --- /dev/null +++ b/wdl-lint/tests/lints/disallowed-declaration-name/source.errors @@ -0,0 +1,88 @@ +note[DisallowedDeclarationName]: declaration identifier 'arrayData' contains type name 'Array' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:13:20 + │ +13 │ Array[Int] arrayData + │ ^^^^^^^^^ + │ + = fix: rename the identifier to not include the type name + +note[DisallowedDeclarationName]: declaration identifier 'bool_flag' contains type name 'Bool' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:14:17 + │ +14 │ Boolean bool_flag + │ ^^^^^^^^^ + │ + = fix: rename the identifier to not include the type name + +note[DisallowedDeclarationName]: declaration identifier 'floatNumber' contains type name 'Float' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:15:15 + │ +15 │ Float floatNumber + │ ^^^^^^^^^^^ + │ + = fix: rename the identifier to not include the type name + +note[DisallowedDeclarationName]: declaration identifier 'my_int' contains type name 'Int' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:16:13 + │ +16 │ Int my_int + │ ^^^^^^ + │ + = fix: rename the identifier to not include the type name + +note[DisallowedDeclarationName]: declaration identifier 'dir' contains type name 'Dir' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:17:19 + │ +17 │ Directory dir + │ ^^^ + │ + = fix: rename the identifier to not include the type name + +note[DisallowedDeclarationName]: declaration identifier 'reference_directory' contains type name 'Directory' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:18:19 + │ +18 │ Directory reference_directory + │ ^^^^^^^^^^^^^^^^^^^ + │ + = fix: rename the identifier to not include the type name + +note[DisallowedDeclarationName]: declaration identifier 'count_int' contains type name 'Int' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:30:9 + │ +30 │ Int count_int = 42 + │ ^^^^^^^^^ + │ + = fix: rename the identifier to not include the type name + +note[DisallowedDeclarationName]: declaration identifier 'result_integer' contains type name 'Integer' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:31:9 + │ +31 │ Int result_integer = 42 + │ ^^^^^^^^^^^^^^ + │ + = fix: rename the identifier to not include the type name + +warning[SnakeCase]: private declaration name `foo_bar_InT` is not snake_case + ┌─ tests/lints/disallowed-declaration-name/source.wdl:35:9 + │ +35 │ Int foo_bar_InT = 42 # Split by convert-case to [foo, bar, In, T] + │ ^^^^^^^^^^^ this name must be snake_case + │ + = fix: replace `foo_bar_InT` with `foo_bar_in_t` + +warning[SnakeCase]: private declaration name `foo_bar_INT` is not snake_case + ┌─ tests/lints/disallowed-declaration-name/source.wdl:38:9 + │ +38 │ Int foo_bar_INT = 42 + │ ^^^^^^^^^^^ this name must be snake_case + │ + = fix: replace `foo_bar_INT` with `foo_bar_int` + +note[DisallowedDeclarationName]: declaration identifier 'result_int' contains type name 'Int' + ┌─ tests/lints/disallowed-declaration-name/source.wdl:45:13 + │ +45 │ Int result_int = 42 + │ ^^^^^^^^^^ + │ + = fix: rename the identifier to not include the type name + diff --git a/wdl-lint/tests/lints/disallowed-declaration-name/source.wdl b/wdl-lint/tests/lints/disallowed-declaration-name/source.wdl new file mode 100644 index 000000000..fd595f256 --- /dev/null +++ b/wdl-lint/tests/lints/disallowed-declaration-name/source.wdl @@ -0,0 +1,52 @@ +#@ except: MissingRequirements, InputSorting, NonmatchingOutput, MissingMetas + +version 1.2 + +task test_declaration_names { + meta { + description: "This is a test of disallowed declaration names" + } + + #@ except: SnakeCase + input { + # BAD + Array[Int] arrayData + Boolean bool_flag + Float floatNumber + Int my_int + Directory dir + Directory reference_directory + + # GOOD + Int intermittent + File Interval + String name + String name_str + String nameString + Directory direct_descendant + } + + # BAD + Int count_int = 42 + Int result_integer = 42 + + # GOOD + String name_string = "test" + Int foo_bar_InT = 42 # Split by convert-case to [foo, bar, In, T] + # Split by convert-case to [foo, bar, INT] + # and INT will not flag as we don't call `to_lowercase()` on the split words. + Int foo_bar_INT = 42 + + command <<<>>> + + #@ except: SnakeCase + output { + # BAD + Int result_int = 42 + # GOOD + File file = "output.txt" + String resultString = "result" + } + + runtime {} +} diff --git a/wdl-lint/tests/lints/disallowed-input-name/source.errors b/wdl-lint/tests/lints/disallowed-input-name/source.errors index a1e6b2b22..4f41091cb 100644 --- a/wdl-lint/tests/lints/disallowed-input-name/source.errors +++ b/wdl-lint/tests/lints/disallowed-input-name/source.errors @@ -1,31 +1,31 @@ note[DisallowedInputName]: declaration identifier must be at least 3 characters - ┌─ tests/lints/disallowed-input-name/source.wdl:19:14 + ┌─ tests/lints/disallowed-input-name/source.wdl:18:14 │ -19 │ File f # This is not OK +18 │ File f # This is not OK │ ^ │ = fix: rename the identifier to be at least 3 characters long note[DisallowedInputName]: declaration identifier starts with 'in' - ┌─ tests/lints/disallowed-input-name/source.wdl:20:16 + ┌─ tests/lints/disallowed-input-name/source.wdl:19:16 │ -20 │ String inString # This is not OK +19 │ String inString # This is not OK │ ^^^^^^^^ │ = fix: rename the identifier to not start with 'in' note[DisallowedInputName]: declaration identifier starts with 'input' - ┌─ tests/lints/disallowed-input-name/source.wdl:21:16 + ┌─ tests/lints/disallowed-input-name/source.wdl:20:16 │ -21 │ String input_string # This is not OK +20 │ String input_string # This is not OK │ ^^^^^^^^^^^^ │ = fix: rename the identifier to not start with 'input' note[DisallowedInputName]: declaration identifier starts with 'in' - ┌─ tests/lints/disallowed-input-name/source.wdl:22:16 + ┌─ tests/lints/disallowed-input-name/source.wdl:21:16 │ -22 │ String in_string # This is not OK +21 │ String in_string # This is not OK │ ^^^^^^^^^ │ = fix: rename the identifier to not start with 'in' diff --git a/wdl-lint/tests/lints/disallowed-input-name/source.wdl b/wdl-lint/tests/lints/disallowed-input-name/source.wdl index 8b2184dbc..1f629f7a2 100644 --- a/wdl-lint/tests/lints/disallowed-input-name/source.wdl +++ b/wdl-lint/tests/lints/disallowed-input-name/source.wdl @@ -12,7 +12,6 @@ task foo { input_string: "Not OK" in_string: "Not OK" invalid: "OK" - int: "OK" } input { @@ -21,7 +20,6 @@ task foo { String input_string # This is not OK String in_string # This is not OK String invalid # This is OK - Int int = 1 # This is OK } command <<< >>>