fix: move import validation from wdl-analysis to wdl-ast.

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"

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,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
@@ -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)));
             }
         }
 

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

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::<version::VersionVisitor>::default(),
                 Box::<requirements::RequirementsVisitor>::default(),
                 Box::<exprs::ScopedExprVisitor>::default(),
+                Box::<imports::ImportsVisitor>::default(),
             ],
         }
     }

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(),
+            ));
+        }
+    }
+}

wdl-analysis/tests/analysis/import-namespace-invalid/source.diagnostics → 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

wdl-analysis/tests/analysis/placeholder-in-import/source.diagnostics → 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