Skip to content

Commit

Permalink
fix: move import validation from wdl-analysis to wdl-ast.
Browse files Browse the repository at this point in the history
This commit moves the validation of imports from `wdl-anlaysis` to `wdl-ast`
where the rest of the validation visits occur.

Fixes stjude-rust-labs#152.
  • Loading branch information
peterhuene committed Aug 13, 2024
1 parent da8b7e6 commit e65deaa
Show file tree
Hide file tree
Showing 11 changed files with 118 additions and 62 deletions.
16 changes: 8 additions & 8 deletions Gauntlet.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ commit_hash = "ae5b3026ea0102ab4164d30d552cabfc16076b44"

[repositories."broadinstitute/warp"]
identifier = "broadinstitute/warp"
commit_hash = "a899f970f62404623dd42ec63d48346e74e86ea7"
commit_hash = "9b5e498e19b89ef72a2c27bb68884c54de768d76"

[repositories."chanzuckerberg/czid-workflows"]
identifier = "chanzuckerberg/czid-workflows"
Expand All @@ -49,7 +49,7 @@ filters = ["/template/task-templates.wdl"]

[repositories."theiagen/public_health_bioinformatics"]
identifier = "theiagen/public_health_bioinformatics"
commit_hash = "935a8b446c94dc3c72eaa79d0d060977ca0ef7d6"
commit_hash = "109e5878fd71d4102dff91110d18d6d6f1d2c002"

[[diagnostics]]
document = "aws-samples/amazon-omics-tutorials:/example-workflows/gatk-best-practices/workflows/somatic-snps-and-indels/mutec2.wdl"
Expand Down Expand Up @@ -384,32 +384,32 @@ permalink = "https://github.com/broadinstitute/palantir-workflows/blob/ae5b3026e
[[diagnostics]]
document = "broadinstitute/warp:/pipelines/skylab/scATAC/scATAC.wdl"
message = "scATAC.wdl:203:9: error: duplicate key `cpu` in runtime section"
permalink = "https://github.com/broadinstitute/warp/blob/a899f970f62404623dd42ec63d48346e74e86ea7/pipelines/skylab/scATAC/scATAC.wdl/#L203"
permalink = "https://github.com/broadinstitute/warp/blob/9b5e498e19b89ef72a2c27bb68884c54de768d76/pipelines/skylab/scATAC/scATAC.wdl/#L203"

[[diagnostics]]
document = "broadinstitute/warp:/tasks/broad/GermlineVariantDiscovery.wdl"
message = "GermlineVariantDiscovery.wdl:140:32: error: expected string, but found integer"
permalink = "https://github.com/broadinstitute/warp/blob/a899f970f62404623dd42ec63d48346e74e86ea7/tasks/broad/GermlineVariantDiscovery.wdl/#L140"
permalink = "https://github.com/broadinstitute/warp/blob/9b5e498e19b89ef72a2c27bb68884c54de768d76/tasks/broad/GermlineVariantDiscovery.wdl/#L140"

[[diagnostics]]
document = "broadinstitute/warp:/tasks/broad/GermlineVariantDiscovery.wdl"
message = "GermlineVariantDiscovery.wdl:67:32: error: expected string, but found integer"
permalink = "https://github.com/broadinstitute/warp/blob/a899f970f62404623dd42ec63d48346e74e86ea7/tasks/broad/GermlineVariantDiscovery.wdl/#L67"
permalink = "https://github.com/broadinstitute/warp/blob/9b5e498e19b89ef72a2c27bb68884c54de768d76/tasks/broad/GermlineVariantDiscovery.wdl/#L67"

[[diagnostics]]
document = "broadinstitute/warp:/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl"
message = "UltimaGenomicsWholeGenomeGermlineTasks.wdl:814:27: error: expected string, but found integer"
permalink = "https://github.com/broadinstitute/warp/blob/a899f970f62404623dd42ec63d48346e74e86ea7/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl/#L814"
permalink = "https://github.com/broadinstitute/warp/blob/9b5e498e19b89ef72a2c27bb68884c54de768d76/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl/#L814"

[[diagnostics]]
document = "broadinstitute/warp:/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl"
message = "UltimaGenomicsWholeGenomeGermlineTasks.wdl:866:27: error: expected string, but found integer"
permalink = "https://github.com/broadinstitute/warp/blob/a899f970f62404623dd42ec63d48346e74e86ea7/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl/#L866"
permalink = "https://github.com/broadinstitute/warp/blob/9b5e498e19b89ef72a2c27bb68884c54de768d76/tasks/broad/UltimaGenomicsWholeGenomeGermlineTasks.wdl/#L866"

[[diagnostics]]
document = "broadinstitute/warp:/tests/cemba/pr/CheckCembaOutputs.wdl"
message = "CheckCembaOutputs.wdl:1:1: error: a WDL document must start with a version statement"
permalink = "https://github.com/broadinstitute/warp/blob/a899f970f62404623dd42ec63d48346e74e86ea7/tests/cemba/pr/CheckCembaOutputs.wdl/#L1"
permalink = "https://github.com/broadinstitute/warp/blob/9b5e498e19b89ef72a2c27bb68884c54de768d76/tests/cemba/pr/CheckCembaOutputs.wdl/#L1"

[[diagnostics]]
document = "chanzuckerberg/czid-workflows:/workflows/index-generation/index-generation.wdl"
Expand Down
54 changes: 11 additions & 43 deletions wdl-analysis/src/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use url::Url;
use wdl_ast::support::token;
use wdl_ast::v1;
use wdl_ast::v1::ImportStatement;
use wdl_ast::v1::StringPart;
use wdl_ast::v1::WorkflowStatement;
use wdl_ast::Ast;
use wdl_ast::AstNode;
Expand Down Expand Up @@ -67,17 +66,6 @@ impl fmt::Display for NameContext {
}
}

/// Creates an "empty import" diagnostic
fn empty_import(span: Span) -> Diagnostic {
Diagnostic::error("import URI cannot be empty").with_highlight(span)
}

/// Creates a "placeholder in import" diagnostic
fn placeholder_in_import(span: Span) -> Diagnostic {
Diagnostic::error("import URI cannot contain placeholders")
.with_label("remove this placeholder", span)
}

/// Creates a "name conflict" diagnostic
fn name_conflict(name: &str, conflicting: NameContext, first: NameContext) -> Diagnostic {
Diagnostic::error(format!("conflicting {conflicting} name `{name}`"))
Expand Down Expand Up @@ -107,13 +95,6 @@ fn namespace_conflict(name: &str, conflicting: Span, first: Span, suggest_fix: b
}
}

/// Creates an "invalid import namespace" diagnostic
fn invalid_import_namespace(span: Span) -> Diagnostic {
Diagnostic::error("import namespace is not a valid WDL identifier")
.with_label("a namespace cannot be derived from this import path", span)
.with_fix("add an `as` clause to the import to specify a namespace")
}

/// Creates an "import cycle" diagnostic
fn import_cycle(span: Span) -> Diagnostic {
Diagnostic::error("import introduces a dependency cycle")
Expand Down Expand Up @@ -676,10 +657,11 @@ impl DocumentScope {
let (uri, scope) =
match Self::resolve_import(graph, import, importer_index, importer_version) {
Ok(scope) => scope,
Err(diagnostic) => {
Err(Some(diagnostic)) => {
diagnostics.push(diagnostic);
return;
}
Err(None) => return,
};

// Check for conflicting namespaces
Expand Down Expand Up @@ -707,7 +689,7 @@ impl DocumentScope {
}
}
None => {
diagnostics.push(invalid_import_namespace(span));
// Invalid import, ignore it
return;
}
}
Expand Down Expand Up @@ -1207,46 +1189,32 @@ impl DocumentScope {
stmt: &v1::ImportStatement,
importer_index: NodeIndex,
importer_version: &Version,
) -> Result<(Arc<Url>, Arc<DocumentScope>), Diagnostic> {
) -> Result<(Arc<Url>, Arc<DocumentScope>), Option<Diagnostic>> {
let uri = stmt.uri();
let span = uri.syntax().text_range().to_span();
let text = match uri.text() {
Some(text) => text,
None => {
if uri.is_empty() {
return Err(empty_import(span));
}

let span = uri
.parts()
.find_map(|p| match p {
StringPart::Text(_) => None,
StringPart::Placeholder(p) => Some(p),
})
.expect("should contain a placeholder")
.syntax()
.text_range()
.to_span();
return Err(placeholder_in_import(span));
return Err(None);
}
};

let uri = match graph.get(importer_index).uri().join(text.as_str()) {
Ok(uri) => uri,
Err(e) => return Err(invalid_relative_import(&e, span)),
Err(e) => return Err(Some(invalid_relative_import(&e, span))),
};

let import_index = graph.get_index(&uri).expect("missing import node in graph");
let import_node = graph.get(import_index);

// Check for an import cycle to report
if graph.contains_cycle(importer_index, import_index) {
return Err(import_cycle(span));
return Err(Some(import_cycle(span)));
}

// Check for a failure to load the import
if let ParseState::Error(e) = import_node.parse_state() {
return Err(import_failure(text.as_str(), e, span));
return Err(Some(import_failure(text.as_str(), e, span)));
}

// Ensure the import has a matching WDL version
Expand All @@ -1262,15 +1230,15 @@ impl DocumentScope {
let our_version = stmt.version();
if matches!((our_version.as_str().split('.').next(), importer_version.as_str().split('.').next()), (Some(our_major), Some(their_major)) if our_major != their_major)
{
return Err(incompatible_import(
return Err(Some(incompatible_import(
our_version.as_str(),
span,
importer_version,
));
)));
}
}
None => {
return Err(import_missing_version(span));
return Err(Some(import_missing_version(span)));
}
}

Expand Down

This file was deleted.

4 changes: 0 additions & 4 deletions wdl-analysis/tests/analysis/placeholder-in-import/ok.wdl

This file was deleted.

4 changes: 4 additions & 0 deletions wdl-ast/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
specification](https://github.com/openwdl/wdl/blob/wdl-1.2/SPEC.md#container)
([#142](https://github.com/stjude-rust-labs/wdl/pull/142)).

### Fixed

* Moved validation of import statements to `wdl-ast` ([#158](https://github.com/stjude-rust-labs/wdl/pull/158)).

## 0.5.0 - 07-17-2024

### Added
Expand Down
2 changes: 2 additions & 0 deletions wdl-ast/src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::Visitor;

mod counts;
mod exprs;
mod imports;
mod keys;
mod numbers;
mod requirements;
Expand Down Expand Up @@ -103,6 +104,7 @@ impl Default for Validator {
Box::<version::VersionVisitor>::default(),
Box::<requirements::RequirementsVisitor>::default(),
Box::<exprs::ScopedExprVisitor>::default(),
Box::<imports::ImportsVisitor>::default(),
],
}
}
Expand Down
90 changes: 90 additions & 0 deletions wdl-ast/src/validation/imports.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
//! Validation of imports.
use crate::v1;
use crate::v1::StringPart;
use crate::AstNode;
use crate::Diagnostic;
use crate::Diagnostics;
use crate::Document;
use crate::Span;
use crate::SupportedVersion;
use crate::ToSpan;
use crate::VisitReason;
use crate::Visitor;

/// Creates an "empty import" diagnostic
fn empty_import(span: Span) -> Diagnostic {
Diagnostic::error("import URI cannot be empty").with_highlight(span)
}

/// Creates a "placeholder in import" diagnostic
fn placeholder_in_import(span: Span) -> Diagnostic {
Diagnostic::error("import URI cannot contain placeholders")
.with_label("remove this placeholder", span)
}

/// Creates an "invalid import namespace" diagnostic
fn invalid_import_namespace(span: Span) -> Diagnostic {
Diagnostic::error("import namespace is not a valid WDL identifier")
.with_label("a namespace cannot be derived from this import path", span)
.with_fix("add an `as` clause to the import to specify a namespace")
}

/// An AST visitor that ensures that imports are valid.
#[derive(Debug, Default)]
pub struct ImportsVisitor;

impl Visitor for ImportsVisitor {
type State = Diagnostics;

fn document(
&mut self,
_: &mut Self::State,
reason: VisitReason,
_: &Document,
_: SupportedVersion,
) {
if reason == VisitReason::Exit {
return;
}

*self = Default::default();
}

fn import_statement(
&mut self,
state: &mut Self::State,
reason: VisitReason,
stmt: &v1::ImportStatement,
) {
if reason == VisitReason::Exit {
return;
}

let uri = stmt.uri();
if uri.is_empty() {
state.add(empty_import(uri.syntax().text_range().to_span()));
return;
}

if uri.text().is_none() {
let span = uri
.parts()
.find_map(|p| match p {
StringPart::Text(_) => None,
StringPart::Placeholder(p) => Some(p.syntax().text_range().to_span()),
})
.expect("should have a placeholder span");

state.add(placeholder_in_import(span));
return;
}

if stmt.namespace().is_none() {
state.add(invalid_import_namespace(
uri.syntax().text_range().to_span(),
));
return;
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: import namespace is not a valid WDL identifier
┌─ tests/analysis/import-namespace-invalid/source.wdl:5:8
┌─ tests/validation/import-namespace-invalid/source.wdl:5:8
5 │ import "invalid-namespace.wdl"
│ ^^^^^^^^^^^^^^^^^^^^^^^ a namespace cannot be derived from this import path
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error: import URI cannot contain placeholders
┌─ tests/analysis/placeholder-in-import/source.wdl:5:23
┌─ tests/validation/placeholder-in-import/source.wdl:5:23
5 │ import "this contains ~{"a placeholder"}" as foo
│ ^^^^^^^^^^^^^^^^^^ remove this placeholder

error: import URI cannot contain placeholders
┌─ tests/analysis/placeholder-in-import/source.wdl:6:28
┌─ tests/validation/placeholder-in-import/source.wdl:6:28
6 │ import "this also contains ${"a placeholder"}" as bar
│ ^^^^^^^^^^^^^^^^^^ remove this placeholder
Expand Down

0 comments on commit e65deaa

Please sign in to comment.