-
Notifications
You must be signed in to change notification settings - Fork 11
actually fix name collisions in derive closures #2044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f5379c9
f469683
ac6b853
26b50e8
f24c583
b950d9a
a7908a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1441,6 +1441,39 @@ function extractDeriveCallback( | |
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve capture name collisions with the original input parameter name. | ||
| * If a capture has the same name as originalInputParamName, rename it (e.g., multiplier -> multiplier_1). | ||
| * Returns a mapping from original capture names to their potentially renamed versions. | ||
| */ | ||
| function resolveDeriveCaptureNameCollisions( | ||
| originalInputParamName: string, | ||
| captureTree: Map<string, CaptureTreeNode>, | ||
| ): Map<string, string> { | ||
| const captureNameMap = new Map<string, string>(); | ||
| const usedNames = new Set<string>([originalInputParamName]); | ||
|
|
||
| for (const [captureName] of captureTree) { | ||
| if (captureName === originalInputParamName) { | ||
| // Collision detected - rename the capture | ||
| let renamed = `${captureName}_1`; | ||
| let suffix = 1; | ||
| while (usedNames.has(renamed) || captureTree.has(renamed)) { | ||
| suffix++; | ||
| renamed = `${captureName}_${suffix}`; | ||
| } | ||
| captureNameMap.set(captureName, renamed); | ||
| usedNames.add(renamed); | ||
| } else { | ||
| // No collision - use original name | ||
| captureNameMap.set(captureName, captureName); | ||
| usedNames.add(captureName); | ||
| } | ||
| } | ||
|
|
||
| return captureNameMap; | ||
| } | ||
|
|
||
| /** | ||
| * Build the merged input object containing both the original input and captures. | ||
| * Example: {value, multiplier} where value is the original input and multiplier is a capture. | ||
|
|
@@ -1452,6 +1485,7 @@ function buildDeriveInputObject( | |
| originalInput: ts.Expression, | ||
| originalInputParamName: string, | ||
| captureTree: Map<string, CaptureTreeNode>, | ||
| captureNameMap: Map<string, string>, | ||
| factory: ts.NodeFactory, | ||
| hadZeroParameters: boolean, | ||
| ): ts.ObjectLiteralExpression { | ||
|
|
@@ -1478,15 +1512,90 @@ function buildDeriveInputObject( | |
| } | ||
| } | ||
|
|
||
| // Add captures | ||
| properties.push(...buildCapturePropertyAssignments(captureTree, factory)); | ||
| // Add captures with potentially renamed property names | ||
| for (const [originalName, node] of captureTree) { | ||
| const propertyName = captureNameMap.get(originalName) ?? originalName; | ||
| properties.push( | ||
| factory.createPropertyAssignment( | ||
| createPropertyName(propertyName, factory), | ||
| buildHierarchicalParamsValue(node, originalName, factory), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| return factory.createObjectLiteralExpression( | ||
| properties, | ||
| properties.length > 1, | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Rewrite the callback body to use renamed capture identifiers. | ||
| * For example, if `multiplier` was renamed to `multiplier_1`, replace all | ||
| * references to the captured `multiplier` with `multiplier_1`. | ||
| */ | ||
| function rewriteCaptureReferences( | ||
| body: ts.ConciseBody, | ||
| captureNameMap: Map<string, string>, | ||
| factory: ts.NodeFactory, | ||
| ): ts.ConciseBody { | ||
| // Build a reverse map: original capture name -> list of renamed names that should be substituted | ||
| const substitutions = new Map<string, string>(); | ||
| for (const [originalName, renamedName] of captureNameMap) { | ||
| if (originalName !== renamedName) { | ||
| substitutions.set(originalName, renamedName); | ||
| } | ||
| } | ||
|
|
||
| if (substitutions.size === 0) { | ||
| return body; // No substitutions needed | ||
| } | ||
|
|
||
| const visitor = (node: ts.Node, parent?: ts.Node): ts.Node => { | ||
| // Handle shorthand property assignments specially | ||
| // { multiplier } needs to become { multiplier: multiplier_1 } if multiplier is renamed | ||
| if (ts.isShorthandPropertyAssignment(node)) { | ||
| const substituteName = substitutions.get(node.name.text); | ||
| if (substituteName) { | ||
| // Expand shorthand into full property assignment | ||
| return factory.createPropertyAssignment( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expanding shorthand captures to property assignments needs to preserve Prompt for AI agents |
||
| node.name, // Property name stays the same | ||
| factory.createIdentifier(substituteName), // Value uses renamed identifier | ||
| ); | ||
| } | ||
| // No substitution needed, keep as shorthand | ||
| return node; | ||
| } | ||
|
|
||
| // Don't substitute identifiers that are property names | ||
| if (ts.isIdentifier(node)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This identifier substitution needs to guard against property-access/property-name contexts; otherwise we rename property names (e.g. Prompt for AI agents✅ Addressed in |
||
| // Skip if this identifier is the property name in a property access (e.g., '.get' in 'obj.get') | ||
| if ( | ||
| parent && ts.isPropertyAccessExpression(parent) && parent.name === node | ||
| ) { | ||
| return node; | ||
| } | ||
|
|
||
| // Skip if this identifier is a property name in an object literal (e.g., 'foo' in '{ foo: value }') | ||
| if (parent && ts.isPropertyAssignment(parent) && parent.name === node) { | ||
| return node; | ||
| } | ||
|
|
||
| const substituteName = substitutions.get(node.text); | ||
| if (substituteName) { | ||
| return factory.createIdentifier(substituteName); | ||
| } | ||
| } | ||
|
|
||
| return ts.visitEachChild(node, (child) => visitor(child, node), undefined); | ||
| }; | ||
|
|
||
| return ts.visitNode( | ||
| body, | ||
| (node) => visitor(node, undefined), | ||
| ) as ts.ConciseBody; | ||
| } | ||
|
|
||
| /** | ||
| * Create the derive callback with parameter aliasing to preserve user's parameter name. | ||
| * Example: ({value: v, multiplier}) => v * multiplier | ||
|
|
@@ -1499,6 +1608,7 @@ function createDeriveCallback( | |
| transformedBody: ts.ConciseBody, | ||
| originalInputParamName: string, | ||
| captureTree: Map<string, CaptureTreeNode>, | ||
| captureNameMap: Map<string, string>, | ||
| context: TransformationContext, | ||
| hadZeroParameters: boolean, | ||
| ): ts.ArrowFunction | ts.FunctionExpression { | ||
|
|
@@ -1590,14 +1700,19 @@ function createDeriveCallback( | |
| ), | ||
| ); | ||
|
|
||
| // Add bindings for captures | ||
| // Add bindings for captures using the potentially renamed property names | ||
| const createBindingIdentifier = (name: string): ts.Identifier => { | ||
| return reserveIdentifier(name, usedBindingNames, factory); | ||
| }; | ||
|
|
||
| // Create binding elements using the renamed capture names | ||
| const renamedCaptureNames = Array.from(captureTree.keys()).map( | ||
| (originalName) => captureNameMap.get(originalName) ?? originalName, | ||
| ); | ||
|
|
||
| bindingElements.push( | ||
| ...createBindingElementsFromNames( | ||
| captureTree.keys(), | ||
| renamedCaptureNames, | ||
| factory, | ||
| createBindingIdentifier, | ||
| ), | ||
|
|
@@ -1613,6 +1728,13 @@ function createDeriveCallback( | |
| undefined, | ||
| ); | ||
|
|
||
| // Rewrite the body to use renamed capture identifiers | ||
| const rewrittenBody = rewriteCaptureReferences( | ||
| transformedBody, | ||
| captureNameMap, | ||
| factory, | ||
| ); | ||
|
|
||
| // Create the new callback | ||
| if (ts.isArrowFunction(callback)) { | ||
| return factory.createArrowFunction( | ||
|
|
@@ -1621,7 +1743,7 @@ function createDeriveCallback( | |
| [parameter], | ||
| undefined, // No return type - rely on inference | ||
| callback.equalsGreaterThanToken, | ||
| transformedBody, | ||
| rewrittenBody, | ||
| ); | ||
| } else { | ||
| return factory.createFunctionExpression( | ||
|
|
@@ -1631,7 +1753,7 @@ function createDeriveCallback( | |
| callback.typeParameters, | ||
| [parameter], | ||
| undefined, // No return type - rely on inference | ||
| transformedBody as ts.Block, | ||
| rewrittenBody as ts.Block, | ||
| ); | ||
| } | ||
| } | ||
|
|
@@ -1646,6 +1768,7 @@ function buildDeriveInputSchema( | |
| originalInputParamName: string, | ||
| originalInput: ts.Expression, | ||
| captureTree: Map<string, CaptureTreeNode>, | ||
| captureNameMap: Map<string, string>, | ||
| context: TransformationContext, | ||
| hadZeroParameters: boolean, | ||
| ): ts.TypeNode { | ||
|
|
@@ -1677,11 +1800,40 @@ function buildDeriveInputSchema( | |
| ); | ||
| } | ||
|
|
||
| // Add type elements for captures | ||
| typeElements.push( | ||
| ...buildTypeElementsFromCaptureTree(captureTree, context), | ||
| // Add type elements for captures using the existing helper | ||
| const captureTypeElements = buildTypeElementsFromCaptureTree( | ||
| captureTree, | ||
| context, | ||
| ); | ||
|
|
||
| // Rename the property signatures if there are collisions | ||
| for (const typeElement of captureTypeElements) { | ||
| if ( | ||
| ts.isPropertySignature(typeElement) && ts.isIdentifier(typeElement.name) | ||
| ) { | ||
| const originalName = typeElement.name.text; | ||
| const renamedName = captureNameMap.get(originalName) ?? originalName; | ||
|
|
||
| if (renamedName !== originalName) { | ||
| // Create a new property signature with the renamed identifier | ||
| typeElements.push( | ||
| factory.createPropertySignature( | ||
| typeElement.modifiers, | ||
| factory.createIdentifier(renamedName), | ||
| typeElement.questionToken, | ||
| typeElement.type, | ||
| ), | ||
| ); | ||
| } else { | ||
| // No renaming needed | ||
| typeElements.push(typeElement); | ||
| } | ||
| } else { | ||
| // Not a simple property signature, keep as-is | ||
| typeElements.push(typeElement); | ||
| } | ||
| } | ||
|
|
||
| // Create object type literal | ||
| return factory.createTypeLiteralNode(typeElements); | ||
| } | ||
|
|
@@ -1757,11 +1909,18 @@ function transformDeriveCall( | |
| // In this case, we don't need to preserve the input - just use captures | ||
| const hadZeroParameters = callback.parameters.length === 0; | ||
|
|
||
| // Resolve capture name collisions with the original input parameter name | ||
| const captureNameMap = resolveDeriveCaptureNameCollisions( | ||
| originalInputParamName, | ||
| captureTree, | ||
| ); | ||
|
|
||
| // Build merged input object | ||
| const mergedInput = buildDeriveInputObject( | ||
| originalInput, | ||
| originalInputParamName, | ||
| captureTree, | ||
| captureNameMap, | ||
| factory, | ||
| hadZeroParameters, | ||
| ); | ||
|
|
@@ -1772,6 +1931,7 @@ function transformDeriveCall( | |
| transformedBody, | ||
| originalInputParamName, | ||
| captureTree, | ||
| captureNameMap, | ||
| context, | ||
| hadZeroParameters, | ||
| ); | ||
|
|
@@ -1782,6 +1942,7 @@ function transformDeriveCall( | |
| originalInputParamName, | ||
| originalInput, | ||
| captureTree, | ||
| captureNameMap, | ||
| context, | ||
| hadZeroParameters, | ||
| ); | ||
|
|
@@ -1790,19 +1951,26 @@ function transformDeriveCall( | |
| // SchemaInjectionTransformer will use this to generate the result schema | ||
| const signature = context.checker.getSignatureFromDeclaration(callback); | ||
| let resultTypeNode: ts.TypeNode | undefined; | ||
| let resultType: ts.Type | undefined; | ||
|
|
||
| if (callback.type) { | ||
| // Explicit return type annotation - use it | ||
| // Explicit return type annotation - use it directly (no need to register in typeRegistry) | ||
| resultTypeNode = callback.type; | ||
| } else if (signature) { | ||
| // Infer from callback signature | ||
| const returnType = signature.getReturnType(); | ||
| resultType = signature.getReturnType(); | ||
| resultTypeNode = context.checker.typeToTypeNode( | ||
| returnType, | ||
| resultType, | ||
| context.sourceFile, | ||
| ts.NodeBuilderFlags.NoTruncation | | ||
| ts.NodeBuilderFlags.UseStructuralFallback, | ||
| ); | ||
|
|
||
| // Register the result Type in typeRegistry for the synthetic TypeNode | ||
| // This fixes schema generation for shorthand properties referencing captured variables | ||
| if (resultTypeNode && context.options.typeRegistry) { | ||
| context.options.typeRegistry.set(resultTypeNode, resultType); | ||
| } | ||
| } | ||
|
|
||
| // Build the derive call expression | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning the bare root schema removes the
$schemadeclaration, yet this generator still emits$defs. Validators that assume draft-07 when$schemais missing will treat$defsas unknown and break validation. Please keep$schemaon the returned schema.Prompt for AI agents