diff --git a/cli/tests/ui/binding_interpolation_unknown_binding.json5 b/cli/tests/ui/binding_interpolation_unknown_binding.json5 index 0b78461..66a0779 100644 --- a/cli/tests/ui/binding_interpolation_unknown_binding.json5 +++ b/cli/tests/ui/binding_interpolation_unknown_binding.json5 @@ -13,13 +13,13 @@ ], }, }, + components: { + provider: "./simple-provider.json5", + }, slots: { backend: { kind: "http" }, }, - provides: { - api: { kind: "http", endpoint: "http" }, - }, bindings: [ - { name: "backend", to: "self.backend", from: "self.api" }, + { name: "backend", to: "self.backend", from: "#provider.api" }, ], } diff --git a/cli/tests/ui/binding_self.json5 b/cli/tests/ui/binding_self.json5 new file mode 100644 index 0000000..a6a5230 --- /dev/null +++ b/cli/tests/ui/binding_self.json5 @@ -0,0 +1,21 @@ +{ + manifest_version: "0.1.0", + program: { + image: "ghcr.io/acme/app:v1", + entrypoint: ["app"], + network: { + endpoints: [ + { name: "http", port: 8080 }, + ], + }, + }, + slots: { + backend: { kind: "http" }, + }, + provides: { + api: { kind: "http", endpoint: "http" }, + }, + bindings: [ + { to: "self.backend", from: "self.api" }, + ], +} diff --git a/cli/tests/ui/binding_self.stderr b/cli/tests/ui/binding_self.stderr new file mode 100644 index 0000000..586db40 --- /dev/null +++ b/cli/tests/ui/binding_self.stderr @@ -0,0 +1,12 @@ +Error: manifest::self_binding + + × check failed + ╰─▶ binding connects `self` to itself + ╭─[/tests/ui/binding_self.json5:19:5] + 18 │ bindings: [ + 19 │ { to: "self.backend", from: "self.api" }, + · ────────────────────┬─────────────────── + · ╰── self-binding here + 20 │ ], + ╰──── + help: bindings must connect different components diff --git a/cli/tests/ui/simple-provider.json5 b/cli/tests/ui/simple-provider.json5 new file mode 100644 index 0000000..26d9909 --- /dev/null +++ b/cli/tests/ui/simple-provider.json5 @@ -0,0 +1,18 @@ +{ + manifest_version: "0.1.0", + program: { + image: "ghcr.io/acme/provider:v1", + entrypoint: ["provider"], + network: { + endpoints: [ + { name: "http", port: 8080 }, + { name: "agent", port: 8081 }, + ], + }, + }, + provides: { + api: { kind: "http", endpoint: "http" }, + agent: { kind: "a2a", endpoint: "agent" }, + }, + exports: { api: "api", agent: "agent" }, +} diff --git a/cli/tests/ui/slot_interpolation_invalid_field.json5 b/cli/tests/ui/slot_interpolation_invalid_field.json5 index 643c84e..6870820 100644 --- a/cli/tests/ui/slot_interpolation_invalid_field.json5 +++ b/cli/tests/ui/slot_interpolation_invalid_field.json5 @@ -13,13 +13,13 @@ ], }, }, + components: { + provider: "./simple-provider.json5", + }, slots: { agent: { kind: "a2a" }, }, - provides: { - agent_provider: { kind: "a2a", endpoint: "agent" }, - }, bindings: [ - { to: "self.agent", from: "self.agent_provider" }, + { to: "self.agent", from: "#provider.agent" }, ], } diff --git a/compiler/src/reporter/docker_compose/mod.rs b/compiler/src/reporter/docker_compose/mod.rs index af98361..0d5e63b 100644 --- a/compiler/src/reporter/docker_compose/mod.rs +++ b/compiler/src/reporter/docker_compose/mod.rs @@ -298,9 +298,6 @@ fn render_docker_compose_inner(output: &CompileOutput) -> DcResult { continue; } let from = binding_from_component(&b.from); - if from.component == b.to.component { - continue; - } strong_deps .entry(b.to.component) .or_default() @@ -371,20 +368,16 @@ fn render_docker_compose_inner(output: &CompileOutput) -> DcResult { ) })?; - let remote_host = if provider == consumer { - "127.0.0.1".to_string() - } else { - names - .get(&provider) - .ok_or_else(|| { - format!( - "internal error: missing sidecar name for provider {}", - component_label(s, provider) - ) - })? - .sidecar - .clone() - }; + let remote_host = names + .get(&provider) + .ok_or_else(|| { + format!( + "internal error: missing sidecar name for provider {}", + component_label(s, provider) + ) + })? + .sidecar + .clone(); slot_proxies_by_component .entry(consumer) diff --git a/compiler/src/tests.rs b/compiler/src/tests.rs index 031b2e4..e8618eb 100644 --- a/compiler/src/tests.rs +++ b/compiler/src/tests.rs @@ -786,25 +786,41 @@ async fn duplicate_slot_bindings_across_manifests_error() { let dir = tmp_dir("scenario-duplicate-slot-binding"); let root_path = dir.path().join("root.json5"); let child_path = dir.path().join("child.json5"); + let provider_path = dir.path().join("provider.json5"); write_file( - &child_path, + &provider_path, r#" { manifest_version: "0.1.0", program: { - image: "child", - entrypoint: ["child"], + image: "provider", + entrypoint: ["provider"], network: { endpoints: [{ name: "endpoint", port: 80 }] }, }, - slots: { api: { kind: "http" } }, provides: { http: { kind: "http", endpoint: "endpoint" } }, - bindings: [ - { to: "self.api", from: "self.http" }, - ], + exports: { http: "http" }, } "#, ); + write_file( + &child_path, + &format!( + r##" + {{ + manifest_version: "0.1.0", + components: {{ + provider: "{provider}", + }}, + slots: {{ api: {{ kind: "http" }} }}, + bindings: [ + {{ to: "self.api", from: "#provider.http" }}, + ], + }} + "##, + provider = file_url(&provider_path), + ), + ); write_file( &root_path, &format!( diff --git a/manifest/README.md b/manifest/README.md index e821a6b..724b6ae 100644 --- a/manifest/README.md +++ b/manifest/README.md @@ -448,6 +448,7 @@ Rules enforced by this crate: * `from: "self"` requires `capability` exist in `provides`. * `from: "framework"` requires a known framework capability name (see below). * `framework` is only valid as a binding source; it cannot appear in `to` or `exports`. +* `to` and `from` must refer to different components (self-bindings are invalid). * Any `#child` referenced in `to` or `from` must exist in `components`. * Slot/capability names must not contain `.`. diff --git a/manifest/src/document.rs b/manifest/src/document.rs index a6383cf..3f32059 100644 --- a/manifest/src/document.rs +++ b/manifest/src/document.rs @@ -131,6 +131,13 @@ fn labels_for_manifest_error(err: &ManifestError, spans: &ManifestSpans) -> Vec< ManifestError::DuplicateBindingName { name } => { labels_for_duplicate_binding_name(spans, name) } + ManifestError::SelfBinding { + to, + slot, + from, + capability, + name, + } => labels_for_self_binding(spans, to, slot, from, capability, name.as_deref()), ManifestError::UnknownBindingSlot { slot } => labels_for_unknown_binding_slot(spans, slot), ManifestError::UnknownBindingProvide { capability } => { labels_for_unknown_binding_provide(spans, capability) @@ -350,6 +357,69 @@ fn labels_for_duplicate_binding_name(spans: &ManifestSpans, name: &str) -> Vec, +) -> Vec { + let Some(key) = crate::binding_target_key_for_binding(to, Some(slot)) else { + return vec![primary( + default_span(), + Some("self-binding here".to_string()), + )]; + }; + + let binding = spans.bindings_by_index.iter().find(|b| { + if !binding_name_matches(b, name) { + return false; + } + let target_match = binding_target_key_for_span(b) + .as_ref() + .is_some_and(|k| k == &key); + if !target_match { + return false; + } + binding_source_parts_for_span(b).is_some_and(|(span_from, span_capability)| { + span_from == from && span_capability == capability + }) + }); + + let Some(binding) = binding else { + return vec![primary( + default_span(), + Some("self-binding here".to_string()), + )]; + }; + + vec![primary( + span_or_default(Some(binding.whole)), + Some("self-binding here".to_string()), + )] +} + +fn binding_name_matches(binding: &crate::BindingSpans, name: Option<&str>) -> bool { + match name { + Some(expected) => binding.name_value.as_deref() == Some(expected), + None => binding.name_value.is_none(), + } +} + +fn binding_source_parts_for_span(span: &crate::BindingSpans) -> Option<(String, String)> { + if let (Some(from), Some(capability)) = + (span.from_value.as_deref(), span.capability_value.as_deref()) + { + let source = crate::parse_binding_source_ref(from).ok()?; + return Some((source.to_string(), capability.to_string())); + } + + let from = span.from_value.as_deref()?; + let (source, capability) = crate::split_binding_source(from).ok()?; + Some((source.to_string(), capability)) +} + fn binding_span_or_default( spans: &ManifestSpans, choose: impl FnMut(&crate::BindingSpans) -> Option, diff --git a/manifest/src/lib.rs b/manifest/src/lib.rs index 73c1858..71ceb7f 100644 --- a/manifest/src/lib.rs +++ b/manifest/src/lib.rs @@ -123,6 +123,19 @@ pub enum Error { #[diagnostic(code(manifest::duplicate_binding_name))] DuplicateBindingName { name: String }, + #[error("binding connects `{to}` to itself")] + #[diagnostic( + code(manifest::self_binding), + help("bindings must connect different components") + )] + SelfBinding { + to: String, + slot: String, + from: String, + capability: String, + name: Option, + }, + #[error("binding references unknown child `#{child}`")] #[diagnostic(code(manifest::unknown_binding_child))] UnknownBindingChild { child: String }, @@ -1618,6 +1631,43 @@ fn resolve_binding_source( } } +fn binding_target_parts(target: &BindingTarget) -> (String, String) { + let to = match target { + BindingTarget::SelfSlot(_) => "self".to_string(), + BindingTarget::ChildSlot { child, .. } => format!("#{child}"), + }; + let slot = match target { + BindingTarget::SelfSlot(name) => name.to_string(), + BindingTarget::ChildSlot { slot, .. } => slot.to_string(), + }; + (to, slot) +} + +fn binding_source_parts(source: &BindingSource) -> (String, String) { + match source { + BindingSource::SelfProvide(name) => ("self".to_string(), name.to_string()), + BindingSource::ChildExport { child, export } => (format!("#{child}"), export.to_string()), + BindingSource::Framework(name) => ("framework".to_string(), name.to_string()), + } +} + +fn is_self_binding(target: &BindingTarget, source: &BindingSource) -> bool { + match (target, source) { + (BindingTarget::SelfSlot(_), BindingSource::SelfProvide(_)) => true, + ( + BindingTarget::ChildSlot { + child: target_child, + .. + }, + BindingSource::ChildExport { + child: source_child, + .. + }, + ) => target_child == source_child, + _ => false, + } +} + fn build_bindings( bindings: BTreeSet, ctx: &ValidateCtx<'_>, @@ -1660,16 +1710,21 @@ fn build_bindings( let target = resolve_binding_target(ctx, to, slot)?; let source = resolve_binding_source(ctx, from, capability)?; + let (to, slot) = binding_target_parts(&target); + + if is_self_binding(&target, &source) { + let (from, capability) = binding_source_parts(&source); + let name_value = name.as_ref().map(|name| name.to_string()); + return Err(Error::SelfBinding { + to, + slot, + from, + capability, + name: name_value, + }); + } if bindings_out.contains_key(&target) { - let to = match &target { - BindingTarget::SelfSlot(_) => "self".to_string(), - BindingTarget::ChildSlot { child, .. } => format!("#{child}"), - }; - let slot = match &target { - BindingTarget::SelfSlot(name) => name.to_string(), - BindingTarget::ChildSlot { slot, .. } => slot.to_string(), - }; return Err(Error::DuplicateBindingTarget { to, slot }); } diff --git a/manifest/src/tests.rs b/manifest/src/tests.rs index d965972..69c714a 100644 --- a/manifest/src/tests.rs +++ b/manifest/src/tests.rs @@ -1937,27 +1937,19 @@ fn manifest_doc_error_unknown_export_target_points_to_target() { #[test] fn manifest_doc_error_duplicate_binding_target_marks_second_binding() { - let source = r#" + let source = r##" { manifest_version: "0.1.0", - program: { - image: "x", - entrypoint: ["x"], - network: { endpoints: [{ name: "a", port: 80 }, { name: "b", port: 81 }] }, - }, - slots: { - api: { kind: "http" }, - }, - provides: { - a: { kind: "http", endpoint: "a" }, - b: { kind: "http", endpoint: "b" }, + components: { + a: "https://example.com/a", + b: "https://example.com/b", }, bindings: [ - { to: "self.api", from: "self.a" }, - { to: "self.api", from: "self.b" }, + { to: "#a.api", from: "#b.a" }, + { to: "#a.api", from: "#b.b" }, ], } - "#; + "##; let source: Arc = Arc::from(source); let err = ParsedManifest::parse_named("", Arc::clone(&source)).unwrap_err(); assert!(matches!(err.kind, Error::DuplicateBindingTarget { .. })); @@ -1968,7 +1960,36 @@ fn manifest_doc_error_duplicate_binding_target_marks_second_binding() { .find(|label| label.label() == Some("second binding here")) .expect("second binding label"); let second_text = labeled_span_text(source.as_ref(), second); - assert!(second_text.contains("self.b")); + assert!(second_text.contains("#b.b")); +} + +#[test] +fn manifest_doc_error_self_binding_marks_self_binding() { + let source = r##" + { + manifest_version: "0.1.0", + components: { + child: "https://example.com/child", + provider: "https://example.com/provider", + }, + bindings: [ + { to: "#child.api", from: "#provider.api" }, + { to: "#child", slot: "api", from: "#child", capability: "api" }, + ], + } + "##; + let source: Arc = Arc::from(source); + let err = ParsedManifest::parse_named("", Arc::clone(&source)).unwrap_err(); + assert!(matches!(err.kind, Error::SelfBinding { .. })); + + let labels: Vec<_> = err.labels().expect("labels").collect(); + let binding = labels + .iter() + .find(|label| label.label() == Some("self-binding here")) + .expect("self-binding label"); + let binding_text = labeled_span_text(source.as_ref(), binding); + assert!(binding_text.contains("capability: \"api\"")); + assert!(!binding_text.contains("#provider.api")); } #[test] diff --git a/scenario/src/graph.rs b/scenario/src/graph.rs index 8d6d364..9c623d5 100644 --- a/scenario/src/graph.rs +++ b/scenario/src/graph.rs @@ -12,7 +12,6 @@ pub struct CycleError { /// if A provides something bound into B's slot, A must come before B. /// /// Notes: -/// - Self-bindings are ignored. /// - Weak edges are ignored for ordering and cycle detection (they can point "backwards" without creating a dependency cycle). /// - This is intentionally separate from Scenario (graph ops live here). pub fn topo_order(s: &Scenario) -> Result, CycleError> { @@ -35,9 +34,6 @@ pub fn topo_order(s: &Scenario) -> Result, CycleError> { }; let u = from.component.0; let v = b.to.component.0; - if u == v { - continue; - } out[u].push(v); } @@ -99,7 +95,7 @@ pub fn providers_of(s: &Scenario, id: ComponentId) -> BTreeSet { let BindingFrom::Component(from) = &b.from else { continue; }; - if b.to.component == id && from.component != id { + if b.to.component == id { set.insert(from.component); } }