refactor(core): make node build bind maps conditionally (#34218)

packages/core/src/effect/layer-node/layer-node-tree.ts

@@ -3,6 +3,52 @@ import { LayerNode } from "./layer-node"
 
 type AnyNode = LayerNode.Node<unknown, unknown, any>
 type RuntimeLayer = Layer.Layer<never, unknown, unknown>
+type Visit<Result> = (node: AnyNode, context: VisitContext<Result>) => Result
+
+type VisitContext<Result> = {
+  readonly cache: Map<AnyNode, Result>
+  readonly visit: (node: AnyNode) => Result
+}
+
+function walk<Result>(
+  root: AnyNode,
+  visit: Visit<Result>,
+  options: {
+    readonly cache?: Map<AnyNode, Result>
+    readonly resolve?: (node: AnyNode) => AnyNode
+    readonly detectCycles?: boolean
+  } = {},
+) {
+  const cache = options.cache ?? new Map<AnyNode, Result>()
+  const visiting = new Set<AnyNode>()
+  const stack: AnyNode[] = []
+
+  const recur = (node: AnyNode): Result => {
+    const target = options.resolve?.(node) ?? node
+    const cached = cache.get(target)
+    if (cached !== undefined || cache.has(target)) return cached!
+
+    if (options.detectCycles !== false && visiting.has(target)) {
+      const start = stack.indexOf(target)
+      throw new Error(
+        `Cycle detected in layer tree: ${[...stack.slice(start), target].map((item) => item.name).join(" -> ")}`,
+      )
+    }
+
+    visiting.add(target)
+    stack.push(target)
+    try {
+      const result = visit(target, { cache, visit: recur })
+      if (!cache.has(target)) cache.set(target, result)
+      return result
+    } finally {
+      stack.pop()
+      visiting.delete(target)
+    }
+  }
+
+  return recur(root)
+}
 
 export function hoist<A, E, T extends LayerNode.Tag>(
   root: LayerNode.Node<A, E, any>,
@@ -11,54 +57,28 @@ export function hoist<A, E, T extends LayerNode.Tag>(
   readonly node: LayerNode.Node<A, E>
   readonly hoisted: LayerNode.Node<unknown, E>
 } {
-  const visited = new Map<AnyNode, AnyNode>()
   const hoisted = new Map<string, AnyNode>()
-  const visiting = new Set<AnyNode>()
-  const stack: AnyNode[] = []
 
-  const visit = (node: AnyNode): AnyNode => {
+  const node = walk<AnyNode>(root, (node, context) => {
     if (node.kind === "group") {
-      return { ...node, dependencies: node.dependencies.map(visit) }
+      return { ...node, dependencies: node.dependencies.map(context.visit) }
     }
-
-    const existingNode = visited.get(node)
-    if (existingNode) return existingNode
-
     if (node.tag === tag) {
       const existing = hoisted.get(node.name)
       if (existing && existing !== node) {
         throw new Error(`Tag ${tag} has conflicting implementations for ${node.name}`)
       }
       hoisted.set(node.name, node)
-      const empty = LayerNode.group([])
-      visited.set(node, empty)
-      return empty
+      return LayerNode.group([])
     }
     if (node.kind === "unbound") {
       return node
     }
-
-    if (visiting.has(node)) {
-      const start = stack.indexOf(node)
-      throw new Error(
-        `Cycle detected in layer tree: ${[...stack.slice(start), node].map((item) => item.name).join(" -> ")}`,
-      )
-    }
-    visiting.add(node)
-    stack.push(node)
-    try {
-      const dependencies = node.dependencies.map(visit)
-      const clone = { ...node, dependencies }
-      visited.set(node, clone)
-      return clone
-    } finally {
-      stack.pop()
-      visiting.delete(node)
-    }
-  }
+    return { ...node, dependencies: node.dependencies.map(context.visit) }
+  })
 
   return {
-    node: visit(root) as LayerNode.Node<A, E>,
+    node: node as LayerNode.Node<A, E>,
     hoisted: LayerNode.group(Array.from(hoisted.values())) as LayerNode.Node<unknown, E>,
   }
 }
@@ -68,24 +88,32 @@ export function compile<A, E>(
   replacements?: ReadonlyMap<Layer.Any, Layer.Any>,
 ): Layer.Layer<A, E> {
   const cache = new Map<AnyNode, RuntimeLayer>()
-  const compileNode = (node: AnyNode): RuntimeLayer => {
-    if (node.kind === "unbound") throw new Error(`Unbound layer node: ${node.name}`)
-    const cached = cache.get(node)
-    if (cached) return cached
-    const dependencies = node.dependencies.flatMap(flatten).map(compileNode)
-    const implementation = (replacements?.get(node.implementation!) ?? node.implementation!) as RuntimeLayer
-    const layer =
-      dependencies.length === 0
-        ? implementation
-        : implementation.pipe(Layer.provide(dependencies as [RuntimeLayer, ...RuntimeLayer[]]))
-    cache.set(node, layer)
-    return layer
-  }
+  const compileNode = (node: AnyNode) =>
+    walk<RuntimeLayer>(
+      node,
+      (node, context) => {
+        if (node.kind === "unbound") throw new Error(`Unbound layer node: ${node.name}`)
+        const dependencies = node.dependencies.flatMap(flatten).map(context.visit)
+        const implementation = (replacements?.get(node.implementation!) ?? node.implementation!) as RuntimeLayer
+        return dependencies.length === 0
+          ? implementation
+          : implementation.pipe(Layer.provide(dependencies as [RuntimeLayer, ...RuntimeLayer[]]))
+      },
+      { cache },
+    )
   const layers = flatten(root).map((node) => compileNode(node))
   const layer = layers.reduce<RuntimeLayer>((result, layer) => layer.pipe(Layer.provideMerge(result)), Layer.empty)
   return layer as Layer.Layer<A, E>
 }
 
+export function hasUnbound(root: LayerNode.Node<unknown, unknown, any>, source: AnyNode): boolean {
+  if (source.kind !== "unbound") throw new Error(`Cannot check non-unbound layer node: ${source.name}`)
+  return walk<boolean>(root, (node, context) => {
+    if (node === source) return true
+    return node.dependencies.some(context.visit)
+  })
+}
+
 export function bind<A, E, T extends LayerNode.Tag | undefined>(
   root: LayerNode.Node<A, E, T>,
   source: AnyNode,
@@ -98,17 +126,18 @@ export function bind<A, E, T extends LayerNode.Tag | undefined>(
   if (source.tag !== replacement.tag) {
     throw new Error(`Cannot bind ${source.name} across tags`)
   }
-  const visited = new Map<AnyNode, AnyNode>()
-  const visit = (node: AnyNode): AnyNode => {
-    if (node === source) return replacement
-    const existing = visited.get(node)
-    if (existing) return existing
-    if (node.kind === "unbound") return node
-    const clone = { ...node, dependencies: node.dependencies.map(visit) }
-    visited.set(node, clone)
-    return clone
-  }
-  return visit(root) as LayerNode.Node<A, E, T>
+  return walk<AnyNode>(
+    root,
+    (target, context) => {
+      if (target.kind === "unbound") return target
+      const dependencies: AnyNode[] = []
+      const clone = { ...target, dependencies }
+      context.cache.set(target, clone)
+      dependencies.push(...target.dependencies.map(context.visit))
+      return clone
+    },
+    { detectCycles: false, resolve: (node) => (node === source ? replacement : node) },
+  ) as LayerNode.Node<A, E, T>
 }
 
 function flatten(node: AnyNode): readonly AnyNode[] {

packages/core/src/effect/node-build.ts

@@ -7,6 +7,12 @@ import { makeGlobalNode } from "./node"
 export function build<A, E>(root: LayerNode.Node<A, E, any>, replacements?: readonly LayerNode.Replacement[]) {
   const replacementMap = new Map(replacements?.map((item) => [item.source, item.replacement]))
 
+  if (!LayerNodeTree.hasUnbound(root, LocationServiceMap.node)) {
+    // If the location service map is not needed, we shouldn't pull it
+    // in. Compile the graph normally
+    return LayerNodeTree.compile(root, replacementMap)
+  }
+
   const locationMap = buildLocationServiceMap(replacementMap)
   const locationMapNode = makeGlobalNode({ service: LocationServiceMap.Service, layer: locationMap, deps: [] })
 

packages/core/test/effect/layer-node/node-build.test.ts

@@ -1,21 +1,72 @@
 import { describe, expect, test } from "bun:test"
-import { Context, Effect, Layer } from "effect"
-import { LayerNode } from "@opencode-ai/core/effect/layer-node"
+import { Context, Effect, Layer, LayerMap, Option } from "effect"
+import { LayerNode, LayerNodeTree } from "@opencode-ai/core/effect/layer-node"
 import { Node } from "@opencode-ai/core/effect/node"
 import { NodeBuild } from "@opencode-ai/core/effect/node-build"
 import { Location } from "@opencode-ai/core/location"
 import { LocationServiceMap } from "@opencode-ai/core/location-service-map"
+import type { LocationError, LocationServices } from "@opencode-ai/core/location-services"
 import { Project } from "@opencode-ai/core/project"
 import { AbsolutePath } from "@opencode-ai/core/schema"
 import { tmpdir } from "../../fixture/tmpdir"
 
 class Value extends Context.Service<Value, { readonly value: string }>()("test/TagValue") {}
 class Result extends Context.Service<Result, { readonly value: string }>()("test/TagResult") {}
-class Left extends Context.Service<Left, { readonly value: string }>()("test/TagLeft") {}
-class Right extends Context.Service<Right, { readonly value: string }>()("test/TagRight") {}
-class Last extends Context.Service<Last, { readonly value: string }>()("test/TagLast") {}
+class CycleA extends Context.Service<CycleA, {}>()("test/NodeBuildA") {}
+class CycleB extends Context.Service<CycleB, { readonly directory: AbsolutePath }>()("test/NodeBuildB") {}
 
 describe("node build", () => {
+  test("does not build a location service map when the graph does not require it", async () => {
+    const result = Node.makeGlobalNode({
+      service: Result,
+      layer: Layer.succeed(Result, Result.of({ value: "plain" })),
+      deps: [],
+    })
+    const layer = NodeBuild.build(LayerNode.group([result]))
+    const program = Effect.gen(function* () {
+      expect(Option.isNone(yield* Effect.serviceOption(LocationServiceMap.Service))).toBe(true)
+      return (yield* Result).value
+    }).pipe(Effect.provide(layer))
+
+    expect(await Effect.runPromise(program)).toBe("plain")
+  })
+
+  test("detects cycles through a bound location service map", () => {
+    const a = Node.makeGlobalNode({
+      service: CycleA,
+      layer: Layer.effect(CycleA, Effect.as(LocationServiceMap.Service, CycleA.of({}))),
+      deps: [LocationServiceMap.node],
+    })
+    const b = Node.makeGlobalNode({
+      service: CycleB,
+      layer: Layer.effect(
+        CycleB,
+        Effect.map(CycleA, () => CycleB.of({ directory: AbsolutePath.make(process.cwd()) })),
+      ),
+      deps: [a],
+    })
+    const mapEffect = Effect.gen(function* () {
+      const service = yield* CycleB
+      return yield* LayerMap.make(
+        (ref: Location.Ref) =>
+          Layer.succeed(
+            Location.Service,
+            Location.Service.of({
+              directory: ref.directory,
+              workspaceID: ref.workspaceID,
+              project: { id: Project.ID.global, directory: service.directory },
+            }),
+          ),
+        { idleTimeToLive: "1 minute" },
+      )
+    }) as unknown as Effect.Effect<LayerMap.LayerMap<Location.Ref, LocationServices, LocationError>, never, CycleB>
+    const mapLayer = Layer.effect(LocationServiceMap.Service, mapEffect)
+    const map = Node.makeGlobalNode({ service: LocationServiceMap.Service, layer: mapLayer, deps: [b] })
+    const graph = LayerNodeTree.bind(LayerNode.group([a]), LocationServiceMap.node, map)
+
+    expect(() => NodeBuild.build(graph)).toThrow("Cycle detected in layer tree")
+  })
+
   test("shares top-level project with location services", async () => {
     await using tmp = await tmpdir()
     let acquisitions = 0
@@ -37,6 +88,7 @@ describe("node build", () => {
     const program = Effect.gen(function* () {
       yield* Project.Service
       const locations = yield* LocationServiceMap.Service
+      expect(Option.isSome(yield* Effect.serviceOption(LocationServiceMap.Service))).toBe(true)
       return yield* Location.Service.pipe(Effect.provide(locations.get(ref)))
     }).pipe(Effect.provide(layer))
 
@@ -62,58 +114,10 @@ describe("node build", () => {
     })
     const serviceLayer = NodeBuild.build(LayerNode.group([result]))
     const program = Effect.gen(function* () {
+      expect(Option.isNone(yield* Effect.serviceOption(LocationServiceMap.Service))).toBe(true)
       return (yield* Result).value
     }).pipe(Effect.provide(serviceLayer))
 
     expect(await Effect.runPromise(program)).toBe("value")
   })
-
-  test("rebinds same-tag providers without reacquiring them", async () => {
-    let firstAcquisitions = 0
-    const tags = LayerNode.tags({ global: [] })
-    const global = tags.make("global")
-    const first = global({
-      service: Value,
-      layer: Layer.effect(
-        Value,
-        Effect.sync(() => {
-          firstAcquisitions++
-          return Value.of({ value: "first" })
-        }),
-      ),
-      deps: [],
-    })
-    const second = global({ service: Value, layer: Layer.succeed(Value, Value.of({ value: "second" })), deps: [] })
-    const left = global({
-      service: Left,
-      layer: Layer.effect(
-        Left,
-        Effect.map(Value, (value) => Left.of({ value: value.value })),
-      ),
-      deps: [first],
-    })
-    const right = global({
-      service: Right,
-      layer: Layer.effect(
-        Right,
-        Effect.map(Value, (value) => Right.of({ value: value.value })),
-      ),
-      deps: [second],
-    })
-    const last = global({
-      service: Last,
-      layer: Layer.effect(
-        Last,
-        Effect.map(Value, (value) => Last.of({ value: value.value })),
-      ),
-      deps: [first],
-    })
-    const layer = NodeBuild.build(LayerNode.group([left, right, last])) as Layer.Layer<Left | Right | Last>
-    const values = Effect.gen(function* () {
-      return [(yield* Left).value, (yield* Right).value, (yield* Last).value]
-    }).pipe(Effect.provide(layer))
-
-    expect(await Effect.runPromise(values)).toEqual(["first", "second", "first"])
-    expect(firstAcquisitions).toBe(1)
-  })
 })