Add PreferStructOverMap lint rule (Issue #717)#740
Closed
Abhijit1018 wants to merge 1 commit intostjude-rust-labs:mainfrom
Closed
Add PreferStructOverMap lint rule (Issue #717)#740Abhijit1018 wants to merge 1 commit intostjude-rust-labs:mainfrom
Abhijit1018 wants to merge 1 commit intostjude-rust-labs:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new PreferStructOverMap lint rule to wdl-lint intended to discourage Map[String, *] declarations in favor of struct usage for clearer, more type-safe WDL.
Changes:
- Introduces
PreferStructOverMapRulewith recursive type traversal and WDL version gating (≥ 1.1). - Registers and re-exports the new rule module.
- Adds the rule to the default rule list in
wdl-lint.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crates/wdl-lint/src/rules/prefer_struct_over_map.rs |
Implements the new rule, diagnostics, type traversal, and visitor hooks. |
crates/wdl-lint/src/rules.rs |
Registers the new rule module and exports it. |
crates/wdl-lint/src/lib.rs |
Adds the rule to the constructed rule list returned by rules(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+115
to
+117
| Type::Map(_) => { | ||
| // Already checked above, no need to recurse into value type | ||
| // as that would cause duplicate diagnostics |
| Box::<rules::OutputNameRule>::default(), | ||
| Box::new(rules::DeclarationNameRule::new(config)), | ||
| Box::<rules::RedundantNone>::default(), | ||
| Box::new(rules::PreferStructOverMapRule::new(false)), |
Comment on lines
+188
to
+227
| fn bound_decl(&mut self, diagnostics: &mut Diagnostics, reason: VisitReason, decl: &BoundDecl) { | ||
| if reason == VisitReason::Exit { | ||
| return; | ||
| } | ||
|
|
||
| // Check version - only apply to WDL >= 1.1 | ||
| if let Some(SupportedVersion::V1(version)) = self.version { | ||
| if version < V1::One { | ||
| return; | ||
| } | ||
| } else { | ||
| return; | ||
| } | ||
|
|
||
| let ty = decl.ty(); | ||
| self.check_type(diagnostics, &ty, &self.exceptable_nodes()); | ||
| } | ||
|
|
||
| fn unbound_decl( | ||
| &mut self, | ||
| diagnostics: &mut Diagnostics, | ||
| reason: VisitReason, | ||
| decl: &UnboundDecl, | ||
| ) { | ||
| if reason == VisitReason::Exit { | ||
| return; | ||
| } | ||
|
|
||
| // Check version - only apply to WDL >= 1.1 | ||
| if let Some(SupportedVersion::V1(version)) = self.version { | ||
| if version < V1::One { | ||
| return; | ||
| } | ||
| } else { | ||
| return; | ||
| } | ||
|
|
||
| let ty = decl.ty(); | ||
| self.check_type(diagnostics, &ty, &self.exceptable_nodes()); | ||
| } |
| //! by flagging Map[String, *] types in declarations where a Struct would provide | ||
| //! clearer semantics and better validation. | ||
|
|
||
| use std::fmt::Debug; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces the
PreferStructOverMaplint rule to the WDL pedantic suite. This rule flags uses ofMap[String, *]in declarations, encouraging developers to use Structs for better type safety, validation, and self-documenting code.📋 Rule Overview
Property | Details -- | -- Rule ID | PreferStructOverMap Tags | Clarity, Type Safety WDL Version | ≥ 1.1 Scope | Inputs, Outputs, and Private Declarations (Tasks & Workflows) Default Level | Pedantic (Warning)💡 Rationale
Using a
Map[String, *]for structured data is often an anti-pattern in WDL. While flexible, it lacks the explicit schema validation provided by astruct.Before (Fragile): Relies on runtime key-checking and lacks clear documentation of expected fields.
After (Robust): Provides a clear interface, compile-time (or lint-time) validation, and better IDE support.
🛠 Implementation Details
The rule follows the standard Visitor pattern used throughout the
wdl-lintcrate.Key Features:
Recursive Detection: Deeply inspects nested types (e.g.,
Array[Map[String, Int]]).Key Filtering: Specifically targets
Stringkeys only;Map[Int, *]remains valid.Version Gated: Automatically disables itself for WDL version < 1.1.
Suppression Support: Compatible with
#@ except: PreferStructOverMap.📂 File Changes
crates/wdl-lint/src/rules/prefer_struct_over_map.rsCore logic for type traversal and diagnostic emission.
crates/wdl-lint/src/rules.rsRegistered the new module.
crates/wdl-lint/src/lib.rsIntegrated the rule into the primary lint engine.
📝 Examples
❌ Bad
✅ Good
🔗 Context
Fixes: New
wdl-lintrule:PreferStructOverMap#717