Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Incorrect type-mismatch error #21

Closed
theAeon opened this issue Sep 24, 2024 · 6 comments · Fixed by stjude-rust-labs/wdl#199
Closed

Incorrect type-mismatch error #21

theAeon opened this issue Sep 24, 2024 · 6 comments · Fixed by stjude-rust-labs/wdl#199
Assignees
@peterhuene

Comments

@theAeon
Copy link

theAeon commented Sep 24, 2024

It would seem as of version 0.7.0, sprocket is reporting that a coercion from Array[File] to Array[String] is a type mismatch.

Cromwell's implementation of the 1.0 spec disagrees and to my understanding is correct to do so. Files can be coerced to strings, so it follows that arrays of files can be coerced to arrays of strings.
@peterhuene
Copy link
Collaborator

peterhuene commented Sep 24, 2024 •
edited
Loading

Hi @theAeon. Thank you very much for reporting this issue!

The type coercion part of the WDL specification contains a table of valid type coercions.

It defines a coercion from String to File, but not File to String. In fact, there is an example in that section (coercion_fail.wdl, albeit with some syntax errors) that shows it is expected to be an error to coerce a File to a String.

That said, I do agree that if there were a coercion from File to String, then Array[File] should coerce to Array[String] based on the coercion Array[X] -> Array[Y] (where X is coercible to Y).

However, I did just find a counterexample where it is expecting File to coerce as an argument to the sub function named change_extension_task.wdl, where it is expecting a coercion from File to String.

I believe this warrants an issue on the WDL spec for clarification and to explicitly call out a coercion from File to String. If the spec is corrected, then this will be very easy to fix in sprocket.

@peterhuene
Copy link
Collaborator

I'll file that issue with the spec and link it back to this issue.

@peterhuene
Copy link
Collaborator

I've filed openwdl/wdl#685 to track the spec issue.

@theAeon
Copy link
Author

theAeon commented Sep 25, 2024

I've filed openwdl/wdl#685 to track the spec issue.

Interesting, so the coercion is explicitly defined in spec 1.0.

@peterhuene
Copy link
Collaborator

@theAeon it appears that way! That makes me suspect it was an unintentional omission as to why it's not supported in 1.1 and 1.2; removing a coercion would certainly be a breaking change!

I'm going to preemptively fix this in sprocket so that we allow the coercion from 1.x versions, under the assumption it is an error in the spec.

@peterhuene
Copy link
Collaborator

I have a fix for this included with my "finish static analysis" work (i.e. fully analyze workflows). I had to include it there as otherwise we can't pass a check on our own workflows repo.

peterhuene added a commit to peterhuene/wdl that referenced this issue Sep 27, 2024
@peterhuene
feat: implement static analysis for workflows.
ea01797 
This commit implements full static analysis for workflows, including type
checking.

To perform analysis on workflows, a `WorkflowEvaluationGraph` was implemented
that will return a topologically-sorted set of nodes for evaluating the
contents of a workflow in the correct order.

This also includes several fixes required to remove incorrect diagnostics from
checking the `workflows` repository:

* Treat a coercion to `T?` for a function argument of type `T` as a preference
  over any other coercion.
* Fix the signature of `select_any` such that it is monomorphic.
* Only consider signatures in overload resolution that have sufficient
  arguments.
* Allow coercion from `File` and `Directory` to `String`.
* Allow non-empty array literals to coerce to either empty or non-empty.
* Fix element type calculations for `Array` and `Map` so that `[a, b]`
  successfully calculates when `a` is coercible to `b`.
* Fix `if` expression type calculation such that `if (x) then a else b` works
  when `a` is coercible to `b`.
* Ensure that only equality/inequality expressions are supported on `File` and
  `Directory` now that there is a coercion to `String`.
* Allow index expressions on `Map`.

Included with these changes is a refactoring to place all of analysis
diagnostics in `diagnostics.rs`.

Fixes stjude-rust-labs#197.
Fixes stjude-rust-labs#196.
Fixes stjude-rust-labs#167.
Fixes stjude-rust-labs/sprocket#21.
@peterhuene peterhuene mentioned this issue Sep 27, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@peterhuene peterhuene

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: implement static analysis for workflows. peterhuene/wdl
2 participants
@peterhuene @theAeon