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.

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

Non-empty qualifier missing on array literal type calculation #196

Closed
peterhuene opened this issue Sep 27, 2024 · 4 comments · Fixed by #199
Closed

Non-empty qualifier missing on array literal type calculation #196

peterhuene opened this issue Sep 27, 2024 · 4 comments · Fixed by #199
Assignees

Comments

@peterhuene
Copy link
Collaborator

In the type calculation for literal arrays, we should be adding a non-empty specifier to the array type when there is at least one element in the literal expression.

@a-frantz
Copy link
Member

I'm worried this could lead to a confusing diagnostic. As stated in Slack, I avoid using the non-empty qualifier. If I write an array literal and downstream I see a diagnostic with the + sign, I could see myself being thrown for a loop. I guess it depends how this gets implemented? Just something to consider I guess

@peterhuene
Copy link
Collaborator Author

peterhuene commented Sep 27, 2024

Array[X]+ can coerce to Array[X], but not the inverse. If we do not include the qualifier on the array literal type calculus, you would not be able to call select_first([a, b, c]) (where a, b, and c are T?).

@peterhuene
Copy link
Collaborator Author

peterhuene commented Sep 27, 2024

Erm, not sure how I deleted that comment:

I have a fix incoming for this same function where the element type calculation for both Array and Map such that the first element type might coerce to the second element type, but not the inverse.

For example, if you have [a, b] where a is T and b is T?. As T coerces to T?, but not the inverse, the type of this expression should actually be Array[T?]+ and not Array[T]+, but currently we treat it as an error as the type of b does not coerce to the type of a.

@peterhuene peterhuene self-assigned this Sep 27, 2024
@peterhuene
Copy link
Collaborator Author

So instead of always qualifying a non-empty literal array type with Array[T]+, we need to basically treat it as coercible to either Array[T] or Array[T]+ to support an expression like if (x) then [1, 2, 3] else [], which should have a type of Array[Int]; if we treated the first literal array expression as non-empty, we wouldn't be able to coerce it to Array[Int].

We don't want [] to be Array[Union]+ either, otherwise this would be legal: Array[String]+ foo = [], which the type system should catch.

I have a fix that basically ignores the non-empty qualifier for coercions when involving non-empty literal arrays.

peterhuene added a commit to peterhuene/wdl that referenced this issue Sep 27, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants