feat: implement DisallowedDeclarationName rule

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 have type suffixes.                                         |
 | `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.                                             |

wdl-lint/src/lib.rs

@@ -118,6 +118,7 @@ pub fn rules() -> Vec<Box<dyn Rule>> {
         Box::<rules::ExpressionSpacingRule>::default(),
         Box::<rules::DisallowedInputNameRule>::default(),
         Box::<rules::DisallowedOutputNameRule>::default(),
+        Box::<rules::DisallowedDeclarationNameRule>::default(),
         Box::<rules::ContainerValue>::default(),
         Box::<rules::MissingRequirementsRule>::default(),
         Box::<rules::UnknownRule>::default(),
@@ -134,7 +135,7 @@ pub fn rules() -> Vec<Box<dyn Rule>> {
         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());
             }

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::*;

wdl-lint/src/rules/disallowed_declaration_name.rs

@@ -0,0 +1,204 @@
+//! A lint rule that disallows declaration names with type suffixes.
+
+use std::collections::HashSet;
+
+use wdl_ast::AstNode;
+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::SyntaxElement;
+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 as a
+/// suffix.
+#[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::warning(format!(
+        "declaration identifier '{decl_name}' ends with 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 end with their type name"
+    }
+
+    fn explanation(&self) -> &'static str {
+        "Declaration names should not include their type as a suffix. This makes the code more \
+         verbose and often redundant. For example, use 'counter' instead of 'counter_int' or \
+         'is_active' instead of 'is_active_boolean'."
+    }
+
+    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 binding = decl.name();
+    let name = binding.text();
+    let ty = decl.ty();
+
+    // Extract type names to check based on the type
+    let mut type_names = HashSet::new();
+
+    // Handle different type variants
+    match ty {
+        Type::Ref(_) => return, // Skip type reference types (user-defined structs)
+        Type::Primitive(primitive_type) => {
+            match primitive_type.kind() {
+                PrimitiveTypeKind::Boolean => {
+                    type_names.insert(primitive_type.to_string());
+                    type_names.insert("Bool".to_string());
+                }
+                PrimitiveTypeKind::Integer => {
+                    // Integer is shortened to Int in WDL
+                    type_names.insert(primitive_type.to_string());
+                    type_names.insert("Integer".to_string());
+                }
+                PrimitiveTypeKind::Float => {
+                    type_names.insert(primitive_type.to_string());
+                }
+                PrimitiveTypeKind::Directory => {
+                    type_names.insert(primitive_type.to_string());
+                    type_names.insert("Dir".to_string());
+                }
+                // Include File and String types
+                PrimitiveTypeKind::File => {
+                    type_names.insert(primitive_type.to_string());
+                }
+                PrimitiveTypeKind::String => {
+                    type_names.insert(primitive_type.to_string());
+                }
+            }
+        }
+        Type::Array(_) => {
+            type_names.insert("Array".to_string());
+        }
+        Type::Map(_) => {
+            type_names.insert("Map".to_string());
+        }
+        Type::Pair(_) => {
+            type_names.insert("Pair".to_string());
+        }
+        Type::Object(_) => {
+            type_names.insert("Object".to_string());
+        }
+    }
+
+    let element = match decl {
+        Decl::Bound(d) => SyntaxElement::from(d.inner().clone()),
+        Decl::Unbound(d) => SyntaxElement::from(d.inner().clone()),
+    };
+
+    // Check if the declaration name ends with one of the type names
+    for type_name in &type_names {
+        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 = split_to_words(name);
+
+            if let Some(last_word) = words.last() {
+                if last_word.to_lowercase() == type_lower {
+                    let diagnostic = decl_identifier_with_type(binding.span(), name, type_name);
+                    state.exceptable_add(diagnostic, element.clone(), exceptable_nodes);
+                    return;
+                }
+            }
+        } else {
+            // For longer types, check if the identifier ends with the type name
+            if name.to_lowercase().ends_with(&type_lower) {
+                let diagnostic = decl_identifier_with_type(binding.span(), name, type_name);
+                state.exceptable_add(diagnostic, element.clone(), exceptable_nodes);
+                return;
+            }
+        }
+    }
+}
+
+/// Split an identifier into words using convert_case
+fn split_to_words(identifier: &str) -> Vec<String> {
+    // Use convert_case's built-in split functionality with default boundaries
+    convert_case::split(&identifier, &convert_case::Boundary::defaults())
+        .into_iter()
+        .map(|s| s.to_string())
+        .collect()
+}

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 {}

wdl-lint/tests/lints/disallowed-declaration-name/source.errors

@@ -0,0 +1,115 @@
+warning[NonmatchingOutput]: `outputs` key missing in `meta` section for the task `test_declaration_names`
+  ┌─ tests/lints/disallowed-declaration-name/source.wdl:5:5
+  │
+5 │     meta {
+  │     ^^^^
+  │
+  = fix: add an `outputs` key to `meta` section describing the outputs
+
+note[InputSorting]: input not sorted
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:23:5
+   │
+23 │     input {
+   │     ^^^^^ input section must be sorted
+   │
+   = fix: sort input statements as: 
+     File fileInput
+     File gtfFile
+     File validName
+     File reference
+     Array[Int] arrayData
+     String stringValue
+     String genome
+     Boolean booleanFlag
+     Float floatNumber
+     Int my_int
+     Int count
+
+warning[DisallowedDeclarationName]: declaration identifier 'gtfFile' ends with type name 'File'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:26:14
+   │
+26 │         File gtfFile
+   │              ^^^^^^^
+   │
+   = fix: rename the identifier to not include the type name
+
+warning[DisallowedDeclarationName]: declaration identifier 'my_int' ends with type name 'Int'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:27:13
+   │
+27 │         Int my_int
+   │             ^^^^^^
+   │
+   = fix: rename the identifier to not include the type name
+
+warning[DisallowedDeclarationName]: declaration identifier 'privateFile' ends with type name 'File'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:41:10
+   │
+41 │     File privateFile = "sample.txt"
+   │          ^^^^^^^^^^^
+   │
+   = fix: rename the identifier to not include the type name
+
+warning[DisallowedDeclarationName]: declaration identifier 'count_int' ends with type name 'Int'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:42:9
+   │
+42 │     Int count_int = 42
+   │         ^^^^^^^^^
+   │
+   = fix: rename the identifier to not include the type name
+
+warning[DisallowedDeclarationName]: declaration identifier 'nameString' ends with type name 'String'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:43:12
+   │
+43 │     String nameString = "test"
+   │            ^^^^^^^^^^
+   │
+   = fix: rename the identifier to not include the type name
+
+warning[DisallowedDeclarationName]: declaration identifier 'outputFile' ends with type name 'File'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:49:14
+   │
+49 │         File outputFile = "output.txt"
+   │              ^^^^^^^^^^
+   │
+   = fix: rename the identifier to not include the type name
+
+note[DisallowedOutputName]: declaration identifier starts with 'output'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:49:14
+   │
+49 │         File outputFile = "output.txt"
+   │              ^^^^^^^^^^
+   │
+   = fix: rename the identifier to not start with 'output'
+
+warning[DisallowedDeclarationName]: declaration identifier 'result_int' ends with type name 'Int'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:50:13
+   │
+50 │         Int result_int = 42
+   │             ^^^^^^^^^^
+   │
+   = fix: rename the identifier to not include the type name
+
+warning[DisallowedDeclarationName]: declaration identifier 'resultString' ends with type name 'String'
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:51:16
+   │
+51 │         String resultString = "result"
+   │                ^^^^^^^^^^^^
+   │
+   = fix: rename the identifier to not include the type name
+
+note[Whitespace]: line contains trailing whitespace
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:55:2
+   │
+55 │ } 
+   │  ^
+   │
+   = fix: remove the trailing whitespace
+
+note[EndingNewline]: missing newline at the end of the file
+   ┌─ tests/lints/disallowed-declaration-name/source.wdl:55:2
+   │
+55 │ } 
+   │  ^ expected a newline to follow this
+   │
+   = fix: add a newline at the end of the file
+