fix(schema): resolve representation identifiers by AST identity (#6782)

Author: gcanti

.changeset/fix-schema-representation-identifiers.md

@@ -0,0 +1,5 @@
+---
+"effect": patch
+---
+
+SchemaRepresentation: generate references from encoded AST identity, suffix colliding identifiers instead of throwing, and preserve sharing across property-key context. This avoids false-positive duplicate identifier errors while keeping referentially distinct schemas addressable; generated fallback definitions now use the clearer `Encoded` suffix.

packages/effect/SCHEMA.md

@@ -6299,10 +6299,11 @@ const multiDocument = SchemaRepresentation.toRepresentations([
 Repeated structural nodes, identifiers, and recursive schemas are placed in `references`. `toMultiDocument(document)`
 wraps a single document when a compiler requires multiple roots.
 
-An explicit `identifier` uniquely names one schema within a conversion. Reusing the same schema shares its reference, but
-two distinct schemas with the same explicit `identifier` cause `toRepresentation` or `toRepresentations` to throw a
-`Duplicate identifier` error. Internal `~identifier` annotations are allocation hints rather than uniqueness claims; when
-a derived name is already occupied, the generated reference receives a numeric suffix.
+An explicit `identifier` requests a reference name within a conversion. Reusing the same schema shares its reference. Copies
+whose AST fields are referentially identical once property-key context is ignored are canonicalized and also share a
+reference. Otherwise, when referentially distinct schemas request the same name, the first schema keeps it and later schemas
+receive numeric suffixes in encounter order, such as `Value_1` and `Value_2`. Internal `~identifier` annotations are fallback
+allocation hints; their generated names use the `Encoded` suffix and follow the same collision rules.
 
 ## JSON persistence
 

packages/effect/src/Schema.ts

@@ -14964,24 +14964,7 @@ const toCodecJsonASTBase = SchemaAST.applyToSelfOrLastLinkEncoding((ast) => {
 })
 
 /** @internal */
-export const toCodecJsonAST = memoize((ast: SchemaAST.AST): SchemaAST.AST => {
-  const identifier = InternalAnnotations.resolveIdentifier(ast)
-  const out = toCodecJsonASTBase(ast)
-  if (identifier === undefined || out.encoding === undefined) return out
-
-  const encoded = SchemaAST.getLastEncoding(out)
-  if (
-    InternalAnnotations.resolveIdentifier(encoded) !== undefined ||
-    InternalAnnotations.resolveIdentifierFallback(encoded) === identifier
-  ) {
-    return out
-  }
-
-  const annotated = SchemaAST.annotate(encoded, {
-    [InternalAnnotations.IDENTIFIER_FALLBACK_KEY]: identifier
-  })
-  return SchemaAST.applyToSelfOrLastLinkEncoding(() => annotated)(out)
-})
+export const toCodecJsonAST = memoize(toCodecJsonASTBase)
 
 function withoutConstructorDefault(context: SchemaAST.Context): SchemaAST.Context {
   return context.defaultValue === undefined ?

packages/effect/src/internal/schema/toRepresentation.ts

@@ -17,7 +17,7 @@ export function toRepresentation(
 export function toRepresentations(
   asts: readonly [SchemaAST.AST, ...Array<SchemaAST.AST>]
 ): SchemaRepresentation.MultiDocument {
-  return lowerASTs(asts)
+  return fromASTs(asts)
 }
 
 type CheckRepresentationAnnotation = SchemaRepresentation.CheckRepresentationAnnotation<
@@ -37,34 +37,32 @@ function isShareable(ast: SchemaAST.AST): boolean {
 
 interface ReferenceIdentifier {
   readonly identifier: string
-  readonly isFallback: boolean
+  readonly fallback?: string | undefined
 }
 
-function resolveReferenceIdentifier(ast: SchemaAST.AST): ReferenceIdentifier | undefined {
-  const identifier = InternalAnnotations.resolveIdentifier(ast)
-  if (identifier !== undefined) return { identifier, isFallback: false }
-  const fallback = InternalAnnotations.resolveIdentifierFallback(ast)
-  return fallback === undefined ? undefined : { identifier: `${fallback}JsonEncoding`, isFallback: true }
+function resolveReferenceIdentifier(
+  input: SchemaAST.AST,
+  encoded: SchemaAST.AST
+): ReferenceIdentifier | undefined {
+  const identifier = InternalAnnotations.resolveIdentifier(encoded)
+  if (identifier !== undefined) return { identifier }
+  const fallback = (encoded !== input ? InternalAnnotations.resolveIdentifier(input) : undefined) ??
+    InternalAnnotations.resolveIdentifierFallback(encoded)
+  return fallback === undefined
+    ? undefined
+    : { identifier: `${fallback}Encoded`, fallback }
 }
 
-function hasSameReferenceOwner(self: SchemaAST.AST, that: SchemaAST.AST): boolean {
-  if (self === that) return true
-  const selfKeys = Reflect.ownKeys(self)
-  const thatKeys = Reflect.ownKeys(that)
-  if (selfKeys.length !== thatKeys.length) return false
-  for (const key of selfKeys) {
-    if (key !== "context" && (self as any)[key] !== (that as any)[key]) return false
-  }
-  return true
-}
-
-function lowerASTs(
+function fromASTs(
   asts: readonly [SchemaAST.AST, ...Array<SchemaAST.AST>]
 ): SchemaRepresentation.MultiDocument {
   const references: Record<string, SchemaRepresentation.Representation> = {}
-  const referenceMap = new Map<SchemaAST.AST, string>()
-  const fallbackReferences: Array<readonly [SchemaAST.AST, string]> = []
+  const anonymousReferences = new Map<SchemaAST.AST, string>()
   const referenceOwners = new Map<string, SchemaAST.AST>()
+  const valueIds = new Map<unknown, number>()
+  const canonicalByKey = new Map<string, SchemaAST.AST>()
+  let nextValueId = 0
+  const buildingReferences = new Set<string>()
   const visiting = new Set<SchemaAST.AST>()
   const visited = new Set<SchemaAST.AST>()
   const shared = new Set<SchemaAST.AST>()
@@ -75,35 +73,82 @@ function lowerASTs(
 
   return { representations, references }
 
-  function generateReference(prefix: string, owner: SchemaAST.AST): string {
+  function getReference(prefix: string, owner: SchemaAST.AST, separator = "_"): string {
     let candidate = prefix
     let suffix = 0
     while (referenceOwners.has(candidate)) {
-      candidate = `${prefix}${++suffix}`
+      if (referenceOwners.get(candidate) === owner) return candidate
+      candidate = `${prefix}${separator}${++suffix}`
     }
     referenceOwners.set(candidate, owner)
     return candidate
   }
 
+  function getValueId(value: unknown): number {
+    if (typeof value === "number" && globalThis.Number.isNaN(value)) {
+      return nextValueId++
+    }
+    const found = valueIds.get(value)
+    if (found !== undefined) return found
+    const id = nextValueId++
+    valueIds.set(value, id)
+    return id
+  }
+
+  function getIdentityKey(ast: SchemaAST.AST): string {
+    let identity = ast._tag
+    for (const [key, value] of Object.entries(ast)) {
+      if (key !== "_tag" && key !== "context") identity += `:${getValueId(value)}`
+    }
+    return identity
+  }
+
+  function getCanonicalAST(ast: SchemaAST.AST): SchemaAST.AST {
+    const key = getIdentityKey(ast)
+    const canonical = canonicalByKey.get(key)
+    if (canonical === undefined) {
+      canonicalByKey.set(key, ast)
+      return ast
+    }
+    return canonical
+  }
+
+  function annotateReference(
+    ast: SchemaAST.AST,
+    referenceIdentifier: ReferenceIdentifier,
+    reference: string
+  ): SchemaAST.AST {
+    const fallback = referenceIdentifier.fallback
+    if (fallback !== undefined) {
+      return InternalAnnotations.resolveIdentifierFallback(ast) === fallback
+        ? ast
+        : SchemaAST.annotate(ast, {
+          [InternalAnnotations.IDENTIFIER_FALLBACK_KEY]: fallback
+        })
+    }
+    return reference === referenceIdentifier.identifier
+      ? ast
+      : SchemaAST.annotate(ast, { identifier: reference })
+  }
+
+  function makeReference(reference: string, ast: SchemaAST.AST): SchemaRepresentation.Reference {
+    if (!Object.hasOwn(references, reference) && !buildingReferences.has(reference)) {
+      buildingReferences.add(reference)
+      const representation = on(ast)
+      buildingReferences.delete(reference)
+      InternalRecord.assignProperty(references, reference, representation)
+    }
+    return { _tag: "Reference", $ref: reference }
+  }
+
   function visit(input: SchemaAST.AST): void {
     const ast = SchemaAST.getLastEncoding(input)
-    if (visited.has(ast)) {
-      if (isShareable(ast)) shared.add(ast)
+    const owner = getCanonicalAST(ast)
+    if (visited.has(owner)) {
+      if (isShareable(ast)) shared.add(owner)
       return
     }
-    visited.add(ast)
-    const referenceIdentifier = resolveReferenceIdentifier(ast)
-    if (referenceIdentifier !== undefined && !referenceIdentifier.isFallback) {
-      const owner = referenceOwners.get(referenceIdentifier.identifier)
-      if (owner === undefined) {
-        referenceOwners.set(referenceIdentifier.identifier, ast)
-      } else if (
-        referenceMap.get(ast) !== referenceIdentifier.identifier &&
-        !hasSameReferenceOwner(owner, ast)
-      ) {
-        throw new Error(`Duplicate identifier: ${JSON.stringify(referenceIdentifier.identifier)}`)
-      }
-    }
+    visited.add(owner)
     visitChecks(ast.checks)
     switch (ast._tag) {
       case "Declaration":
@@ -131,45 +176,34 @@ function lowerASTs(
     })
   }
 
-  function recur(ast: SchemaAST.AST): SchemaRepresentation.Representation {
-    const found = referenceMap.get(ast)
-    if (found !== undefined) {
-      return { _tag: "Reference", $ref: found }
-    }
-
-    const projected = SchemaAST.getLastEncoding(ast)
-    if (projected !== ast) {
-      return recur(projected)
-    }
-
-    const referenceIdentifier = resolveReferenceIdentifier(ast)
+  function recur(input: SchemaAST.AST): SchemaRepresentation.Representation {
+    const ast = SchemaAST.getLastEncoding(input)
+    const owner = getCanonicalAST(ast)
+    const referenceIdentifier = resolveReferenceIdentifier(input, ast)
     if (referenceIdentifier !== undefined) {
-      const reference = getReference(referenceIdentifier, ast)
-      referenceMap.set(ast, reference)
-      if (!Object.hasOwn(references, reference)) {
-        InternalRecord.assignProperty(references, reference, on(ast))
-      }
-      return { _tag: "Reference", $ref: reference }
+      const reference = getReference(referenceIdentifier.identifier, owner)
+      return makeReference(reference, annotateReference(ast, referenceIdentifier, reference))
     }
 
-    if (shared.has(ast)) {
-      const reference = generateReference(`${ast._tag}_`, ast)
-      referenceMap.set(ast, reference)
-      InternalRecord.assignProperty(references, reference, on(ast))
-      return { _tag: "Reference", $ref: reference }
+    const found = anonymousReferences.get(owner)
+    if (found !== undefined) {
+      return { _tag: "Reference", $ref: found }
     }
 
-    if (visiting.has(ast)) {
-      const reference = generateReference(`${ast._tag}_`, ast)
-      referenceMap.set(ast, reference)
-      return { _tag: "Reference", $ref: reference }
+    const isShared = shared.has(owner)
+    if (isShared || visiting.has(owner)) {
+      const reference = getReference(`${ast._tag}_`, owner, "")
+      anonymousReferences.set(owner, reference)
+      return isShared
+        ? makeReference(reference, ast)
+        : { _tag: "Reference", $ref: reference }
     }
 
-    visiting.add(ast)
+    visiting.add(owner)
     const representation = on(ast)
-    visiting.delete(ast)
+    visiting.delete(owner)
 
-    const reference = referenceMap.get(ast)
+    const reference = anonymousReferences.get(owner)
     if (reference !== undefined) {
       InternalRecord.assignProperty(references, reference, representation)
       return { _tag: "Reference", $ref: reference }
@@ -178,18 +212,6 @@ function lowerASTs(
     return representation
   }
 
-  function getReference(referenceIdentifier: ReferenceIdentifier, ast: SchemaAST.AST): string {
-    if (!referenceIdentifier.isFallback) {
-      return referenceIdentifier.identifier
-    }
-    for (const [owner, reference] of fallbackReferences) {
-      if (hasSameReferenceOwner(owner, ast)) return reference
-    }
-    const reference = generateReference(referenceIdentifier.identifier, ast)
-    fallbackReferences.push([ast, reference])
-    return reference
-  }
-
   function on(ast: SchemaAST.AST): SchemaRepresentation.Representation {
     const checks = fromChecks(ast.checks)
     switch (ast._tag) {