-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: type checking in tasks. #163
Conversation
Opened #164 to track the missing 1.2 features. |
This commit implements full type checking in tasks. * Detection of cycles in declarations. * Type calculations of expressions. * Type checking of string interpolations. Workflow type checking is forthcoming. The following have not been implemented yet, which is required for full 1.2 support: * support `task` name (with "task" hidden type) in commands and output declarations. * support `input`, `output`, and `hints` hidden types in literal expression evaluation. Additionally, the following changes are included: * Refactored `wdl-analysis` to make certain modules public rather than reexporting everything at the root (cleans up the docs a bit). * Removed DFS space from `DocumentGraph` as it doesn't need to persist after analysis completes. * Simplified `DocumentScope` to contain only whats needed to express names and their types. * Removed the restriction on `Map` type representation to allow `Union` as the key type, which allows type checking to work on a literal `{}`, which has calculated type `Map[Union, Union]`. * Fixed `basename` function to accept a `String` argument. * Fixed `size` function to accept a `String` argument. * Removed the `span_of` function in favor of an `AstNode` extension trait. * Changed the `call_expr` grammar rule to only accept an identifier for the target and not an expression as functions cannot be values in WDL. * Fixed the CLI tools to not print colors when output is not to a terminal. * Fixed incorrect parsing of some expressions which required the interior to be non-empty (e.g. `foo[]` is not a valid index expression).
9a345dd
to
f55c1be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 Qs:
- does merging this bring us up to parity with
miniwdl check
? - I don't see a test for a forward reference, but from my understanding of this code and the spec those should be parseable.
task foo {
input {
String arg = if bool then "--arg" else ""
Boolean bool
}
command <<<>>>
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When those are addressed, looks great! Admittedly, there is a ton here, so I may not have caught everything—I sort of ran with the assumption that the approach you took is solid and we can continue to tweak it in the future if need be (mainly because I don't have any concrete experience with better ways to do it).
It does not yet; once I complete workflow type checking, it will.
I added tests for cycle detection, but you're right, it'd be good to have one that passes without errors with a forward reference. I'll get that added! |
This commit implements full type checking in tasks.
Workflow type checking is forthcoming.
The following have not been implemented yet, which is required for full 1.2 support:
task
name (with "task" hidden type) in commands and output declarations.input
,output
, andhints
hidden types in literal expression evaluation.Additionally, the following changes are included:
wdl-analysis
to make certain modules public rather than reexporting everything at the root (cleans up the docs a bit).DocumentGraph
as it doesn't need to persist after analysis completes.DocumentScope
to contain only whats needed to express names and their types.Map
type representation to allowUnion
as the key type, which allows type checking to work on a literal{}
, which has calculated typeMap[Union, Union]
.basename
function to accept aString
argument.size
function to accept aString
argument.span_of
function in favor of anAstNode
extension trait.call_expr
grammar rule to only accept an identifier for the target and not an expression as functions cannot be values in WDL.foo[]
is not a valid index expression).Before submitting this PR, please make sure:
changes (when appropriate).
CHANGELOG.md
(see"keep a changelog" for more information).