fix: add ordering check to param meta matching #354

Nitish-bot · 2025-03-15T12:52:30Z

This pull request modifies an existing rule to wdl-lint.

Rule Name: MatchingParameterMeta

Closes #259, and now checks for the order of parameter_metadata comparing it to the order of inputs

I have:

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 (see
"keep a changelog" for more information).
Your commit messages follow the conventional commit style.
Your PR title follows 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.
You have run gauntlet --bless to ensure that there are no
unintended changes to the baseline configuration file (Gauntlet.toml).
You have run gauntlet --bless --arena to ensure that all of the
rules added/removed are now reflected in the baseline configuration file
(Arena.toml).

Nitish-bot · 2025-03-15T13:57:46Z

I get a warning across 19 files at the first line saying the same thing i.e.

unknown lint: `rust_2024_incompatible_pat`
the `rust_2024_incompatible_pat` lint is unstable
see issue #123076 <https://github.com/rust-lang/rust/issues/123076> for more information
`#[warn(unknown_lints)]` on by default

I also get one more error warning two files that I haven't even touched so I am assuming they existed before my PR. It's a "missing documentation for a function" warning in wdl-engine/src/backend.rs:710:5 in the recurse_fit_maximal_range function along with another under wdl-doc/src/docs_tree.rs:99:9

Besides that, there are some changes to arena.toml after running the test and I went through a couple of them to find that the ordering of parameters was indeed not consistent.

Finally, while working on this PR I looked at some other lint rules and such to understand the codebase better and I think there is a certain inconsistency in the source.wdl files, some explain through comments, others assign strings and use those to convey the desired effect and some don't do any at all. I was wondering if this is a priority for you guys and if so I could go ahead and do a small review PR of all the rules and messages as a newcomer to see if they are easy to understand.

This is my first PR here so I am looking forward to learning how you guys do things around here and I was wondering if there is a separate communication channel where we can connect better.

wdl-lint/src/rules/matching_parameter_meta.rs

a-frantz · 2025-03-17T19:45:30Z

I get a warning across 19 files at the first line saying the same thing i.e.
unknown lint: `rust_2024_incompatible_pat`
the `rust_2024_incompatible_pat` lint is unstable
see issue #123076 <https://github.com/rust-lang/rust/issues/123076> for more information
`#[warn(unknown_lints)]` on by default
I also get one more error warning two files that I haven't even touched so I am assuming they existed before my PR. It's a "missing documentation for a function" warning in wdl-engine/src/backend.rs:710:5 in the recurse_fit_maximal_range function along with another under wdl-doc/src/docs_tree.rs:99:9

I believe that all these errors would be resolved by rebasing onto main. Your fork is out of sync with our latest changes.

Besides that, there are some changes to arena.toml after running the test and I went through a couple of them to find that the ordering of parameters was indeed not consistent.

Yes, the Arena.toml changes look correct to me!

peterhuene · 2025-03-17T19:55:20Z

Hi @Nitish-bot. I merged in a very large refactoring just a few minutes ago that is causing a conflict with your PR.

The CHANGELOG.md conflict should be pretty easy to resolve, but if you need any assistance with resolving conflicts in matching_parameter_meta.rs, please notify me on GitHub or Slack and I'll help out.

…aram_meta_ordering

Co-authored-by: Andrew Frantz <[email protected]>

Nitish-bot · 2025-03-17T23:43:08Z

Hey @a-frantz @peterhuene, I think I have addressed all concerns here and the PR is ready to be merged.

a-frantz · 2025-03-18T11:50:38Z

wdl-lint/CHANGELOG.md

@@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

 ## Unreleased
+### Fixed
+
+* Fixed the `MatchingParameterMeta` rule to also check if the order of inputs matches parameter metadata ([#354](https://github.com/stjude-rust-labs/wdl/pull/354))


Please move this to the existing ### Fixed header under ## Unreleased

You mean the previous ### Fixed pertaining to MatchingParameteMeta?

edit: nvm I got it now

here - https://github.com/Nitish-bot/wdl/blob/param_meta_ordering/wdl-lint/CHANGELOG.md?plain=1#L24-L26

a-frantz · 2025-03-18T12:06:16Z

wdl-lint/tests/lints/struct-matching-param-meta/source.wdl

@@ -1,5 +1,6 @@
 ## This is a test for checking for missing and extraneous entries
-## in a `parameter_meta` section for a struct.
+## in a `parameter_meta` section, and for ensuring that
+## the order is the same as `input` section.


Can you add a new struct definition with an out of order parameter_meta to this test?

a-frantz · 2025-03-18T12:07:03Z

wdl-lint/tests/lints/matching-param-meta/source.wdl

+    output {}
+}
+
+workflow test {


Can you add a task test case?

a-frantz · 2025-03-18T12:14:07Z

wdl-lint/src/rules/matching_parameter_meta.rs

+    .with_fix(format!(
+        "order the parameter metadata as:\n{}",
+        expected_order
+    ))


I don't love this, but I'm not sure what a straightforward improvement would be.
My issue is that the "fix" here can't be copy and pasted, as it just outputs the order of the keys without their associated values. However, it would be difficult to properly format the values for output.

We've discussed vague plans of making wdl-lint dependent on wdl-format so that we can supply better fix messages, and I think we should probably wait for that to come to fruition instead of writing some complicated logic for this.

So for now, I'm fine with this fix message but its not ideal.

a-frantz · 2025-03-18T12:17:31Z

@Nitish-bot two other test cases I would like to see in addition to what I just commented.

an "out of order" input section with a matching parameter_meta (i.e. the parameter_meta should be in the same order as the input section, but the input section should trigger an InputSorting diagnostic)
an "out of order" input section with an "out of order" parameter_meta (i.e. the parameter_meta should be in a different order than the input section, and the input section should trigger an InputSorting diagnostic)

Nitish-bot · 2025-03-19T08:45:35Z

@a-frantz

an "out of order" input section with an "out of order" parameter_meta (i.e. the parameter_meta should be in a different order than the input section, and the input section should trigger an InputSorting diagnostic)

do you mean that it should only trigger an InputSorting diagnostic? because it would also trigger the MatchingParam Diagnostic along with it

edit (19/03/25 11 PM +5:30 GMT): I have since updated it to my understanding please check it out

…aram_meta_ordering

Nitish-bot · 2025-03-24T16:23:16Z

@claymcleod the revisions have been made, it is indeed awaiting review

wdl-lint/tests/lints/input-not-sorted/source.wdl

wdl-lint/tests/lints/matching-param-meta/source.wdl

Co-authored-by: Andrew Frantz <[email protected]>

wdl-lint/src/rules/matching_parameter_meta.rs

wdl-lint/tests/lints/matching-param-meta/source.wdl

…aram_meta_ordering

Nitish-bot · 2025-03-30T18:00:50Z

Aside from the review, I moved the struct definition test into struct-matching-param-meta test and removed the RuntimeKeys lint directive since it is deprecated and no longer throws a warning in WDL 1.2.
I can probably write a simple bash script to remove it from all test files that use 1.2 but that belongs in a PR of it's own.

wdl-lint/tests/lints/matching-param-meta/source.wdl

a-frantz

I have the one concern about the interaction between InputSorting and MatchingParameterMeta, but otherwise this looks good to me 👍

a-frantz · 2025-03-31T14:19:01Z

wdl-lint/tests/lints/input-and-parameter-meta-sorting/source.errors

+note[MatchingParameterMeta]: parameter metadata in task `input_sorting_test_2` is out of order
+   ┌─ tests/lints/input-and-parameter-meta-sorting/source.wdl:39:5
+   │
+39 │     parameter_meta {
+   │     ^^^^^^^^^^^^^^ parameter metadata must be in the same order as inputs
+   │
+   = fix: order the parameter metadata as:
+     w
+     p
+     t
+     q
+     b
+
+note[InputSorting]: input not sorted
+   ┌─ tests/lints/input-and-parameter-meta-sorting/source.wdl:47:5
+   │
+47 │     input {
+   │     ^^^^^ input section must be sorted
+   │
+   = fix: sort input statements as: 
+     File t
+     File b
+     Directory w
+     Array[String]+ p
+     Array[String]+ q


Making a new comment with the same concern I raised in an earlier review.

These diagnostics conflict with each other, and it could be confusing to users. How should we handle this case? @adthrasher

Ideally, I think the MatchingParameterMeta rule would output the "correct" order that InputSorting uses. Or it would have a flag on the task/workflow that indicates the input is not properly sorted and would update the diagnostic accordingly. That's not a straightforward change at this point, so we may have to live with it. To me, the confusion stems from parameter_meta preceding input in the source. If the InputSorting diagnostic was first, then I think it's potentially less confusing for users.

A simple approach would be to update the "fix" message to something like: "fix: Based on current the input block, order the parameter metadata as:".

A simple approach would be to update the "fix" message to something like: "fix: Based on current the input block, order the parameter metadata as:".

I think this seems a decent bandaid for the situation. With an updated fix message, I'd be ok merging this if it's accompanied by a new issue with these details. We can come back and tinker at it later.

A potential fix is to rely on wdl-format to get the right order. However wdl-format currently doesn't do this sort, so that would need to be implemented first.

I made a suggested change above, please double-check the wording. Also the issue is:
#379

wdl-lint/src/rules/matching_parameter_meta.rs

a-frantz

pending the fix update: looks great. This fix is overdue! Happy to get it merged and into the next Sprocket release 🚀

Nitish-bot · 2025-03-31T16:55:12Z

I have made the required change.

A potential fix is to rely on wdl-format to get the right order. However wdl-format currently doesn't do this sort, so that would need to be implemented first.

Also you mentioned handling this in wdl-format, I would love to implement this in another PR if this is a priority.

a-frantz · 2025-03-31T16:59:09Z

I have made the required change.

A potential fix is to rely on wdl-format to get the right order. However wdl-format currently doesn't do this sort, so that would need to be implemented first.

Also you mentioned handling this in wdl-format, I would love to implement this in another PR if this is a priority.

I will write up some more details of what I had in mind in a new issue.

You also need to rebless the tests

Nitish-bot · 2025-03-31T17:03:01Z

alright I am running the tests, in the meanwhile I found out andrew-t already created an issue #379 wherein he mentions that this would require some discussion within the team. I can open a slack thread to discuss this better.

a-frantz · 2025-03-31T17:15:28Z

issue created: #380

Nitish-bot · 2025-04-01T07:04:11Z

Alright this should be ready to get merged

Add ordering check to param meta matching

11bde78

Nitish-bot changed the title ~~Add ordering check to param meta matching~~ fix: add ordering check to param meta matching Mar 15, 2025

Use only intersection of expected and actual inputs

d675beb

Nitish-bot marked this pull request as ready for review March 15, 2025 13:58

minor fix

684accd

a-frantz reviewed Mar 17, 2025

View reviewed changes

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

Nitish-bot and others added 3 commits March 18, 2025 03:10

Merge branch 'main' of https://github.com/stjude-rust-labs/wdl into p…

ce06653

…aram_meta_ordering

fix: regression after merge

41727de

Update wdl-lint/src/rules/matching_parameter_meta.rs

fa7f4ba

Co-authored-by: Andrew Frantz <[email protected]>

a-frantz reviewed Mar 18, 2025

View reviewed changes

Nitish-bot added 2 commits March 18, 2025 21:27

add: task and struct test

5330664

Undo incorrect change

8922bde

Nitish-bot added 2 commits March 19, 2025 14:17

Added input sorting test

9f62e4f

Merge branch 'main' of https://github.com/stjude-rust-labs/wdl into p…

ba3c768

…aram_meta_ordering

claymcleod assigned Nitish-bot Mar 19, 2025

claymcleod added S-awaiting-pass-CI State: awaiting the CI to pass S-awaiting-revisions State: awaiting revisions based on code review feedback and removed S-awaiting-pass-CI State: awaiting the CI to pass labels Mar 19, 2025

claymcleod requested a review from a-frantz March 23, 2025 20:33

claymcleod added S-needs-review State: awaiting initial or additional review and removed S-awaiting-revisions State: awaiting revisions based on code review feedback labels Mar 23, 2025

a-frantz reviewed Mar 24, 2025

View reviewed changes

wdl-lint/tests/lints/input-not-sorted/source.wdl Outdated Show resolved Hide resolved

wdl-lint/tests/lints/matching-param-meta/source.wdl Outdated Show resolved Hide resolved

Update wdl-lint/tests/lints/matching-param-meta/source.wdl

e312cf9

Co-authored-by: Andrew Frantz <[email protected]>

adthrasher reviewed Mar 28, 2025

View reviewed changes

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

adthrasher reviewed Mar 28, 2025

View reviewed changes

wdl-lint/tests/lints/matching-param-meta/source.wdl Outdated Show resolved Hide resolved

adthrasher reviewed Mar 28, 2025

View reviewed changes

wdl-lint/tests/lints/matching-param-meta/source.wdl Outdated Show resolved Hide resolved

adthrasher reviewed Mar 28, 2025

View reviewed changes

wdl-lint/tests/lints/matching-param-meta/source.wdl Outdated Show resolved Hide resolved

adthrasher removed the S-needs-review State: awaiting initial or additional review label Mar 28, 2025

Nitish-bot added 2 commits March 30, 2025 22:27

Merge branch 'main' of https://github.com/stjude-rust-labs/wdl into p…

06117e3

…aram_meta_ordering

chore: reviewed changes

f2cbf8a

Nitish-bot commented Mar 30, 2025

View reviewed changes

wdl-lint/tests/lints/matching-param-meta/source.wdl Show resolved Hide resolved

Nitish-bot added 3 commits March 31, 2025 17:45

chore: fmt

6a19e88

chore: rework matching param test source.wdl

d4c3787

undo changes to input-not-sorted

f946098

a-frantz reviewed Mar 31, 2025

View reviewed changes

a-frantz requested review from adthrasher, a-frantz, peterhuene and claymcleod March 31, 2025 14:29

a-frantz added S-needs-review State: awaiting initial or additional review and removed S-awaiting-revisions State: awaiting revisions based on code review feedback labels Mar 31, 2025

adthrasher reviewed Mar 31, 2025

View reviewed changes

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

a-frantz approved these changes Mar 31, 2025

View reviewed changes

chore: update fix message to be clearer

372ea3e

chore: reBLESS

3acc024

adthrasher approved these changes Apr 1, 2025

View reviewed changes

adthrasher merged commit e0d636e into stjude-rust-labs:main Apr 1, 2025
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add ordering check to param meta matching #354

fix: add ordering check to param meta matching #354

Nitish-bot commented Mar 15, 2025 •

edited by claymcleod

Loading

Nitish-bot commented Mar 15, 2025 •

edited

Loading

a-frantz commented Mar 17, 2025

peterhuene commented Mar 17, 2025

Nitish-bot commented Mar 17, 2025

a-frantz Mar 18, 2025

Nitish-bot Mar 18, 2025 •

edited

Loading

a-frantz Mar 18, 2025

a-frantz Mar 18, 2025

a-frantz Mar 18, 2025

a-frantz Mar 18, 2025

a-frantz commented Mar 18, 2025

Nitish-bot commented Mar 19, 2025 •

edited

Loading

Nitish-bot commented Mar 24, 2025

Nitish-bot commented Mar 30, 2025 •

edited

Loading

a-frantz left a comment

a-frantz Mar 31, 2025

adthrasher Mar 31, 2025

adthrasher Mar 31, 2025 •

edited

Loading

a-frantz Mar 31, 2025

adthrasher Mar 31, 2025

a-frantz left a comment

Nitish-bot commented Mar 31, 2025

a-frantz commented Mar 31, 2025

Nitish-bot commented Mar 31, 2025

a-frantz commented Mar 31, 2025

Nitish-bot commented Apr 1, 2025

fix: add ordering check to param meta matching #354

fix: add ordering check to param meta matching #354

Conversation

Nitish-bot commented Mar 15, 2025 • edited by claymcleod Loading

Nitish-bot commented Mar 15, 2025 • edited Loading

a-frantz commented Mar 17, 2025

peterhuene commented Mar 17, 2025

Nitish-bot commented Mar 17, 2025

Choose a reason for hiding this comment

Nitish-bot Mar 18, 2025 • edited Loading

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

a-frantz commented Mar 18, 2025

Nitish-bot commented Mar 19, 2025 • edited Loading

Nitish-bot commented Mar 24, 2025

Nitish-bot commented Mar 30, 2025 • edited Loading

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

adthrasher Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-frantz left a comment

Choose a reason for hiding this comment

Nitish-bot commented Mar 31, 2025

a-frantz commented Mar 31, 2025

Nitish-bot commented Mar 31, 2025

a-frantz commented Mar 31, 2025

Nitish-bot commented Apr 1, 2025

Nitish-bot commented Mar 15, 2025 •

edited by claymcleod

Loading

Nitish-bot commented Mar 15, 2025 •

edited

Loading

Nitish-bot Mar 18, 2025 •

edited

Loading

Nitish-bot commented Mar 19, 2025 •

edited

Loading

Nitish-bot commented Mar 30, 2025 •

edited

Loading

adthrasher Mar 31, 2025 •

edited

Loading