From 59040c331ce2875dfbfb96416d6435466797ca15 Mon Sep 17 00:00:00 2001 From: Matthew Gibbons Date: Fri, 14 Aug 2026 14:54:58 +0000 Subject: [PATCH] P6.x: convert instantiate_provider_expr to bynk-emit::ir's CapRefIr (Provider given/deps wiring) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scope: project.rs's instantiate_provider_expr (the deps-object constructor for a `provides` binding, both Bynk-authored and external/adapter-supplied) now reads a provider's own given clause through bynk-emit::ir::CapRefIr instead of walking bynk_syntax::ast::CapRef directly. Byte-identical output confirmed across the entire fixture suite. Real fix found along the way, not just a data-source swap: ProviderBody:: External was a bare unit variant carrying no given field at all, silently dropping an external provider's own given clause — despite its own doc comment already claiming "given lowers unconditionally" (P6.14, #1186). ProviderDecl::given is populated the same way regardless of external (nothing in the grammar or checker gates it), so this was a real gap between the type's documented intent and its actual shape, not a deliberate omission. ProviderBody::External now carries given: Vec too, and lower_provider_item_ir populates it for both branches via the new lower_provider_given_ir. Why a standalone lower_provider_given_ir, not the full IrItem::Provider: building a real IrItem::Provider at project.rs's call site would be unsafe for a Bynk provider specifically — ProviderBody::Bynk::ops unconditionally lowers every op's body through lower_provider_op_ir -> lower_expr_ir, which still can't handle Ok/Err/Some/None construction (the same gap that made Agent/Service's own full IrItem construction impractical at their emitter call sites, #1196/#1198). lower_provider_given_ir never touches ops/bodies, mirroring #1198's own lower_service_handler_signature_ir precedent: a narrow standalone reader, not the full assembly. What's not changed: plan_agent_given_deps (project.rs) stays AST-driven — it's Agent handler given, a separate concern from Provider given, and IrHandler::given is bare names only (no context/prefix data), blocked on Agent's own still-deferred given-with-context IR work. Raw CapRef/ ProviderDecl.given reads persist elsewhere too (emitter/workers.rs's own deps construction and cross-context scan, project.rs's own second given scan, and EmitProjectCtx::agent_method_givens, which stores raw Vec directly) — this slice does not remove bynk_syntax::ast::CapRef coupling from the crate, only from this one call site. ast_importers unaffected (still 8). New test coverage: lower_provider_item_ir's own external-provider test now exercises a non-empty given (previously always empty), pinning that ProviderBody::External carries it correctly where it used to be silently dropped. Part of #1137. A step toward project.rs's own eventual slice 6 cleanup (design/tracks/the-ir.md §6, per the reconciliation in a companion PR) — not slice 6 itself, and does not claim to be. Co-Authored-By: Claude Sonnet 5 --- bynk-emit/src/ir.rs | 11 +++- bynk-emit/src/ir/lower.rs | 59 +++++++++++++++---- bynk-emit/src/project.rs | 22 ++++--- .../pending/p6-provider-given-deps-wiring.md | 4 ++ 4 files changed, 75 insertions(+), 21 deletions(-) create mode 100644 design/pending/p6-provider-given-deps-wiring.md diff --git a/bynk-emit/src/ir.rs b/bynk-emit/src/ir.rs index 4b7e62fc..4af4951a 100644 --- a/bynk-emit/src/ir.rs +++ b/bynk-emit/src/ir.rs @@ -880,7 +880,16 @@ pub(crate) enum ProviderBody { /// `provides Cap = Name` with no brace block — the adapter's own /// binding supplies the implementation; the emitter produces no class /// (`bynk-greenfield-compiler.md:1310`, `Provider{External} | nothing`). - External, + /// Carries `given` too (added #1187's Provider `given`/deps-wiring + /// slice, correcting a real gap this bare-unit shape left): an external + /// provider's own `given` clause is populated the same way `Bynk`'s is + /// (`ProviderDecl::given` is not gated on `external` anywhere in the + /// grammar or checker) and `instantiate_provider_expr` + /// (`bynk-emit/src/project.rs`) needs it to build an external + /// provider's own `deps` constructor argument — this variant's own doc + /// comment already claimed `given` "lowers unconditionally" (P6.14's + /// own review of #1186) before this fix actually made that true. + External { given: Vec }, } /// P6.14's real `CapRefIr` ([DECISION A], #1174, review of #1186) — one diff --git a/bynk-emit/src/ir/lower.rs b/bynk-emit/src/ir/lower.rs index 6595aa6a..8b3d621e 100644 --- a/bynk-emit/src/ir/lower.rs +++ b/bynk-emit/src/ir/lower.rs @@ -1543,16 +1543,19 @@ fn lower_op_sig_ir(op: &CapabilityOp, program: &CheckedProgram) -> OpSig { /// this, unlike `Actor`, was buildable this slice). `external: true` means /// `ops` is empty by the field's own doc comment /// (`bynk-syntax/src/ast.rs:592-595`) — nothing to lower. `given` lowers -/// unconditionally (review of #1186): `provider.given` is populated the -/// same way regardless of `external` (a provider's own dependency list, -/// not something the brace block gates), and [`ProviderBody::Bynk`]'s own -/// doc comment names why it, unlike `module`, is not deferrable. +/// unconditionally via [`lower_provider_given_ir`] (#1187's Provider +/// `given`/deps-wiring slice, fixing a real gap the `External` variant's own +/// bare-unit shape used to leave: `provider.given` is populated the same way +/// regardless of `external`, and [`ProviderBody::Bynk`]'s own doc comment +/// names why it, unlike `module`, is not deferrable — that reasoning always +/// applied to `External` too, just wasn't wired through). pub(crate) fn lower_provider_item_ir(provider: &ProviderDecl, program: &CheckedProgram) -> IrItem { + let given = lower_provider_given_ir(provider); let body = if provider.external { - ProviderBody::External + ProviderBody::External { given } } else { ProviderBody::Bynk { - given: provider.given.iter().map(lower_cap_ref_ir).collect(), + given, ops: provider .ops .iter() @@ -1567,6 +1570,24 @@ pub(crate) fn lower_provider_item_ir(provider: &ProviderDecl, program: &CheckedP } } +/// A provider's own `given` clause, resolved independent of +/// [`ProviderBody`]'s `external`/`Bynk` dispatch and independent of +/// [`lower_provider_item_ir`]'s own full assembly (#1187's Provider +/// `given`/deps-wiring slice) — the standalone entry point +/// `bynk-emit/src/project.rs`'s `instantiate_provider_expr` actually calls. +/// Building a real `IrItem::Provider` there would be unsafe for a `Bynk` +/// provider specifically: `ProviderBody::Bynk::ops` unconditionally lowers +/// every op's body through `lower_provider_op_ir` → `lower_expr_ir`, which +/// still can't handle `Ok`/`Err`/`Some`/`None` construction — the identical +/// gap that made `Agent`/`Service`'s own full `IrItem` construction +/// impractical at their emitter call sites (#1196/#1198's own findings). +/// This function never touches `ops`/bodies, so it carries none of that +/// risk; [`lower_provider_item_ir`] itself now calls it too, rather than +/// hand-duplicating the one-line `map`. +pub(crate) fn lower_provider_given_ir(provider: &ProviderDecl) -> Vec { + provider.given.iter().map(lower_cap_ref_ir).collect() +} + /// P6.14 (#1174, review of #1186): adapt one `given` entry into a real /// [`crate::ir::CapRefIr`] — [`CapRefIr`]'s own doc comment has the full /// grounding for the `QualifiedName -> String` flattening and for why a @@ -7414,7 +7435,20 @@ provides Store = MemStore given Random, Clock { name: "ExternalStore".to_string(), span: Span::default(), }, - given: Vec::new(), + // Review of #1187's own Provider given/deps-wiring slice: an + // external provider's own `given` is populated the same way a + // Bynk one's is (nothing in the grammar or checker gates it on + // `external`) — non-empty here specifically to pin that + // `ProviderBody::External` now carries it, where it used to be + // silently dropped (a bare-unit variant with nowhere to put it). + given: vec![CapRef { + context: None, + name: bynk_syntax::ast::Ident { + name: "Clock".to_string(), + span: Span::default(), + }, + span: Span::default(), + }], ops: Vec::new(), external: true, documentation: None, @@ -7428,11 +7462,12 @@ provides Store = MemStore given Random, Clock { }; assert_eq!(def, "ExternalStore"); assert_eq!(cap, "Store"); - assert!( - matches!(body, ProviderBody::External), - "expected ProviderBody::External, got {:?}", - body - ); + let ProviderBody::External { given } = body else { + panic!("expected ProviderBody::External, got {:?}", body) + }; + assert_eq!(given.len(), 1); + assert_eq!(given[0].context, None); + assert_eq!(given[0].name, "Clock"); } #[test] diff --git a/bynk-emit/src/project.rs b/bynk-emit/src/project.rs index ffb8b982..78bf964f 100644 --- a/bynk-emit/src/project.rs +++ b/bynk-emit/src/project.rs @@ -25,6 +25,8 @@ use std::path::{Component, Path, PathBuf}; use std::sync::Arc; use crate::emitter; +use crate::ir::CapRefIr; +use crate::ir::lower::lower_provider_given_ir; use bynk_check::check_pipeline::{self, prepare_unit_check_ctx}; use bynk_check::checker; use bynk_check::checker::{TyId, Types}; @@ -2636,7 +2638,12 @@ pub(crate) fn instantiate_provider_expr( return format!("new {bodied_ns}.{cap}()"); }; // Build the by-name deps object from the provider's `given`, if any. - let deps_obj = if provider.given.is_empty() { + // #1187's Provider given/deps-wiring slice: reads bynk-emit::ir's own + // CapRefIr (lower_provider_given_ir — a standalone reader, never a full + // IrItem::Provider; see that function's own doc comment for why) instead + // of walking bynk_syntax::ast::CapRef directly. + let given: Vec = lower_provider_given_ir(provider); + let deps_obj = if given.is_empty() { None } else { let consumed = unit_consumes.get(provider_ctx).cloned().unwrap_or_default(); @@ -2648,21 +2655,20 @@ pub(crate) fn instantiate_provider_expr( .get(provider_ctx) .cloned() .unwrap_or_default(); - let deps: Vec = provider - .given + let deps: Vec = given .iter() .map(|g| { - let target_ctx = match g.prefix() { - Some(p) => resolve_consume_prefix(&p, &consumed, &aliases) + let target_ctx = match &g.context { + Some(p) => resolve_consume_prefix(p, &consumed, &aliases) .unwrap_or_else(|| provider_ctx.to_string()), None => flattened - .get(g.key()) + .get(&g.name) .cloned() .unwrap_or_else(|| provider_ctx.to_string()), }; let expr = instantiate_provider_expr( &target_ctx, - g.key(), + &g.name, unit_tables, unit_consumes, unit_consumes_aliases, @@ -2672,7 +2678,7 @@ pub(crate) fn instantiate_provider_expr( locale_negotiation, referenced_units, ); - format!("{}: {}", g.key(), expr) + format!("{}: {}", g.name, expr) }) .collect(); Some(format!("{{ {} }}", deps.join(", "))) diff --git a/design/pending/p6-provider-given-deps-wiring.md b/design/pending/p6-provider-given-deps-wiring.md new file mode 100644 index 00000000..e9639904 --- /dev/null +++ b/design/pending/p6-provider-given-deps-wiring.md @@ -0,0 +1,4 @@ +--- +level: patch +changelog: "project.rs's instantiate_provider_expr now reads a provider's given clause from bynk-emit::ir (CapRefIr) instead of walking bynk_syntax::ast::CapRef directly; ProviderBody::External also gains the given field it was silently dropping (internal only, byte-identical output)" +---