From 151b8eb4e47f282f87fd9b9db69a19100ec9814a Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Tue, 13 Aug 2024 13:25:26 -0700 Subject: [PATCH] fix: move import validation from `wdl-analysis` to `wdl-ast`. This commit moves the validation of imports from `wdl-anlaysis` to `wdl-ast` where the rest of the validation visits occur. Fixes #152. --- Gauntlet.toml | 16 ++-- wdl-analysis/src/scope.rs | 54 +++-------- .../invalid-namespace.wdl | 4 - .../analysis/placeholder-in-import/ok.wdl | 4 - wdl-ast/CHANGELOG.md | 4 + wdl-ast/src/validation.rs | 2 + wdl-ast/src/validation/imports.rs | 89 +++++++++++++++++++ .../import-namespace-invalid/source.errors | 2 +- .../import-namespace-invalid/source.wdl | 0 .../placeholder-in-import/source.errors | 4 +- .../placeholder-in-import/source.wdl | 0 11 files changed, 117 insertions(+), 62 deletions(-) delete mode 100644 wdl-analysis/tests/analysis/import-namespace-invalid/invalid-namespace.wdl delete mode 100644 wdl-analysis/tests/analysis/placeholder-in-import/ok.wdl create mode 100644 wdl-ast/src/validation/imports.rs rename wdl-analysis/tests/analysis/import-namespace-invalid/source.diagnostics => wdl-ast/tests/validation/import-namespace-invalid/source.errors (79%) rename {wdl-analysis/tests/analysis => wdl-ast/tests/validation}/import-namespace-invalid/source.wdl (100%) rename wdl-analysis/tests/analysis/placeholder-in-import/source.diagnostics => wdl-ast/tests/validation/placeholder-in-import/source.errors (74%) rename {wdl-analysis/tests/analysis => wdl-ast/tests/validation}/placeholder-in-import/source.wdl (100%) diff --git a/Gauntlet.toml b/Gauntlet.toml index 8dd37a00..8952c0e7 100644 --- a/Gauntlet.toml +++ b/Gauntlet.toml @@ -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" @@ -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" @@ -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" diff --git a/wdl-analysis/src/scope.rs b/wdl-analysis/src/scope.rs index 51273be8..cc79f51f 100644 --- a/wdl-analysis/src/scope.rs +++ b/wdl-analysis/src/scope.rs @@ -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; @@ -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}`")) @@ -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") @@ -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 @@ -707,7 +689,7 @@ impl DocumentScope { } } None => { - diagnostics.push(invalid_import_namespace(span)); + // Invalid import, ignore it return; } } @@ -1207,33 +1189,19 @@ impl DocumentScope { stmt: &v1::ImportStatement, importer_index: NodeIndex, importer_version: &Version, - ) -> Result<(Arc, Arc), Diagnostic> { + ) -> Result<(Arc, Arc), Option> { 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"); @@ -1241,12 +1209,12 @@ impl DocumentScope { // 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 @@ -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))); } } diff --git a/wdl-analysis/tests/analysis/import-namespace-invalid/invalid-namespace.wdl b/wdl-analysis/tests/analysis/import-namespace-invalid/invalid-namespace.wdl deleted file mode 100644 index 9892c584..00000000 --- a/wdl-analysis/tests/analysis/import-namespace-invalid/invalid-namespace.wdl +++ /dev/null @@ -1,4 +0,0 @@ -version 1.1 - -workflow test { -} diff --git a/wdl-analysis/tests/analysis/placeholder-in-import/ok.wdl b/wdl-analysis/tests/analysis/placeholder-in-import/ok.wdl deleted file mode 100644 index 9892c584..00000000 --- a/wdl-analysis/tests/analysis/placeholder-in-import/ok.wdl +++ /dev/null @@ -1,4 +0,0 @@ -version 1.1 - -workflow test { -} diff --git a/wdl-ast/CHANGELOG.md b/wdl-ast/CHANGELOG.md index c2da483e..a1b884c2 100644 --- a/wdl-ast/CHANGELOG.md +++ b/wdl-ast/CHANGELOG.md @@ -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 diff --git a/wdl-ast/src/validation.rs b/wdl-ast/src/validation.rs index c398f4d7..649162c6 100644 --- a/wdl-ast/src/validation.rs +++ b/wdl-ast/src/validation.rs @@ -12,6 +12,7 @@ use crate::Visitor; mod counts; mod exprs; +mod imports; mod keys; mod numbers; mod requirements; @@ -103,6 +104,7 @@ impl Default for Validator { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ], } } diff --git a/wdl-ast/src/validation/imports.rs b/wdl-ast/src/validation/imports.rs new file mode 100644 index 00000000..b51b3090 --- /dev/null +++ b/wdl-ast/src/validation/imports.rs @@ -0,0 +1,89 @@ +//! 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(), + )); + } + } +} diff --git a/wdl-analysis/tests/analysis/import-namespace-invalid/source.diagnostics b/wdl-ast/tests/validation/import-namespace-invalid/source.errors similarity index 79% rename from wdl-analysis/tests/analysis/import-namespace-invalid/source.diagnostics rename to wdl-ast/tests/validation/import-namespace-invalid/source.errors index be769ffe..9f4a15df 100644 --- a/wdl-analysis/tests/analysis/import-namespace-invalid/source.diagnostics +++ b/wdl-ast/tests/validation/import-namespace-invalid/source.errors @@ -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 diff --git a/wdl-analysis/tests/analysis/import-namespace-invalid/source.wdl b/wdl-ast/tests/validation/import-namespace-invalid/source.wdl similarity index 100% rename from wdl-analysis/tests/analysis/import-namespace-invalid/source.wdl rename to wdl-ast/tests/validation/import-namespace-invalid/source.wdl diff --git a/wdl-analysis/tests/analysis/placeholder-in-import/source.diagnostics b/wdl-ast/tests/validation/placeholder-in-import/source.errors similarity index 74% rename from wdl-analysis/tests/analysis/placeholder-in-import/source.diagnostics rename to wdl-ast/tests/validation/placeholder-in-import/source.errors index d0f6c5b3..928827ae 100644 --- a/wdl-analysis/tests/analysis/placeholder-in-import/source.diagnostics +++ b/wdl-ast/tests/validation/placeholder-in-import/source.errors @@ -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 diff --git a/wdl-analysis/tests/analysis/placeholder-in-import/source.wdl b/wdl-ast/tests/validation/placeholder-in-import/source.wdl similarity index 100% rename from wdl-analysis/tests/analysis/placeholder-in-import/source.wdl rename to wdl-ast/tests/validation/placeholder-in-import/source.wdl