Skip to content

Fix stack overflow when model properties reference itself #2375

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.

Sign up for GitHub

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

Merged
timotheeguerin merged 10 commits into from
Sep 7, 2023
Merged

Fix stack overflow when model properties reference itself #2375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cefaf7e
Add test
Sep 6, 2023
2d361ef
Changelog
Sep 6, 2023
0cbe74a
Merge branch 'main' into fix/circular-prop
timotheeguerin Sep 6, 2023
e650d44
Merge branch 'main' into fix/circular-prop
timotheeguerin Sep 6, 2023
09933d8
Need to differentiate resolutions kinds
Sep 6, 2023
9b6a107
fix
Sep 6, 2023
3956494
Fix
Sep 6, 2023
02c2d8f
Merge branch 'main' into fix/circular-prop
timotheeguerin Sep 6, 2023
648623f
Merge branch 'main' into fix/circular-prop
timotheeguerin Sep 6, 2023
1ce4e0e
Merge branch 'main' into fix/circular-prop
timotheeguerin Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions common/changes/@typespec/compiler/fix-circular-prop_2023-09-06-15-32.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/compiler",
"comment": "**Fix** Stackoverflow when model property reference itself",
"type": "none"
}
],
"packageName": "@typespec/compiler"
}
103 changes: 79 additions & 24 deletions packages/compiler/src/core/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<number>();
const pendingResolutions = new PendingResolutions();

for (const file of program.jsSourceFiles.values()) {
mergeSourceFile(file);
Expand Down Expand Up @@ -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);
Expand All @@ -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({
Expand All @@ -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)) {
Expand Down Expand Up @@ -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({
Expand All @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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({
Expand All @@ -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 }));
Expand Down Expand Up @@ -3078,27 +3084,39 @@ 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;

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,
type: undefined!,
decorators: [],
default: defaultValue,
});
if (links) {
linkType(links, type, mapper);

if (pendingResolutions.has(symId, ResolutionKind.Type) && mapper === undefined) {
reportCheckerDiagnostic(
createDiagnostic({
code: "circular-prop",
format: { propName: name },
target: prop,
})
);
type.type = errorType;
} else {
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);
Expand All @@ -3121,6 +3139,8 @@ export function createChecker(program: Program): Checker {
finishType(type);
}

pendingResolutions.finish(symId, ResolutionKind.Type);

return type;
}

Expand Down Expand Up @@ -3439,14 +3459,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({
Expand All @@ -3459,7 +3481,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;
Expand All @@ -3482,7 +3504,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({
Expand All @@ -3496,10 +3518,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;
}
Expand Down Expand Up @@ -5908,6 +5930,39 @@ const ReflectionNameToKind = {

const _assertReflectionNameToKind: Record<string, Type["kind"]> = ReflectionNameToKind;

enum ResolutionKind {
Type,
BaseType,
}

class PendingResolutions {
#data = new Map<number, Set<ResolutionKind>>();

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);
}
}
}

enum Related {
false = 0,
true = 1,
Expand Down
6 changes: 6 additions & 0 deletions packages/compiler/src/core/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,12 @@ const diagnostics = {
default: paramMessage`Alias type '${"typeName"}' recursively references itself.`,
},
},
"circular-prop": {

@daviwil daviwil Sep 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be circular-property-reference?

@timotheeguerin timotheeguerin Sep 6, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the same naming style as the other circular reference errors

 },
  "circular-base-type": {
    severity: "error",
    messages: {
      default: paramMessage`Type '${"typeName"}' recursively references itself as a base type.`,
    },
  },
  "circular-op-signature": {
    severity: "error",
    messages: {
      default: paramMessage`Operation '${"typeName"}' recursively references itself.`,
    },
  },
  "circular-alias-type": {
    severity: "error",
    messages: {
      default: paramMessage`Alias type '${"typeName"}' recursively references itself.`,
    },
  },

@daviwil daviwil Sep 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, cool

severity: "error",
messages: {
default: paramMessage`Property '${"propName"}' recursively references itself.`,
},
},
"conflict-marker": {
severity: "error",
messages: {
Expand Down
47 changes: 47 additions & 0 deletions packages/compiler/test/checker/model.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
});
});
});
});