From c9088b0bd0528b39794337c64a2b7cf932c368b7 Mon Sep 17 00:00:00 2001 From: Yavor Panayotov Date: Tue, 16 Jun 2026 17:21:49 +0300 Subject: [PATCH] Cross-module drift detection for qualified default types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #47. Extends default field-schema validation to qualified `default alias/Type = { ... }` literals in the multi-file CLI. `collect_entity_field_schemas` exposes each module's entity/value type → field names; the CLI builds a per-file `imported_entity_fields` map (alias → type → fields) on `CrossModuleContext`, mirroring `imported_triggers`, and threads it through `analyze_with_cross_module` into `Ctx`. `check_default_field_schemas` then flags a qualified-default field not declared on the imported type (`allium.default.unknownField`). Aliases or types whose target module is outside the check set are left unvalidated rather than flagged — the same convention as qualified triggers. This is multi-file-CLI-only: the single-file TypeScript analyzer cannot see other modules, so it continues to validate only local default types (an asymmetry already documented for qualified-trigger resolution). The cross-module path carries field names only, so rule 14c and nested-object recursion remain local-type-only. Adds two CLI integration tests (drift flagged across modules; not flagged when the target is outside the check set) and updates the parity doc. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/allium-parser/src/analysis.rs | 96 +++++++++++++++++----- crates/allium-parser/src/lib.rs | 4 +- crates/allium/src/main.rs | 47 +++++++++-- crates/allium/tests/cross_module_unused.rs | 59 +++++++++++++ docs/project/rust-checker-parity.md | 17 ++-- 5 files changed, 190 insertions(+), 33 deletions(-) diff --git a/crates/allium-parser/src/analysis.rs b/crates/allium-parser/src/analysis.rs index 94f72fb..d6dd07c 100644 --- a/crates/allium-parser/src/analysis.rs +++ b/crates/allium-parser/src/analysis.rs @@ -62,18 +62,18 @@ pub fn analyze_with_cross_module( external_refs: &HashSet, resolved_use_paths: &HashSet, imported_triggers: &HashMap>, + imported_entity_fields: &HashMap>>, ambiguous_imports: &AmbiguousImports, ) -> Vec { - run_checks( - Ctx::new( - module, - external_refs, - Some(resolved_use_paths), - Some(imported_triggers), - Some(ambiguous_imports), - ), - source, - ) + let mut ctx = Ctx::new( + module, + external_refs, + Some(resolved_use_paths), + Some(imported_triggers), + Some(ambiguous_imports), + ); + ctx.imported_entity_fields = Some(imported_entity_fields); + run_checks(ctx, source) } fn run_checks(mut ctx: Ctx<'_>, source: &str) -> Vec { @@ -131,6 +131,7 @@ pub fn analyse_with_cross_module( external_refs: &HashSet, resolved_use_paths: &HashSet, imported_triggers: &HashMap>, + imported_entity_fields: &HashMap>>, ambiguous_imports: &AmbiguousImports, ) -> crate::diagnostic::AnalyseResult { let diagnostics = analyze_with_cross_module( @@ -139,6 +140,7 @@ pub fn analyse_with_cross_module( external_refs, resolved_use_paths, imported_triggers, + imported_entity_fields, ambiguous_imports, ); let findings = find_process_issues(module, Some(imported_triggers)); @@ -309,6 +311,11 @@ struct Ctx<'a> { /// `Some(map)` = multi-file mode; names and triggers that more than one /// imported module could resolve. ambiguous_imports: Option<&'a AmbiguousImports>, + /// Multi-file mode only: per `use` alias, the imported module's entity/value + /// type → declared field names. Lets a qualified `default alias/Type` literal + /// be validated against the imported schema. Aliases whose targets fall + /// outside the check set are absent. `None` in single-file mode. + imported_entity_fields: Option<&'a HashMap>>>, diagnostics: Vec, findings: Vec, } @@ -327,6 +334,7 @@ impl<'a> Ctx<'a> { resolved_use_paths, imported_triggers, ambiguous_imports, + imported_entity_fields: None, diagnostics: Vec::new(), findings: Vec::new(), } @@ -4157,6 +4165,22 @@ pub fn collect_trigger_outputs(module: &Module) -> HashSet { names.into_iter().map(str::to_string).collect() } +/// Collect each entity/value type's declared field names, keyed by type name. +/// +/// Used by multi-file checking to build the per-alias schema map that lets a +/// qualified `default alias/Type = { ... }` literal be validated against the +/// imported type's fields (drift detection across `use` imports). +pub fn collect_entity_field_schemas(module: &Module) -> HashMap> { + let mut out: HashMap> = HashMap::new(); + for (name, fields) in collect_local_type_schemas(module) { + out.insert( + name.to_string(), + fields.keys().map(|f| f.to_string()).collect(), + ); + } + out +} + fn collect_qrefs_from_item(kind: &BlockItemKind, out: &mut Vec<(String, String)>) { match kind { BlockItemKind::Clause { value, .. } @@ -4613,15 +4637,45 @@ impl Ctx<'_> { let mut diagnostics = Vec::new(); for d in &self.module.declarations { let Decl::Default(def) = d else { continue }; - if def.type_alias.is_some() { - continue; // imported type — schema not visible here - } let (Some(type_name), Expr::ObjectLiteral { fields, .. }) = (&def.type_name, &def.value) else { continue; }; - validate_object_literal(fields, &type_name.name, &schemas, &mut diagnostics); + match &def.type_alias { + None => { + validate_object_literal(fields, &type_name.name, &schemas, &mut diagnostics); + } + Some(alias) => { + // Qualified `default alias/Type`: validate the top-level + // field set against the imported module's schema, when that + // module is in the check set (multi-file mode). Aliases or + // types outside the check set are left unvalidated rather + // than flagged. Nested validation and rule 14c need the + // imported field *types*, which aren't carried cross-module, + // so only unknown-field drift is checked here. + if let Some(imported) = self + .imported_entity_fields + .and_then(|m| m.get(alias.name.as_str())) + .and_then(|types| types.get(type_name.name.as_str())) + { + for field in fields { + if !imported.contains(field.name.name.as_str()) { + diagnostics.push( + Diagnostic::error( + field.name.span, + format!( + "Default sets field '{}' which is not declared on '{}/{}'.", + field.name.name, alias.name, type_name.name + ), + ) + .with_code("allium.default.unknownField"), + ); + } + } + } + } + } } for diag in diagnostics { self.push(diag); @@ -6087,6 +6141,7 @@ mod tests { &HashSet::new(), &HashSet::new(), &imported, + &HashMap::new(), &AmbiguousImports::default(), ) } @@ -6133,6 +6188,7 @@ mod tests { &HashSet::new(), &HashSet::new(), &imported, + &HashMap::new(), &ambiguous, ) } @@ -6567,7 +6623,7 @@ mod tests { let input = format!("-- allium: 3\n{src}"); let result = parse(&input); let resolved: HashSet = ["./core.allium".to_string()].into_iter().collect(); - let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &AmbiguousImports::default()); + let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &HashMap::new(), &AmbiguousImports::default()); assert!(!has_code(&ds, "allium.use.unresolvedPath")); } @@ -6578,7 +6634,7 @@ mod tests { let result = parse(&input); // Only "./other.allium" is resolved — "./missing.allium" is not. let resolved: HashSet = ["./other.allium".to_string()].into_iter().collect(); - let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &AmbiguousImports::default()); + let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &HashMap::new(), &AmbiguousImports::default()); assert!(has_code(&ds, "allium.use.unresolvedPath")); } @@ -6599,7 +6655,7 @@ mod tests { let src = "use \"./missing.allium\" as missing\n\nentity Handler {\n x: String\n}\n"; let input = format!("-- allium: 3\n{src}"); let result = parse(&input); - let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &HashSet::new(), &HashMap::new(), &AmbiguousImports::default()); + let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &HashSet::new(), &HashMap::new(), &HashMap::new(), &AmbiguousImports::default()); assert!(has_code(&ds, "allium.use.unresolvedPath")); } @@ -6609,7 +6665,7 @@ mod tests { let input = format!("-- allium: 3\n{src}"); let result = parse(&input); let resolved: HashSet = ["./other.allium".to_string()].into_iter().collect(); - let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &AmbiguousImports::default()); + let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &HashMap::new(), &AmbiguousImports::default()); let diag = ds.iter().find(|d| d.code == Some("allium.use.unresolvedPath")).unwrap(); assert!(diag.message.contains("nowhere.allium"), "message should name the path: {}", diag.message); } @@ -6620,7 +6676,7 @@ mod tests { let input = format!("-- allium: 3\n{src}"); let result = parse(&input); let resolved: HashSet = ["./other.allium".to_string()].into_iter().collect(); - let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &AmbiguousImports::default()); + let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &HashMap::new(), &AmbiguousImports::default()); assert!(!has_code(&ds, "allium.use.unresolvedPath")); } @@ -6630,7 +6686,7 @@ mod tests { let input = format!("-- allium: 3\n{src}"); let result = parse(&input); let resolved: HashSet = ["./found.allium".to_string()].into_iter().collect(); - let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &AmbiguousImports::default()); + let ds = analyze_with_cross_module(&result.module, &input, &HashSet::new(), &resolved, &HashMap::new(), &HashMap::new(), &AmbiguousImports::default()); let unresolved: Vec<_> = ds.iter() .filter(|d| d.code == Some("allium.use.unresolvedPath")) .collect(); diff --git a/crates/allium-parser/src/lib.rs b/crates/allium-parser/src/lib.rs index 883f6df..1dbad10 100644 --- a/crates/allium-parser/src/lib.rs +++ b/crates/allium-parser/src/lib.rs @@ -8,8 +8,8 @@ pub mod span; pub use analysis::{ analyze, analyze_with_cross_module, analyze_with_external_refs, analyse, analyse_with_cross_module, analyse_with_external_refs, collect_all_referenced_idents, - collect_declared_names, collect_qualified_references, collect_trigger_outputs, - AmbiguousImports, + collect_declared_names, collect_entity_field_schemas, collect_qualified_references, + collect_trigger_outputs, AmbiguousImports, }; pub use ast::Module; pub use diagnostic::{AnalyseResult, Diagnostic, Finding}; diff --git a/crates/allium/src/main.rs b/crates/allium/src/main.rs index 21087a6..c8d8fe9 100644 --- a/crates/allium/src/main.rs +++ b/crates/allium/src/main.rs @@ -194,6 +194,11 @@ struct CrossModuleContext { /// Per-file: `use` alias → trigger names the aliased module provides or /// emits. Aliases whose targets are outside the check set are absent. imported_triggers: HashMap>>, + /// Per-file: `use` alias → (imported entity/value type → declared field + /// names). Lets a qualified `default alias/Type` literal be validated + /// against the imported schema. Aliases whose targets are outside the check + /// set are absent. + imported_entity_fields: HashMap>>>, /// Per-file: names and triggers that more than one imported module could /// resolve, so unqualified references to them are ambiguous (issue #15). ambiguous_imports: HashMap, @@ -207,7 +212,7 @@ struct CrossModuleContext { fn run_multi_file( command: &str, args: &[String], - analyse_file: impl Fn(&Path, &str, &allium_parser::ParseResult, &SourceMap, &HashSet, &HashSet, &HashMap>, &AmbiguousImports) -> FileResult, + analyse_file: impl Fn(&Path, &str, &allium_parser::ParseResult, &SourceMap, &HashSet, &HashSet, &HashMap>, &HashMap>>, &AmbiguousImports) -> FileResult, ) -> ExitCode { let files = resolve_files(args); if files.is_empty() { @@ -247,8 +252,9 @@ fn run_multi_file( let refs = ctx.external_refs.get(&key).cloned().unwrap_or_default(); let use_paths = ctx.resolved_use_paths.get(&key).cloned().unwrap_or_default(); let imports = ctx.imported_triggers.get(&key).cloned().unwrap_or_default(); + let imported_fields = ctx.imported_entity_fields.get(&key).cloned().unwrap_or_default(); let ambiguous = ctx.ambiguous_imports.get(&key).unwrap_or(&no_ambiguity); - let file_result = analyse_file(&pf.path, &pf.source, &pf.result, &source_map, &refs, &use_paths, &imports, ambiguous); + let file_result = analyse_file(&pf.path, &pf.source, &pf.result, &source_map, &refs, &use_paths, &imports, &imported_fields, ambiguous); if file_result.has_issues { any_issues = true; @@ -299,9 +305,26 @@ fn build_cross_module_context(parsed: &[ParsedFile]) -> CrossModuleContext { }) .collect(); + // Pre-compute each file's entity/value type → field-name schema so importing + // files can validate `default alias/Type = { ... }` literals against the + // imported type's declared fields. + let entity_field_outputs: HashMap>> = parsed + .iter() + .map(|pf| { + ( + canonical_key(&pf.path), + allium_parser::collect_entity_field_schemas(&pf.result.module), + ) + }) + .collect(); + let mut external_refs: HashMap> = HashMap::new(); let mut resolved_use_paths: HashMap> = HashMap::new(); let mut imported_triggers: HashMap>> = HashMap::new(); + let mut imported_entity_fields: HashMap< + PathBuf, + HashMap>>, + > = HashMap::new(); let mut ambiguous_imports: HashMap = HashMap::new(); for pf in parsed { @@ -373,6 +396,17 @@ fn build_cross_module_context(parsed: &[ParsedFile]) -> CrossModuleContext { } imported_triggers.insert(file_key.clone(), imported_for_file); + // 3b. Imported entity field schemas — for each alias whose target is in + // the check set, that module's entity/value type → field names. + let mut imported_fields_for_file: HashMap>> = + HashMap::new(); + for (alias, target_key) in &alias_targets { + if let Some(fields) = entity_field_outputs.get(target_key) { + imported_fields_for_file.insert((*alias).to_string(), fields.clone()); + } + } + imported_entity_fields.insert(file_key.clone(), imported_fields_for_file); + // 4. Ambiguous imports — names declared, and triggers provided or // emitted, by more than one distinct imported file. Keyed by // distinct target so that two aliases for the same file are not @@ -426,6 +460,7 @@ fn build_cross_module_context(parsed: &[ParsedFile]) -> CrossModuleContext { external_refs, resolved_use_paths, imported_triggers, + imported_entity_fields, ambiguous_imports, } } @@ -440,8 +475,8 @@ fn canonical_key(path: &Path) -> PathBuf { } fn cmd_check(args: &[String]) -> ExitCode { - run_multi_file("check", args, |path, source, result, source_map, external_refs, resolved_use_paths, imported_triggers, ambiguous_imports| { - let analysis = allium_parser::analyze_with_cross_module(&result.module, source, external_refs, resolved_use_paths, imported_triggers, ambiguous_imports); + run_multi_file("check", args, |path, source, result, source_map, external_refs, resolved_use_paths, imported_triggers, imported_entity_fields, ambiguous_imports| { + let analysis = allium_parser::analyze_with_cross_module(&result.module, source, external_refs, resolved_use_paths, imported_triggers, imported_entity_fields, ambiguous_imports); let diagnostics: Vec = result .diagnostics .iter() @@ -456,8 +491,8 @@ fn cmd_check(args: &[String]) -> ExitCode { } fn cmd_analyse(args: &[String]) -> ExitCode { - run_multi_file("analyse", args, |path, source, result, source_map, external_refs, resolved_use_paths, imported_triggers, ambiguous_imports| { - let analyse_result = allium_parser::analyse_with_cross_module(&result.module, source, external_refs, resolved_use_paths, imported_triggers, ambiguous_imports); + run_multi_file("analyse", args, |path, source, result, source_map, external_refs, resolved_use_paths, imported_triggers, imported_entity_fields, ambiguous_imports| { + let analyse_result = allium_parser::analyse_with_cross_module(&result.module, source, external_refs, resolved_use_paths, imported_triggers, imported_entity_fields, ambiguous_imports); let diagnostics: Vec = result .diagnostics .iter() diff --git a/crates/allium/tests/cross_module_unused.rs b/crates/allium/tests/cross_module_unused.rs index a5c9dfe..8d1cb1d 100644 --- a/crates/allium/tests/cross_module_unused.rs +++ b/crates/allium/tests/cross_module_unused.rs @@ -903,3 +903,62 @@ fn qualified_trigger_with_alias_outside_check_set_not_flagged() { diags.iter().map(|d| (&d.code, &d.message)).collect::>() ); } + +#[test] +fn qualified_default_drift_flagged_across_modules() { + // A `default alias/Type` literal is validated against the imported type's + // fields when the target module is in the check set (allium-tools#47). + let dir = TempDir::new("xmod-default-drift"); + dir.write( + "parent.allium", + "-- allium: 3\nentity Policy {\n id: String\n name: String\n}\n", + ); + dir.write( + "fragment.allium", + "-- allium: 3\nuse \"./parent.allium\" as gp\n\ndefault gp/Policy good = { id: \"p1\", name: \"ok\" }\ndefault gp/Policy drift = { id: \"p2\", naem: \"typo\" }\n", + ); + + let output = allium() + .args(["check", dir.path().to_str().unwrap()]) + .output() + .expect("spawn allium"); + let diags = parse_diagnostics(&String::from_utf8_lossy(&output.stdout)); + + let unknown: Vec<_> = diags + .iter() + .filter(|d| d.code == "allium.default.unknownField") + .collect(); + assert_eq!( + unknown.len(), + 1, + "exactly one drift field (naem): {:?}", + unknown.iter().map(|d| &d.message).collect::>() + ); + assert!(unknown[0].message.contains("naem")); + assert!(unknown[0].message.contains("gp/Policy")); +} + +#[test] +fn qualified_default_not_flagged_when_target_outside_check_set() { + // Only fragment.allium is checked; the imported module's schema is not + // visible, so the qualified default must not be flagged. + let dir = TempDir::new("xmod-default-noflag"); + dir.write( + "parent.allium", + "-- allium: 3\nentity Policy {\n id: String\n}\n", + ); + dir.write( + "fragment.allium", + "-- allium: 3\nuse \"./parent.allium\" as gp\n\ndefault gp/Policy d = { id: \"x\", whatever: 1 }\n", + ); + + let output = allium() + .args(["check", dir.path().join("fragment.allium").to_str().unwrap()]) + .output() + .expect("spawn allium"); + let codes = diagnostic_codes(&String::from_utf8_lossy(&output.stdout)); + assert!( + !codes.iter().any(|c| c == "allium.default.unknownField"), + "qualified default with target outside check set must not be flagged: {codes:?}" + ); +} diff --git a/docs/project/rust-checker-parity.md b/docs/project/rust-checker-parity.md index a673f28..ed2d45b 100644 --- a/docs/project/rust-checker-parity.md +++ b/docs/project/rust-checker-parity.md @@ -251,11 +251,18 @@ Two language features were added for juxt/allium#43: reported as `allium.default.unknownField` (recursing into nested object literals), and an empty list literal whose target field is not a `List` is reported as `allium.list.emptyListNoElementType` (language-reference rule 14c). - Qualified (imported) default types are **not** validated this way — their field - schema is not visible to a single-module pass; cross-module drift detection - would require plumbing imported entity field sets through `CrossModuleContext` - (as is done for triggers) and would be CLI-multi-file-only, since the - single-file TypeScript analyzer cannot see other modules. + Qualified (imported) default types are validated cross-module in the Rust CLI's + multi-file mode: `collect_entity_field_schemas` exposes each module's + entity/value field names, the CLI builds a per-file `imported_entity_fields` + map (alias → type → fields) on `CrossModuleContext` exactly as it does for + triggers, and `check_default_field_schemas` flags a qualified-default field not + declared on the imported type (`allium.default.unknownField`). Aliases or types + outside the check set are left unvalidated, never flagged — the same convention + as qualified triggers. This is multi-file-CLI-only: the single-file TypeScript + analyzer cannot see other modules, so it validates only **local** default types + (the same asymmetry already documented for qualified-trigger resolution). The + cross-module path carries field *names* only, so rule 14c and nested-object + recursion remain local-type-only. `allium.surface.requiresWithoutDeferred` is TypeScript-only (no Rust equivalent yet). When porting it, note the deferred-name matching semantics fixed in issue #26: a named requires block matches a deferred declaration by its full name, by a trailing `.`-separated segment, or — for module-qualified declarations like `deferred billing/InvoiceWorkflow` — by the unqualified name after the `alias/` prefix. The alias alone must not satisfy the match.