From cefaf7e87206d0d2ec498deb465e13c909a9dbf8 Mon Sep 17 00:00:00 2001 From: Microsoft Auto Changeset Bot Date: Wed, 6 Sep 2023 08:21:03 -0700 Subject: [PATCH 1/5] Add test --- packages/compiler/src/core/checker.ts | 22 ++++++++- packages/compiler/src/core/messages.ts | 6 +++ packages/compiler/test/checker/model.test.ts | 47 ++++++++++++++++++++ 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index fbdbfbb0fd9..a62effe67ba 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -3072,13 +3072,31 @@ export function createChecker(program: Program): Checker { prop: ModelPropertyNode, mapper: TypeMapper | undefined ): ModelProperty { + const symId = getSymbolId(getSymbolForMember(prop)!); const links = getSymbolLinksForMember(prop); + if (links && links.declaredType && mapper === undefined) { return links.declaredType as ModelProperty; } - const name = prop.id.sv; + if (pendingResolutions.has(symId)) { + if (mapper === undefined) { + reportCheckerDiagnostic( + createDiagnostic({ + code: "circular-prop", + format: { propName: name }, + target: prop, + }) + ); + } + if (links) { + links.declaredType = errorType; + } + return errorType as any; + } + pendingResolutions.add(symId); + const valueType = getTypeForNode(prop.value, mapper); const defaultValue = prop.default && checkDefault(prop.default, valueType); @@ -3115,6 +3133,8 @@ export function createChecker(program: Program): Checker { finishType(type); } + pendingResolutions.delete(symId); + return type; } diff --git a/packages/compiler/src/core/messages.ts b/packages/compiler/src/core/messages.ts index 8c57b21a32c..016d1dba60f 100644 --- a/packages/compiler/src/core/messages.ts +++ b/packages/compiler/src/core/messages.ts @@ -811,6 +811,12 @@ const diagnostics = { default: paramMessage`Alias type '${"typeName"}' recursively references itself.`, }, }, + "circular-prop": { + severity: "error", + messages: { + default: paramMessage`Property '${"propName"}' recursively references itself.`, + }, + }, "conflict-marker": { severity: "error", messages: { diff --git a/packages/compiler/test/checker/model.test.ts b/packages/compiler/test/checker/model.test.ts index 85d873e8132..04ce988a0ad 100644 --- a/packages/compiler/test/checker/model.test.ts +++ b/packages/compiler/test/checker/model.test.ts @@ -815,4 +815,51 @@ describe("compiler: models", () => { strictEqual(((C as Model).properties.get("b")?.type as any).name, "B"); }); }); + + describe("property circular references", () => { + it("emit diagnostics if property reference itself", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { a: A.a } + ` + ); + const diagnostics = await testHost.diagnose("main.tsp"); + expectDiagnostics(diagnostics, { + code: "circular-prop", + message: "Property 'a' recursively references itself.", + }); + }); + + it("emit diagnostics if property reference itself via another prop", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { a: B.a } + model B { a: A.a } + ` + ); + const diagnostics = await testHost.diagnose("main.tsp"); + expectDiagnostics(diagnostics, { + code: "circular-prop", + message: "Property 'a' recursively references itself.", + }); + }); + + it("emit diagnostics if property reference itself via alias", async () => { + testHost.addTypeSpecFile( + "main.tsp", + ` + model A { a: B.a } + model B { a: C } + alias C = A.a; + ` + ); + const diagnostics = await testHost.diagnose("main.tsp"); + expectDiagnostics(diagnostics, { + code: "circular-prop", + message: "Property 'a' recursively references itself.", + }); + }); + }); }); From 2d361ef39c22011e49a852abc87c6fd4e249e194 Mon Sep 17 00:00:00 2001 From: Microsoft Auto Changeset Bot Date: Wed, 6 Sep 2023 08:32:45 -0700 Subject: [PATCH 2/5] Changelog --- .../compiler/fix-circular-prop_2023-09-06-15-32.json | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 common/changes/@typespec/compiler/fix-circular-prop_2023-09-06-15-32.json diff --git a/common/changes/@typespec/compiler/fix-circular-prop_2023-09-06-15-32.json b/common/changes/@typespec/compiler/fix-circular-prop_2023-09-06-15-32.json new file mode 100644 index 00000000000..642fba8c088 --- /dev/null +++ b/common/changes/@typespec/compiler/fix-circular-prop_2023-09-06-15-32.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/compiler", + "comment": "**Fix** Stackoverflow when model property reference itself", + "type": "none" + } + ], + "packageName": "@typespec/compiler" +} \ No newline at end of file From 09933d8ab07ffe0c7ef6bc0045387d9214ba88c5 Mon Sep 17 00:00:00 2001 From: Microsoft Auto Changeset Bot Date: Wed, 6 Sep 2023 09:49:29 -0700 Subject: [PATCH 3/5] Need to differentiate resolutions kinds --- packages/compiler/src/core/checker.ts | 109 +++++++++++++++++--------- 1 file changed, 73 insertions(+), 36 deletions(-) diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index c00ed110e1e..8cd93ae3319 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -315,7 +315,7 @@ export function createChecker(program: Program): Checker { * Set keeping track of node pending type resolution. * Key is the SymId of a node. It can be retrieved with getNodeSymId(node) */ - const pendingResolutions = new Set(); + const pendingResolutions = new PendingResolutions(); for (const file of program.jsSourceFiles.values()) { mergeSourceFile(file); @@ -1733,7 +1733,7 @@ export function createChecker(program: Program): Checker { // Ensure that we don't end up with a circular reference to the same operation const opSymId = getNodeSymId(operation); if (opSymId) { - pendingResolutions.add(opSymId); + pendingResolutions.start(opSymId, ResolutionKind.BaseType); } const target = resolveTypeReferenceSym(opReference, mapper); @@ -1742,7 +1742,9 @@ export function createChecker(program: Program): Checker { } // Did we encounter a circular operation reference? - if (pendingResolutions.has(getNodeSymId(target.declarations[0] as any))) { + if ( + pendingResolutions.has(getNodeSymId(target.declarations[0] as any), ResolutionKind.BaseType) + ) { if (mapper === undefined) { reportCheckerDiagnostic( createDiagnostic({ @@ -1759,7 +1761,7 @@ export function createChecker(program: Program): Checker { // Resolve the base operation type const baseOperation = checkTypeReferenceSymbol(target, opReference, mapper); if (opSymId) { - pendingResolutions.delete(opSymId); + pendingResolutions.finish(opSymId, ResolutionKind.BaseType); } if (isErrorType(baseOperation)) { @@ -2908,14 +2910,16 @@ export function createChecker(program: Program): Checker { return undefined; } const modelSymId = getNodeSymId(model); - pendingResolutions.add(modelSymId); + pendingResolutions.start(modelSymId, ResolutionKind.BaseType); const target = resolveTypeReferenceSym(heritageRef, mapper); if (target === undefined) { return undefined; } - if (pendingResolutions.has(getNodeSymId(target.declarations[0] as any))) { + if ( + pendingResolutions.has(getNodeSymId(target.declarations[0] as any), ResolutionKind.BaseType) + ) { if (mapper === undefined) { reportCheckerDiagnostic( createDiagnostic({ @@ -2928,7 +2932,7 @@ export function createChecker(program: Program): Checker { return undefined; } const heritageType = checkTypeReferenceSymbol(target, heritageRef, mapper); - pendingResolutions.delete(modelSymId); + pendingResolutions.finish(modelSymId, ResolutionKind.BaseType); if (isErrorType(heritageType)) { compilerAssert(program.hasError(), "Should already have reported an error.", heritageRef); return undefined; @@ -2960,7 +2964,7 @@ export function createChecker(program: Program): Checker { if (!isExpr) return undefined; const modelSymId = getNodeSymId(model); - pendingResolutions.add(modelSymId); + pendingResolutions.start(modelSymId, ResolutionKind.BaseType); let isType; if (isExpr.kind === SyntaxKind.ModelExpression) { reportCheckerDiagnostic( @@ -2978,7 +2982,9 @@ export function createChecker(program: Program): Checker { if (target === undefined) { return undefined; } - if (pendingResolutions.has(getNodeSymId(target.declarations[0] as any))) { + if ( + pendingResolutions.has(getNodeSymId(target.declarations[0] as any), ResolutionKind.BaseType) + ) { if (mapper === undefined) { reportCheckerDiagnostic( createDiagnostic({ @@ -2996,7 +3002,7 @@ export function createChecker(program: Program): Checker { return undefined; } - pendingResolutions.delete(modelSymId); + pendingResolutions.finish(modelSymId, ResolutionKind.BaseType); if (isType.kind !== "Model") { reportCheckerDiagnostic(createDiagnostic({ code: "is-model", target: isExpr })); @@ -3086,7 +3092,16 @@ export function createChecker(program: Program): Checker { } const name = prop.id.sv; - if (pendingResolutions.has(symId)) { + const type: ModelProperty = createType({ + kind: "ModelProperty", + name, + node: prop, + optional: prop.optional, + type: undefined!, + decorators: [], + }); + + if (pendingResolutions.has(symId, ResolutionKind.Type)) { if (mapper === undefined) { reportCheckerDiagnostic( createDiagnostic({ @@ -3096,25 +3111,12 @@ export function createChecker(program: Program): Checker { }) ); } - if (links) { - links.declaredType = errorType; - } - return errorType as any; + type.type = errorType; + } else { + pendingResolutions.start(symId, ResolutionKind.Type); + type.type = getTypeForNode(prop.value, mapper); + type.default = prop.default && checkDefault(prop.default, type.type); } - pendingResolutions.add(symId); - - const valueType = getTypeForNode(prop.value, mapper); - const defaultValue = prop.default && checkDefault(prop.default, valueType); - - const type: ModelProperty = createType({ - kind: "ModelProperty", - name, - node: prop, - optional: prop.optional, - type: valueType, - decorators: [], - default: defaultValue, - }); if (links) { linkType(links, type, mapper); } @@ -3139,7 +3141,7 @@ export function createChecker(program: Program): Checker { finishType(type); } - pendingResolutions.delete(symId); + pendingResolutions.finish(symId, ResolutionKind.Type); return type; } @@ -3459,14 +3461,16 @@ export function createChecker(program: Program): Checker { mapper: TypeMapper | undefined ): Scalar | undefined { const symId = getNodeSymId(scalar); - pendingResolutions.add(symId); + pendingResolutions.start(symId, ResolutionKind.BaseType); const target = resolveTypeReferenceSym(extendsRef, mapper); if (target === undefined) { return undefined; } - if (pendingResolutions.has(getNodeSymId(target.declarations[0] as any))) { + if ( + pendingResolutions.has(getNodeSymId(target.declarations[0] as any), ResolutionKind.BaseType) + ) { if (mapper === undefined) { reportCheckerDiagnostic( createDiagnostic({ @@ -3479,7 +3483,7 @@ export function createChecker(program: Program): Checker { return undefined; } const extendsType = checkTypeReferenceSymbol(target, extendsRef, mapper); - pendingResolutions.delete(symId); + pendingResolutions.finish(symId, ResolutionKind.BaseType); if (isErrorType(extendsType)) { compilerAssert(program.hasError(), "Should already have reported an error.", extendsRef); return undefined; @@ -3502,7 +3506,7 @@ export function createChecker(program: Program): Checker { checkTemplateDeclaration(node, mapper); const aliasSymId = getNodeSymId(node); - if (pendingResolutions.has(aliasSymId)) { + if (pendingResolutions.has(aliasSymId, ResolutionKind.Type)) { if (mapper === undefined) { reportCheckerDiagnostic( createDiagnostic({ @@ -3516,10 +3520,10 @@ export function createChecker(program: Program): Checker { return errorType; } - pendingResolutions.add(aliasSymId); + pendingResolutions.start(aliasSymId, ResolutionKind.Type); const type = getTypeForNode(node.value, mapper); linkType(links, type, mapper); - pendingResolutions.delete(aliasSymId); + pendingResolutions.finish(aliasSymId, ResolutionKind.Type); return type; } @@ -5842,3 +5846,36 @@ const ReflectionNameToKind = { } as const; const _assertReflectionNameToKind: Record = ReflectionNameToKind; + +enum ResolutionKind { + Type, + BaseType, +} + +class PendingResolutions { + #data = new Map>(); + + start(symId: number, kind: ResolutionKind) { + let existing = this.#data.get(symId); + if (existing === undefined) { + existing = new Set(); + this.#data.set(symId, existing); + } + existing.add(kind); + } + + has(symId: number, kind: ResolutionKind): boolean { + return this.#data.get(symId)?.has(kind) ?? false; + } + + finish(symId: number, kind: ResolutionKind) { + const existing = this.#data.get(symId); + if (existing === undefined) { + return; + } + existing?.delete(kind); + if (existing.size === 0) { + this.#data.delete(symId); + } + } +} From 9b6a107a7c3ff048c1bba08af8376ae25dbec5b4 Mon Sep 17 00:00:00 2001 From: Microsoft Auto Changeset Bot Date: Wed, 6 Sep 2023 10:36:33 -0700 Subject: [PATCH 4/5] fix --- packages/compiler/src/core/checker.ts | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index 8cd93ae3319..3c62c2242b9 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -3101,25 +3101,23 @@ export function createChecker(program: Program): Checker { decorators: [], }); - if (pendingResolutions.has(symId, ResolutionKind.Type)) { - if (mapper === undefined) { - reportCheckerDiagnostic( - createDiagnostic({ - code: "circular-prop", - format: { propName: name }, - target: prop, - }) - ); - } + if (pendingResolutions.has(symId, ResolutionKind.Type) && mapper === undefined) { + reportCheckerDiagnostic( + createDiagnostic({ + code: "circular-prop", + format: { propName: name }, + target: prop, + }) + ); type.type = errorType; } else { + if (links) { + linkType(links, type, mapper); + } pendingResolutions.start(symId, ResolutionKind.Type); type.type = getTypeForNode(prop.value, mapper); type.default = prop.default && checkDefault(prop.default, type.type); } - if (links) { - linkType(links, type, mapper); - } type.decorators = checkDecorators(type, prop, mapper); const parentTemplate = getParentTemplateNode(prop); From 39564942d78327dad55a979f0d7625955353c31a Mon Sep 17 00:00:00 2001 From: Microsoft Auto Changeset Bot Date: Wed, 6 Sep 2023 11:53:08 -0700 Subject: [PATCH 5/5] Fix --- packages/compiler/src/core/checker.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index 3c62c2242b9..2a64c52dadb 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -3111,12 +3111,12 @@ export function createChecker(program: Program): Checker { ); type.type = errorType; } else { - if (links) { - linkType(links, type, mapper); - } pendingResolutions.start(symId, ResolutionKind.Type); type.type = getTypeForNode(prop.value, mapper); type.default = prop.default && checkDefault(prop.default, type.type); + if (links) { + linkType(links, type, mapper); + } } type.decorators = checkDecorators(type, prop, mapper);