diff --git a/crates/allium-parser/src/analysis.rs b/crates/allium-parser/src/analysis.rs index bcb2ed7..5f253fb 100644 --- a/crates/allium-parser/src/analysis.rs +++ b/crates/allium-parser/src/analysis.rs @@ -4065,6 +4065,68 @@ fn collect_qualified_field_refs_from_item<'a>( } } +/// Fields read as `bound.field`, for names bound to an imported entity. +/// +/// The bound-name counterpart of [`collect_qualified_field_refs`]: a surface +/// that writes `context shelf: alias/Shelf` reads the relationship as +/// `shelf.copies`, and the qualified spelling never appears. +fn collect_bound_field_refs<'a>( + expr: &'a Expr, + bound: &HashMap<&str, &str>, + out: &mut HashSet<&'a str>, +) { + if let Expr::MemberAccess { object, field, .. } | Expr::OptionalAccess { object, field, .. } = + expr + { + if matches!(object.as_ref(), Expr::Ident(i) if bound.contains_key(i.name.as_str())) { + out.insert(&field.name); + } + } + walk_expr_children(expr, &mut |child| collect_bound_field_refs(child, bound, out)); +} + +fn collect_bound_field_refs_from_item<'a>( + kind: &'a BlockItemKind, + bound: &HashMap<&str, &str>, + out: &mut HashSet<&'a str>, +) { + match kind { + BlockItemKind::Clause { value, .. } + | BlockItemKind::Assignment { value, .. } + | BlockItemKind::ParamAssignment { value, .. } + | BlockItemKind::Let { value, .. } + | BlockItemKind::PathAssignment { value, .. } + | BlockItemKind::InvariantBlock { body: value, .. } + | BlockItemKind::FieldWithWhen { value, .. } => { + collect_bound_field_refs(value, bound, out); + } + BlockItemKind::ForBlock { collection, filter, items, .. } => { + collect_bound_field_refs(collection, bound, out); + if let Some(f) = filter { + collect_bound_field_refs(f, bound, out); + } + for item in items { + collect_bound_field_refs_from_item(&item.kind, bound, out); + } + } + BlockItemKind::IfBlock { branches, else_items } => { + for b in branches { + collect_bound_field_refs(&b.condition, bound, out); + for item in &b.items { + collect_bound_field_refs_from_item(&item.kind, bound, out); + } + } + if let Some(items) = else_items { + for item in items { + collect_bound_field_refs_from_item(&item.kind, bound, out); + } + } + } + _ => {} + } +} + + fn collect_accessed_fields_from_item<'a>(kind: &'a BlockItemKind, out: &mut HashSet<&'a str>) { match kind { BlockItemKind::Clause { value, .. } @@ -4799,10 +4861,11 @@ pub fn collect_reverse_contributions<'a>( ); } - // Fields the importer references through this alias (`alias/Entity.field`), - // restricted to real imported-entity fields. Credited so the imported module - // does not report a field as unused when its only reference lives across the - // boundary. + // Fields the importer references through this alias, credited so the + // imported module does not report a field as unused when its only reference + // lives across the boundary. Two spellings reach the same field: + // `alias/Entity.field`, and `binding.field` where the binding was typed to + // an imported entity. let mut imported_field_names: HashSet<&str> = HashSet::new(); for entity in module_blocks(imported, BlockKind::Entity) .chain(module_blocks(imported, BlockKind::ExternalEntity)) @@ -4816,21 +4879,70 @@ pub fn collect_reverse_contributions<'a>( } } if !imported_field_names.is_empty() { - let mut qualified_refs: HashSet<&str> = HashSet::new(); + let mut refs: HashSet<&str> = HashSet::new(); for block in &importer.declarations { if let Decl::Block(b) = block { for item in &b.items { - collect_qualified_field_refs_from_item(&item.kind, alias, &mut qualified_refs); + collect_qualified_field_refs_from_item(&item.kind, alias, &mut refs); + } + } + } + + // Every imported entity, in the shape the binding-type helpers take. + // They read this map for membership only, so passing all the entities + // rather than `status_by_entity`'s status-bearing ones types a binding + // whatever the entity's lifecycle — which is what a field read wants, + // and what a lifecycle check deliberately does not. + let entity_names: HashMap<&str, HashSet<&str>> = + module_blocks(imported, BlockKind::Entity) + .chain(module_blocks(imported, BlockKind::ExternalEntity)) + .filter_map(|e| e.name.as_ref().map(|n| (n.name.as_str(), HashSet::new()))) + .collect(); + + // A trigger's parameters, typed from the surface that provides it, so a + // rule subscribing to that trigger knows which of its bindings is an + // imported entity. The same table `command_param_types` holds above, + // built over every entity rather than the status-bearing ones. + let mut param_types: HashMap<&str, Vec>> = HashMap::new(); + collect_importer_command_param_types(importer, alias, &entity_names, &mut param_types); + + // A surface reads through the name its own `context`/`facing` bound. + for surface in module_blocks(importer, BlockKind::Surface) { + let mut bound: HashMap<&str, &str> = HashMap::new(); + for item in &surface.items { + if let BlockItemKind::Clause { keyword, value } = &item.kind { + if keyword == "context" || keyword == "facing" { + qualified_context_binding(value, alias, &entity_names, &mut bound); + } } } + if bound.is_empty() { + continue; + } + for item in &surface.items { + collect_bound_field_refs_from_item(&item.kind, &bound, &mut refs); + } } - for f in qualified_refs { + + // A rule reads through the name its trigger bound. + for rule in module_blocks(importer, BlockKind::Rule) { + let bound = rule_trigger_bindings(rule, alias, ¶m_types); + if bound.is_empty() { + continue; + } + for item in &rule.items { + collect_bound_field_refs_from_item(&item.kind, &bound, &mut refs); + } + } + + for f in refs { if imported_field_names.contains(f) { out.referenced_fields.insert(f.to_string()); } } } + out } @@ -5226,6 +5338,49 @@ fn collect_qualified_created( /// qualified subject types the binding directly; or a local trigger the /// importer owns whose parameters its own surface typed to a qualified imported /// entity (`context t: alias/Entity`, then `provides: LocalTrigger(t)`, #65). +/// Rule bindings typed to an imported entity by the trigger the rule subscribes +/// to, mapped positionally against `param_types`. +/// +/// The same resolution [`collect_witnessed_transition`] does for its own +/// `when:` clause, over a parameter table built for every imported entity +/// rather than the status-bearing ones. A qualified `alias/Trigger` names an +/// imported trigger; a bare `Trigger` names one the importer owns whose +/// parameters its surface typed to an imported entity (#65). +fn rule_trigger_bindings<'a>( + rule: &'a BlockDecl, + alias: &str, + param_types: &HashMap<&'a str, Vec>>, +) -> HashMap<&'a str, &'a str> { + let mut bound: HashMap<&str, &str> = HashMap::new(); + for item in &rule.items { + let BlockItemKind::Clause { keyword, value } = &item.kind else { + continue; + }; + if keyword != "when" { + continue; + } + let Expr::Call { function, args, .. } = value else { + continue; + }; + let trigger_name = match function.as_ref() { + Expr::QualifiedName(q) if q.qualifier.as_deref() == Some(alias) => Some(&q.name), + Expr::Ident(id) => Some(&id.name), + _ => None, + }; + let Some(name) = trigger_name else { continue }; + let Some(params) = param_types.get(name.as_str()) else { + continue; + }; + for (arg, param) in args.iter().zip(params) { + if let (CallArg::Positional(Expr::Ident(b)), Some(entity)) = (arg, param) { + bound.insert(b.name.as_str(), entity); + } + } + } + bound +} + + fn collect_witnessed_transition( rule: &BlockDecl, alias: &str, @@ -8721,6 +8876,89 @@ surface AccountManagement { assert!(!other.referenced_fields.contains("due_at")); } + #[test] + fn reverse_contributions_credit_field_read_through_a_context_binding() { + let imported = + module_of("entity Shelf {\n name: String\n copies: Copy with shelf = this\n}\n"); + // `copies` is read as `shelf.copies`, where `shelf` is bound by the + // surface's own `context` to an imported entity. That is the ordinary + // way a surface reads an imported relationship — the qualified form + // `shelves/Shelf.copies` never appears — so it must be credited just as + // a qualified reference is. + let importer = module_of( + "use \"./s.allium\" as shelves\nsurface ShelfView {\n context shelf: shelves/Shelf\n exposes:\n for c in shelf.copies:\n c.title\n}\n", + ); + let rc = collect_reverse_contributions(&importer, "shelves", &imported); + assert!(rc.referenced_fields.contains("copies")); + let other = collect_reverse_contributions(&importer, "other", &imported); + assert!(!other.referenced_fields.contains("copies")); + } + + #[test] + fn reverse_contributions_credit_field_read_through_a_facing_binding() { + let imported = module_of("entity Reader {\n handle: String\n}\n"); + // `facing` types a binding the same way `context` does, so the credit + // must not depend on which keyword introduced it. + let importer = module_of( + "use \"./r.allium\" as people\nsurface Desk {\n facing reader: people/Reader\n exposes:\n reader.handle\n}\n", + ); + let rc = collect_reverse_contributions(&importer, "people", &imported); + assert!(rc.referenced_fields.contains("handle")); + } + + #[test] + fn reverse_contributions_credit_field_read_through_a_refined_binding() { + let imported = module_of("entity Shelf {\n name: String\n copies: Copy\n}\n"); + // `context b: alias/E where …` types `b` exactly as the bare form does + // (#76 family); the refinement must not cost the credit. + let importer = module_of( + "use \"./s.allium\" as shelves\nsurface ShelfView {\n context shelf: shelves/Shelf where name = \"a\"\n exposes:\n shelf.copies\n}\n", + ); + let rc = collect_reverse_contributions(&importer, "shelves", &imported); + assert!(rc.referenced_fields.contains("copies")); + } + + #[test] + fn reverse_contributions_ignore_binding_to_a_name_that_is_no_entity() { + let imported = module_of("entity Shelf {\n copies: Copy\n}\n"); + // The alias resolves, but `Ledger` is not an entity of the imported + // module, so the binding types nothing and the read credits nothing. + let importer = module_of( + "use \"./s.allium\" as shelves\nsurface ShelfView {\n context led: shelves/Ledger\n exposes:\n led.copies\n}\n", + ); + let rc = collect_reverse_contributions(&importer, "shelves", &imported); + assert!(!rc.referenced_fields.contains("copies")); + } + + #[test] + fn reverse_contributions_credit_field_read_through_a_rule_trigger_binding() { + let imported = module_of("entity Shelf {\n copies: Copy with shelf = this\n}\n"); + // The read is in a rule, not a surface: `shelf` is bound by the rule's + // trigger, which the importer's own surface typed by passing its + // `context` binding as the argument (#65's shape, one channel over). + let importer = module_of( + "use \"./s.allium\" as shelves\nsurface Desk {\n context shelf: shelves/Shelf\n provides:\n Audit(shelf)\n}\nrule DoAudit {\n when: Audit(shelf)\n ensures:\n for c in shelf.copies:\n Noted(copy: c)\n}\n", + ); + let rc = collect_reverse_contributions(&importer, "shelves", &imported); + assert!(rc.referenced_fields.contains("copies")); + let other = collect_reverse_contributions(&importer, "other", &imported); + assert!(!other.referenced_fields.contains("copies")); + } + + #[test] + fn reverse_contributions_ignore_a_field_the_importer_never_reads() { + let imported = + module_of("entity Shelf {\n copies: Copy\n catalogue_key: String\n}\n"); + // Precision: crediting one field of a bound entity must not credit the + // entity's other fields, or the warning stops meaning anything. + let importer = module_of( + "use \"./s.allium\" as shelves\nsurface ShelfView {\n context shelf: shelves/Shelf\n exposes:\n shelf.copies\n}\n", + ); + let rc = collect_reverse_contributions(&importer, "shelves", &imported); + assert!(rc.referenced_fields.contains("copies")); + assert!(!rc.referenced_fields.contains("catalogue_key")); + } + #[test] fn reverse_contributions_filter_undeclared_status() { let imported = module_of("entity Ticket {\n status: open | closed\n}\n"); diff --git a/crates/allium/tests/cross_module_lifecycle.rs b/crates/allium/tests/cross_module_lifecycle.rs index a9a525f..7ba3324 100644 --- a/crates/allium/tests/cross_module_lifecycle.rs +++ b/crates/allium/tests/cross_module_lifecycle.rs @@ -1110,3 +1110,188 @@ fn bad_alias_on_a_collection_is_still_diagnosed() { "an unknown alias on a collection must still be diagnosed.\n{stdout}" ); } + +// =========================================================================== +// A field read across the boundary through a *bound* name. A surface writes +// `context shelf: shelves/Shelf` and reads `shelf.copies`; a rule subscribing +// to a trigger that surface provides reads the same relationship off its own +// trigger binding. The qualified spelling `shelves/Shelf.copies` appears at +// neither site, so the qualified pass credits nothing and the imported module +// reports a field unused that its only reader iterates. Found downstream in +// friend-mesh, where `Group.messages` is read exactly this way. +// +// Same oracles as everywhere above: the merged single file is the control, the +// imported module alone keeps its warning, and crediting needs a real `use` +// edge. +// =========================================================================== + +fn unused_fields(stdout: &str) -> Vec { + parse_diagnostics(stdout) + .into_iter() + .filter(|d| d.code == "allium.field.unused") + .map(|d| d.message) + .collect() +} + +const SHELF_BOUND: &str = r#"-- allium: 3 + +entity Shelf { + name: String + copies: Copy with shelf = this + catalogue_key: String +} + +entity Copy { + title: String + shelf: Shelf +} +"#; + +const CONSOLE_BOUND: &str = r#"-- allium: 3 + +use "./shelf.allium" as shelves + +surface ShelfView { + context shelf: shelves/Shelf + + exposes: + shelf.name + shelf.copies +} +"#; + +const MERGED_BOUND: &str = r#"-- allium: 3 + +entity Shelf { + name: String + copies: Copy with shelf = this + catalogue_key: String +} + +entity Copy { + title: String + shelf: Shelf +} + +surface ShelfView { + context shelf: Shelf + + exposes: + shelf.name + shelf.copies +} +"#; + +#[test] +fn bound_context_read_credits_imported_field() { + let dir = TempDir::new("bound-pair"); + dir.write("shelf.allium", SHELF_BOUND); + dir.write("console.allium", CONSOLE_BOUND); + + let (_ok, stdout) = run("check", &[dir.path().to_str().unwrap()]); + assert!( + !unused_fields(&stdout).iter().any(|m| m.contains("Shelf.copies")), + "`shelf.copies` is read off a context binding typed to the imported entity.\n{stdout}" + ); +} + +#[test] +fn bound_read_leaves_an_unreferenced_field_reported() { + // Precision: crediting `copies` must not credit the rest of the entity. + let dir = TempDir::new("bound-precision"); + dir.write("shelf.allium", SHELF_BOUND); + dir.write("console.allium", CONSOLE_BOUND); + + let (_ok, stdout) = run("check", &[dir.path().to_str().unwrap()]); + assert!( + unused_fields(&stdout).iter().any(|m| m.contains("Shelf.catalogue_key")), + "`catalogue_key` is read by nobody and must still be reported.\n{stdout}" + ); +} + +#[test] +fn bound_read_matches_the_merged_single_file_control() { + let pair = TempDir::new("bound-pair-oracle"); + pair.write("shelf.allium", SHELF_BOUND); + pair.write("console.allium", CONSOLE_BOUND); + let (_ok, pair_out) = run("check", &[pair.path().to_str().unwrap()]); + + let one = TempDir::new("bound-merged"); + one.write("merged.allium", MERGED_BOUND); + let (_ok, one_out) = run("check", &[&one.file("merged.allium")]); + + let mut from_pair = unused_fields(&pair_out); + let mut from_one = unused_fields(&one_out); + from_pair.sort(); + from_one.sort(); + assert_eq!( + from_pair, from_one, + "the pair must report exactly what the equivalent one-file spec reports" + ); +} + +#[test] +fn bound_read_imported_module_alone_keeps_its_warning() { + let dir = TempDir::new("bound-alone"); + dir.write("shelf.allium", SHELF_BOUND); + + let (_ok, stdout) = run("check", &[&dir.file("shelf.allium")]); + assert!( + unused_fields(&stdout).iter().any(|m| m.contains("Shelf.copies")), + "checked alone, shelf.allium has no reader for `copies` and must still warn.\n{stdout}" + ); +} + +#[test] +fn gate_no_import_edge_means_no_field_credit() { + // The `gate_no_import_edge_means_no_credit` guard, for the bound read. + let dir = TempDir::new("bound-no-edge"); + dir.write("shelf.allium", SHELF_BOUND); + dir.write( + "unrelated.allium", + "-- allium: 3\n\nsurface Stray {\n context shelf: Shelf\n\n exposes:\n shelf.copies\n}\n", + ); + + let (_ok, stdout) = run("check", &[dir.path().to_str().unwrap()]); + assert!( + unused_fields(&stdout).iter().any(|m| m.contains("Shelf.copies")), + "without a `use` edge, a co-supplied file's bound read must not credit.\n{stdout}" + ); +} + +#[test] +fn rule_trigger_binding_read_credits_imported_field() { + // The read is in a rule: `shelf` is bound by the trigger, which the + // importer's own surface typed by passing its `context` binding as the + // argument. This is friend-mesh's `Group.members` shape. + let dir = TempDir::new("bound-rule"); + dir.write("shelf.allium", SHELF_BOUND); + dir.write( + "console.allium", + r#"-- allium: 3 + +use "./shelf.allium" as shelves + +surface Desk { + context shelf: shelves/Shelf + + provides: + Audit(shelf) +} + +rule DoAudit { + when: Audit(shelf) + + ensures: + for c in shelf.copies: + Noted(copy: c) +} +"#, + ); + + let (_ok, stdout) = run("check", &[dir.path().to_str().unwrap()]); + assert!( + !unused_fields(&stdout).iter().any(|m| m.contains("Shelf.copies")), + "a rule binding typed by its trigger reads `shelf.copies`.\n{stdout}" + ); +} diff --git a/docs/project/specs/allium-check-tool-behaviour.allium b/docs/project/specs/allium-check-tool-behaviour.allium index 5aa94bb..c6afe9f 100644 --- a/docs/project/specs/allium-check-tool-behaviour.allium +++ b/docs/project/specs/allium-check-tool-behaviour.allium @@ -465,6 +465,28 @@ rule ImporterOwnedTriggerWitnessCreditsImportedLifecycle { ensures: FindingNotObserved(code: "allium.status.noExit", status: source) } +rule BoundContextReadCreditsImportedFieldUse { + when: CheckCommandInvokedWithInputs(inputs) + requires: inputs resolve to multiple files + requires: file_a declares entity with field + requires: file_b has use declaration mapping alias to file_a + requires: file_b surface declares a context or facing binding typed alias/entity + requires: file_b surface reads field off that binding by member access + + ensures: FindingNotObserved(code: "allium.field.unused", field: field) +} + +rule TriggerBindingReadCreditsImportedFieldUse { + when: CheckCommandInvokedWithInputs(inputs) + requires: inputs resolve to multiple files + requires: file_a declares entity with field + requires: file_b has use declaration mapping alias to file_a + requires: file_b surface declares a context binding typed alias/entity and provides a trigger carrying it + requires: file_b rule subscribes to that trigger and reads field off the carried binding + + ensures: FindingNotObserved(code: "allium.field.unused", field: field) +} + rule ImportedModuleAloneKeepsLocalLifecycleWarnings { when: CheckCommandInvokedWithInputs(inputs) requires: inputs resolve to a single file