feat: implement `DisallowedDeclarationName` rule #343

Gyan-max · 2025-03-08T05:20:13Z

This pull request adds a new rule to wdl-lint.

Rule Name: DisallowedDeclarationNameRule

This rule checks for declaration names that contain their type (like File fileInput) which is considered poor style as it adds unnecessary verbosity and creates redundancy since the type is already declared. The rule identifies declarations like File fileInput, Int my_int, and Array[String] stringArray as problematic.

Before submitting this PR, please make sure:

You have added a few sentences describing the PR here.
You have added yourself or the appropriate individual as the assignee.
You have added at least one relevant code reviewer to the PR.
Your code builds clean without any errors or warnings.
You have added an entry to the relevant CHANGELOG.md.
Your commit messages follow the conventional commit style.

Rule specific checks:

You have added the rule as an entry within RULES.md.
You have added the rule to the rules() function in wdl-lint/src/lib.rs.
You have added a test case in wdl-lint/tests/lints that covers every
possible diagnostic emitted for the rule within the file where the rule
is implemented.
If you have implemented a new Visitor callback, you have also
overridden that callback method for the special Validator
(wdl-ast/src/validation.rs) and LintVisitor
(wdl-lint/src/visitor.rs) visitors.
You have run gauntlet --refresh to ensure that there are no
unintended changes to the baseline configuration file (Gauntlet.toml).
You have run gauntlet --refresh --arena to ensure that all of the
rules added/removed are now reflected in the baseline configuration file
(Arena.toml).

Fixes #215

wdl-lint/src/lib.rs

wdl-lint/src/rules/disallowed_declaration_name.rs

a-frantz · 2025-03-08T16:22:25Z

follow the PR template please. Re-write your PR description according to https://github.com/stjude-rust-labs/wdl/blob/main/.github/pull_request_template.md?plain=1

…ct types in declaration names

Gyan-max · 2025-03-09T07:47:46Z

@a-frantz I've addressed all your feedback:

Minimized the diff in lib.rs
Improved the type detection logic to handle various type structures
Updated the diagnostic messages to be more helpful
Fixed the implementation to properly follow the project's coding standards

All tests are now passing. Let me know if you'd like me to make any additional improvements!

a-frantz · 2025-03-09T13:05:25Z

I think your force push went wrong. I still see the old code I reviewed earlier.

Gyan-max · 2025-03-09T16:05:33Z

I think your force push went wrong. I still see the old code I reviewed earlier.

@a-frantz I just force-pushed the updated implementation again. You should now be able to see the changes that address all your feedback. Let me know if you have any further questions or if there are additional improvements needed.

a-frantz · 2025-03-09T16:44:12Z

I think your force push went wrong. I still see the old code I reviewed earlier.

@a-frantz I just force-pushed the updated implementation again. You should now be able to see the changes that address all your feedback. Let me know if you have any further questions or if there are additional improvements needed.

still seeing the old code. I think there's something wrong with your git commands. Try again, and then check the "files changed" tab after to make sure your pushes work.

a-frantz · 2025-03-09T16:45:19Z

Also, your PR description still doesn't have the proper checklist items. Please copy and paste from the template I linked earlier

Gyan-max · 2025-03-09T17:14:04Z

I think your force push went wrong. I still see the old code I reviewed earlier.

@a-frantz I just force-pushed the updated implementation again. You should now be able to see the changes that address all your feedback. Let me know if you have any further questions or if there are additional improvements needed.

still seeing the old code. I think there's something wrong with your git commands. Try again, and then check the "files changed" tab after to make sure your pushes work.

@a-frantz I've made another push with a visible marker comment at the top of the file:

//! FIXING PUSH ISSUE - THIS COMMENT SHOULD BE VISIBLE IN THE PR

Please check the "Files changed" tab to verify that you can now see the updated code. The changes should now include:

Improved diagnostic messages that include both declaration name and type
Better type detection that works case-insensitively
Fixed SyntaxKind values for exceptable_nodes
All tests passing

Let me know if you still can't see the updated code.

Gyan-max · 2025-03-09T17:16:51Z

@a-frantz I've updated the PR description to use the proper template with checklist items as requested. Also, I've made another push with visible changes to the rule file - you should now be able to see the updated implementation in the "Files changed" tab.

a-frantz

Maybe there was some confusion earlier. I see your push worked, but there are still some issues that I highlighted before that weren't resolved. I unresolved the relevant github threads.

wdl-doc/src/lib.rs

wdl-lint/src/lib.rs

wdl-lint/src/rules/disallowed_declaration_name.rs

Gyan-max · 2025-03-09T18:47:35Z

@a-frantz I've addressed all your feedback:

Reverted changes to wdl-doc/src/lib.rs - this file now has no changes
Minimized changes to wdl-lint/src/lib.rs - only adding the DisallowedDeclarationNameRule and changing the for loop
Simplified the type checking in disallowed_declaration_name.rs:
- Removed the redundant common_types array and associated checks
- Kept the direct type comparison approach

I've pushed all these changes. Please let me know if there's anything else I need to fix.

…raction

wdl-lint/src/rules/disallowed_declaration_name.rs

Gyan-max · 2025-03-12T14:52:13Z

@a-frantz sorry to bother you again
I installed shellcheck on my machine and re-run all the tests. All passed.
and i also updated test outputs so please review it once more and do let me if any further changes are needed.
ThankYou

claymcleod · 2025-03-19T16:13:56Z

@a-frantz, this is ready for rereview.

a-frantz · 2025-03-19T16:23:24Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+            SyntaxKind::InputSectionNode,
+            SyntaxKind::OutputSectionNode,
+            SyntaxKind::BoundDeclNode,
+            SyntaxKind::UnboundDeclNode,


context: We try to keep each list of "exceptable nodes" minimal, preferring the most specific node when excepting two different nodes would lead to the same behavior.

That being said, this will need to be expanded to include task and workflow definition nodes so that all private declarations of a task or workflow can be "excepted" at once. That would make this list one of our longer lists of exceptable nodes, but each node would cover a different set of declarations.

Please add those two nodes to this list, and then I think we'll have all our bases covered.

task and workflow definitions still need to be added here

a-frantz · 2025-03-19T16:25:39Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+    let name = decl.name();
+    let name_str = name.as_str();


nit: these don't get used till line 172, and I prefer to keep declarations close to where they're used. Can you move these down? It reduces the need to scroll up and down looking for where something is used/defined.

wdl-lint/src/rules/disallowed_declaration_name.rs

Gyan-max · 2025-03-20T16:33:18Z

Hi @a-frantz,
Thanks for the feedback! I've addressed all the requested changes:
Added TaskNode and WorkflowNode to the exceptable_nodes list to allow excepting all private declarations of a task or workflow at once.
Moved the declaration of name and name_str variables closer to where they're used in the code to improve readability.
Added Dir as an additional check for the Directory type to catch both naming variants.
Added Int as an additional check for the Integer type, with a comment explaining that Integer is shortened to Int in WDL.
Removed the blank lines within the match statement to make the code more concise as per the style guidelines.
All changes have been committed and pushed. Let me know if there's anything else that needs addressing!

a-frantz

Did you reBLESS the tests and Arena? Or did the latest round of tweaks not result in any changes?

wdl-lint/src/rules/disallowed_declaration_name.rs

…name rule

Gyan-max · 2025-03-20T17:00:29Z

I've updated the implementation with the suggested improvements:

Fixed the Integer type handling - I'm now checking for "Integer" explicitly (as primitive_type.to_string() already returns "Int").
Implemented a more generic approach for short type names (3 characters or fewer) using word splitting, which covers both "Int" and "Dir" cases.
Made the code cleaner with better comments to explain the rationale and implementation.

This approach avoids hardcoding specific types and should be more maintainable going forward.

a-frantz · 2025-03-20T17:02:29Z

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 prefixes or suffixes.                             |


a-frantz · 2025-03-20T17:04:04Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+         verbose and often redundant. For example, use 'counter' instead of 'counterInt' or \
+         'is_active' instead of 'isActiveBoolean'."


Suggested change

verbose and often redundant. For example, use 'counter' instead of 'counterInt' or \

'is_active' instead of 'isActiveBoolean'."

verbose and often redundant. For example, use 'counter' instead of 'counter_int' or \

'is_active' instead of 'is_active_boolean'."

a-frantz · 2025-03-20T17:05:28Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+        }
+    }
+
+    // Get declaration name after type checking (moving closer to usage)


Suggested change

// Get declaration name after type checking (moving closer to usage)

a-frantz · 2025-03-20T17:06:17Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+    let name = decl.name();
+    let name_str = name.as_str();


Suggested change

let name = decl.name();

let name_str = name.as_str();

let name = decl.name().as_str();

a-frantz · 2025-03-20T17:08:24Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+            let name_lower = name_str.to_lowercase();
+            if name_lower.ends_with(&type_lower) {


Suggested change

let name_lower = name_str.to_lowercase();

if name_lower.ends_with(&type_lower) {

if name.to_lowercase().ends_with(&type_lower) {

a-frantz · 2025-03-20T17:09:56Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+        .collect()
+}
+
+#[cfg(test)]


The tests you've written here are essentially the same as the tests checked by source.wdl and source.errors, so I think it's ok to just delete all these.

a-frantz · 2025-03-20T17:11:07Z

wdl-lint/tests/lints/deprecated-object/source.errors

Can you except DisallowedDeclarationName from this source.wdl to prevent these errors?

a-frantz · 2025-03-20T17:11:58Z

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

please fix all of these

a-frantz · 2025-03-20T17:13:36Z

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

+warning[DisallowedDeclarationName]: declaration identifier 'int' ends with type name 'Int'
+   ┌─ tests/lints/disallowed-input-name/source.wdl:24:13
+   │
+24 │         Int int = 1  # This is OK


I think we just delete this line from source.wdl to prevent this diagnostic. It's not a good example anymore

…ame rule

Gyan-max · 2025-03-20T17:39:19Z

@a-frantz All done, please review it once and let me know if any further improvement is needed.

a-frantz · 2025-03-20T17:45:31Z

@Gyan-max please fix everything reported by CI. But this is really close! Nice work

Gyan-max · 2025-03-20T18:12:28Z

@Gyan-max please fix everything reported by CI. But this is really close! Nice work

On it!

Gyan-max · 2025-03-20T18:26:21Z

@a-frantz All tests are now passing with both cargo test and cargo clippy, and I've blessed the test baselines using BLESS=1.

a-frantz

mostly nits about unhelpful comments. We like to avoid comments that are describing something "obvious", although that is kind of subjective 🤷

a-frantz · 2025-03-20T18:29:33Z

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

You can except NonmatchingOutput and InputSorting, but please correct the rest of the diagnostics reported here that aren't related to the new lint.

a-frantz · 2025-03-20T18:30:26Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+            SyntaxKind::InputSectionNode,
+            SyntaxKind::OutputSectionNode,
+            SyntaxKind::BoundDeclNode,
+            SyntaxKind::UnboundDeclNode,


task and workflow definitions still need to be added here

a-frantz · 2025-03-20T18:35:06Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+    // Get the declaration name
+    let binding = decl.name();
+    let name = binding.text();
+
+    // Get the declaration type
+    let ty = decl.ty();


Suggested change

// Get the declaration name

let binding = decl.name();

let name = binding.text();

// Get the declaration type

let ty = decl.ty();

let binding = decl.name();

let name = binding.text();

let ty = decl.ty();

a-frantz · 2025-03-20T18:35:38Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+                    // Add the primitive type name
+                    type_names.insert(primitive_type.to_string());
+                    // Also check for "Bool"
+                    type_names.insert("Bool".to_string());


Suggested change

// Add the primitive type name

type_names.insert(primitive_type.to_string());

// Also check for "Bool"

type_names.insert("Bool".to_string());

type_names.insert(primitive_type.to_string());

type_names.insert("Bool".to_string());

a-frantz · 2025-03-20T18:36:46Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+        }
+    }
+
+    // Get the element for diagnostic reporting


Suggested change

// Get the element for diagnostic reporting

a-frantz · 2025-03-20T18:36:58Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+        Decl::Unbound(d) => SyntaxElement::from(d.inner().clone()),
+    };
+
+    // Check if the declaration name ends with one of the type names


Suggested change

// Check if the declaration name ends with one of the type names

a-frantz · 2025-03-20T18:37:35Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+            // Check if the short type name appears as the last word
+            if let Some(last_word) = words.last() {
+                if last_word.to_lowercase() == type_lower {
+                    let diagnostic = decl_identifier_with_type(decl.name().span(), name, type_name);


Suggested change

let diagnostic = decl_identifier_with_type(decl.name().span(), name, type_name);

let diagnostic = decl_identifier_with_type(binding.span(), name, type_name);

a-frantz · 2025-03-20T18:37:43Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+        // Special handling for short type names (3 characters or less)
+        // These require word-based checks to avoid false positives
+        if type_lower.len() <= 3 {
+            // Split the identifier into words


Suggested change

// Split the identifier into words

… clean up code

Gyan-max · 2025-03-20T19:11:24Z

@a-frantz I addressed all your feedbacks sorry for unnecessary bother and thankyou for your guidance.

a-frantz

I think we maybe want to leave File and String as ignored, but why don't you bless Arena with those enabled and I'll review what comes up, then we can decide what to do with those two types.

a-frantz · 2025-03-20T19:13:44Z

wdl-lint/src/rules/disallowed_declaration_name.rs

+#[derive(Debug, Default)]
+pub struct DisallowedDeclarationNameRule {
+    /// Tracks whether we're currently within an input or output declaration section
+    in_declaration_section: bool,


What's the point of this? It doesn't look like your using it, and I don't think we would care for this rule. Declarations everywhere (inputs, outputs, private decls) should all be handled the same.

I added this because while running the tests it was showing an error.

Can you share the error? I'm not following

just give me a minute, I'm running the tests

…g types

a-frantz · 2025-03-20T20:00:02Z

Arena.toml

Ok I think everything ending in string and file can be ignored. Please update the logic to allow those two type suffixes and then rebless everything

Implement DisallowedDeclarationName rule

15148cb

a-frantz reviewed Mar 8, 2025

View reviewed changes

wdl-lint/src/lib.rs Show resolved Hide resolved

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

Fix implementation of DisallowedDeclarationName rule to properly dete…

41148c2

…ct types in declaration names

Gyan-max force-pushed the disallowed-declaration-name branch from 6587d56 to 41148c2 Compare March 9, 2025 07:44

Gyan-max force-pushed the disallowed-declaration-name branch from beebcb5 to 41148c2 Compare March 9, 2025 16:04

a-frantz assigned a-frantz and Gyan-max and unassigned a-frantz Mar 9, 2025

Add visible marker in comments to verify push is working

5e3649f

Gyan-max force-pushed the disallowed-declaration-name branch from 111492e to 5e3649f Compare March 9, 2025 17:13

a-frantz requested changes Mar 9, 2025

View reviewed changes

Address reviewer feedback on DisallowedDeclarationName rule

87d9332

Gyan-max force-pushed the disallowed-declaration-name branch from 326aadf to 87d9332 Compare March 9, 2025 18:47

Update DisallowedDeclarationName rule to use AST methods for type ext…

77b3457

…raction

Gyan-max force-pushed the disallowed-declaration-name branch from 5e7a53f to 77b3457 Compare March 9, 2025 18:59

adthrasher reviewed Mar 10, 2025

View reviewed changes

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

adthrasher reviewed Mar 10, 2025

View reviewed changes

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

a-frantz reviewed Mar 10, 2025

View reviewed changes

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

a-frantz reviewed Mar 10, 2025

View reviewed changes

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

a-frantz reviewed Mar 10, 2025

View reviewed changes

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

a-frantz reviewed Mar 10, 2025

View reviewed changes

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

test: update expected test outputs with locally installed shellcheck

ca6c5f1

Merge branch 'main' into disallowed-declaration-name

ccc0a32

claymcleod changed the title ~~Implement DisallowedDeclarationName rule~~ feat: implement DisallowedDeclarationName Mar 19, 2025

claymcleod changed the title ~~feat: implement DisallowedDeclarationName~~ feat: implement DisallowedDeclarationName rule Mar 19, 2025

claymcleod added the S-awaiting-review State: awaiting initial or additional review label Mar 19, 2025

a-frantz reviewed Mar 19, 2025

View reviewed changes

Gyan-max and others added 2 commits March 20, 2025 10:04

Merge branch 'main' into disallowed-declaration-name

fd78b9a

refactor: improve disallowed_declaration_name rule

c81a990

a-frantz reviewed Mar 20, 2025

View reviewed changes

wdl-lint/src/rules/disallowed_declaration_name.rs Show resolved Hide resolved

wdl-lint/src/rules/disallowed_declaration_name.rs Outdated Show resolved Hide resolved

refactor: improve short type name handling in disallowed_declaration_…

fbfd5e1

…name rule

a-frantz reviewed Mar 20, 2025

View reviewed changes

fix: update linter tests and documentation for DisallowedDeclarationN…

5ba7346

…ame rule

fix: improve disallowed_declaration_name rule and tests

ec04dea

Gyan-max requested a review from a-frantz March 20, 2025 18:26

a-frantz reviewed Mar 20, 2025

View reviewed changes

fix: improve disallowed_declaration_name rule to handle all types and…

c9aa604

… clean up code

Gyan-max requested a review from a-frantz March 20, 2025 19:12

a-frantz reviewed Mar 20, 2025

View reviewed changes

fix: improve disallowed_declaration_name rule to check File and Strin…

6ceac38

…g types

Gyan-max requested a review from a-frantz March 20, 2025 19:42

a-frantz reviewed Mar 20, 2025

View reviewed changes

	\| `DisallowedDeclarationName` \| Naming \| Ensures that declaration names do not have type prefixes or suffixes. \|
	\| `DisallowedDeclarationName` \| Naming \| Ensures that declaration names do not have type suffixes. \|

		verbose and often redundant. For example, use 'counter' instead of 'counterInt' or \
		'is_active' instead of 'isActiveBoolean'."

	let name = decl.name();
	let name_str = name.as_str();
	let name = decl.name().as_str();

		let name_lower = name_str.to_lowercase();
		if name_lower.ends_with(&type_lower) {

	let name_lower = name_str.to_lowercase();
	if name_lower.ends_with(&type_lower) {
	if name.to_lowercase().ends_with(&type_lower) {

	let diagnostic = decl_identifier_with_type(decl.name().span(), name, type_name);
	let diagnostic = decl_identifier_with_type(binding.span(), name, type_name);

feat: implement DisallowedDeclarationName rule #343

Are you sure you want to change the base?

feat: implement DisallowedDeclarationName rule #343

Conversation

Gyan-max commented Mar 8, 2025 • edited Loading

a-frantz commented Mar 8, 2025

Gyan-max commented Mar 9, 2025

a-frantz commented Mar 9, 2025

Gyan-max commented Mar 9, 2025

a-frantz commented Mar 9, 2025

a-frantz commented Mar 9, 2025

Gyan-max commented Mar 9, 2025

Gyan-max commented Mar 9, 2025

a-frantz left a comment

Choose a reason for hiding this comment

Gyan-max commented Mar 9, 2025

Gyan-max commented Mar 12, 2025

claymcleod commented Mar 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gyan-max commented Mar 20, 2025

a-frantz left a comment

Choose a reason for hiding this comment

Gyan-max commented Mar 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gyan-max commented Mar 20, 2025

a-frantz commented Mar 20, 2025

Gyan-max commented Mar 20, 2025

Gyan-max commented Mar 20, 2025

a-frantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gyan-max commented Mar 20, 2025

a-frantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: implement `DisallowedDeclarationName` rule #343

feat: implement `DisallowedDeclarationName` rule #343

Gyan-max commented Mar 8, 2025 •

edited

Loading