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

feat: type checking in tasks. #163

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

peterhuene
Copy link
Collaborator

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).

Before submitting this PR, please make sure:

  • 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 tests (when appropriate).
  • You have updated the README or other documentation to account for these
    changes (when appropriate).
  • 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.

@peterhuene
Copy link
Collaborator Author

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).
Copy link
Member

@a-frantz a-frantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 Qs:

  1. does merging this bring us up to parity with miniwdl check?
  2. 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 <<<>>>
}

wdl-analysis/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@claymcleod claymcleod left a 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).

@peterhuene
Copy link
Collaborator Author

@a-frantz:

does merging this bring us up to parity with miniwdl check?

It does not yet; once I complete workflow type checking, it will.

I don't see a test for a forward reference, but from my understanding of this code and the spec those should be parseable.

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!

@peterhuene peterhuene merged commit 8e9cd29 into stjude-rust-labs:main Aug 29, 2024
8 checks passed
@peterhuene peterhuene deleted the type-checking branch August 29, 2024 17:43
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 this pull request may close these issues.

3 participants