From 888e8f856c6e1e13f2f334a29e513c7db393219b Mon Sep 17 00:00:00 2001 From: Microsoft Auto Changeset Bot Date: Wed, 6 Sep 2023 14:03:57 -0700 Subject: [PATCH 1/3] Compare recrusive types --- packages/compiler/src/core/checker.ts | 209 +++++++++++++----- .../compiler/test/checker/relation.test.ts | 29 +++ 2 files changed, 179 insertions(+), 59 deletions(-) diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index 352f7c54075..134abcb5a22 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -4939,9 +4939,49 @@ export function createChecker(program: Program): Checker { target: Type | ValueType, diagnosticTarget: DiagnosticTarget ): [boolean, readonly Diagnostic[]] { + const [related, diagnostics] = isTypeAssignableToInternal( + source, + target, + diagnosticTarget, + new MultiKeyMap<[Type | ValueType, Type | ValueType], Related>() + ); + return [related === Related.true, diagnostics]; + } + + function isTypeAssignableToInternal( + source: Type | ValueType, + target: Type | ValueType, + diagnosticTarget: DiagnosticTarget, + relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related> + ): [Related, readonly Diagnostic[]] { + const cached = relationCache.get([source, target]); + if (cached !== undefined) { + return [cached, []]; + } + const [result, diagnostics] = isTypeAssignableToWorker( + source, + target, + diagnosticTarget, + new MultiKeyMap<[Type | ValueType, Type | ValueType], Related>() + ); + relationCache.set([source, target], result); + return [result, diagnostics]; + } + + function isTypeAssignableToWorker( + source: Type | ValueType, + target: Type | ValueType, + diagnosticTarget: DiagnosticTarget, + relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related> + ): [Related, readonly Diagnostic[]] { // BACKCOMPAT: Added May 2023 sprint, to be removed by June 2023 sprint if (source.kind === "TemplateParameter" && source.constraint && target.kind === "Value") { - const [assignable] = isTypeAssignableTo(source.constraint, target.target, diagnosticTarget); + const [assignable] = isTypeAssignableToInternal( + source.constraint, + target.target, + diagnosticTarget, + relationCache + ); if (assignable) { const constraint = getTypeName(source.constraint); reportDeprecated( @@ -4951,36 +4991,41 @@ export function createChecker(program: Program): Checker { )}' in the future. Update the constraint to be 'valueof ${constraint}'`, diagnosticTarget ); - return [true, []]; + return [Related.true, []]; } } if (source.kind === "TemplateParameter") { source = source.constraint ?? unknownType; } - if (source === target) return [true, []]; + if (source === target) return [Related.true, []]; if (target.kind === "Value") { - return isAssignableToValueType(source, target, diagnosticTarget); + return isAssignableToValueType(source, target, diagnosticTarget, relationCache); } if (source.kind === "Value") { - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } const isSimpleTypeRelated = isSimpleTypeAssignableTo(source, target); if (isSimpleTypeRelated === true) { - return [true, []]; + return [Related.true, []]; } else if (isSimpleTypeRelated === false) { - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } if (source.kind === "Union") { for (const variant of source.variants.values()) { - const [variantAssignable] = isTypeAssignableTo(variant.type, target, diagnosticTarget); + const [variantAssignable] = isTypeAssignableToInternal( + variant.type, + target, + diagnosticTarget, + relationCache + ); if (!variantAssignable) { - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } } - return [true, []]; + return [Related.true, []]; } if ( @@ -4992,7 +5037,7 @@ export function createChecker(program: Program): Checker { source.indexer.key.name === "integer" ) { return [ - false, + Related.false, [ createDiagnostic({ code: "missing-index", @@ -5005,49 +5050,66 @@ export function createChecker(program: Program): Checker { ], ]; } else if (target.kind === "Model" && target.indexer !== undefined && source.kind === "Model") { - return isIndexerValid(source, target as Model & { indexer: ModelIndexer }, diagnosticTarget); + return isIndexerValid( + source, + target as Model & { indexer: ModelIndexer }, + diagnosticTarget, + relationCache + ); } else if (target.kind === "Model" && source.kind === "Model") { - return isModelRelatedTo(source, target, diagnosticTarget); + return isModelRelatedTo(source, target, diagnosticTarget, relationCache); } else if (target.kind === "Model" && target.indexer && source.kind === "Tuple") { for (const item of source.values) { - const [related, diagnostics] = isTypeAssignableTo( + const [related, diagnostics] = isTypeAssignableToInternal( item, target.indexer.value!, - diagnosticTarget + diagnosticTarget, + relationCache ); if (!related) { - return [false, diagnostics]; + return [Related.false, diagnostics]; } } - return [true, []]; + return [Related.true, []]; } else if (target.kind === "Tuple" && source.kind === "Tuple") { - return isTupleAssignableToTuple(source, target, diagnosticTarget); + return isTupleAssignableToTuple(source, target, diagnosticTarget, relationCache); } else if (target.kind === "Union") { - return isAssignableToUnion(source, target, diagnosticTarget); + return isAssignableToUnion(source, target, diagnosticTarget, relationCache); } else if (target.kind === "Enum") { return isAssignableToEnum(source, target, diagnosticTarget); } - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } function isAssignableToValueType( source: Type | ValueType, target: ValueType, - diagnosticTarget: DiagnosticTarget - ): [boolean, readonly Diagnostic[]] { + diagnosticTarget: DiagnosticTarget, + relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related> + ): [Related, readonly Diagnostic[]] { if (source.kind === "Value") { - return isTypeAssignableTo(source.target, target.target, diagnosticTarget); + return isTypeAssignableToInternal( + source.target, + target.target, + diagnosticTarget, + relationCache + ); } - const [assignable, diagnostics] = isTypeAssignableTo(source, target.target, diagnosticTarget); + const [assignable, diagnostics] = isTypeAssignableToInternal( + source, + target.target, + diagnosticTarget, + relationCache + ); if (!assignable) { return [assignable, diagnostics]; } if (!isValueType(source)) { - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } - return [true, []]; + return [Related.true, []]; } function isReflectionType(type: Type): type is Model & { name: ReflectionTypeName } { @@ -5142,8 +5204,10 @@ export function createChecker(program: Program): Checker { function isModelRelatedTo( source: Model, target: Model, - diagnosticTarget: DiagnosticTarget - ): [boolean, Diagnostic[]] { + diagnosticTarget: DiagnosticTarget, + relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related> + ): [Related, Diagnostic[]] { + relationCache.set([source, target], Related.maybe); const diagnostics: Diagnostic[] = []; for (const prop of walkPropertiesInherited(target)) { const sourceProperty = getProperty(source, prop.name); @@ -5162,17 +5226,18 @@ export function createChecker(program: Program): Checker { ); } } else { - const [related, propDiagnostics] = isTypeAssignableTo( + const [related, propDiagnostics] = isTypeAssignableToInternal( sourceProperty.type, prop.type, - diagnosticTarget + diagnosticTarget, + relationCache ); if (!related) { diagnostics.push(...propDiagnostics); } } } - return [diagnostics.length === 0, diagnostics]; + return [diagnostics.length === 0 ? Related.true : Related.false, diagnostics]; } function getProperty(model: Model, name: string): ModelProperty | undefined { @@ -5185,15 +5250,16 @@ export function createChecker(program: Program): Checker { function isIndexerValid( source: Model, target: Model & { indexer: ModelIndexer }, - diagnosticTarget: DiagnosticTarget - ): [boolean, readonly Diagnostic[]] { + diagnosticTarget: DiagnosticTarget, + relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related> + ): [Related, readonly Diagnostic[]] { // Model expressions should be able to be assigned. if (source.name === "" && target.indexer.key.name !== "integer") { - return isIndexConstraintValid(target.indexer.value, source, diagnosticTarget); + return isIndexConstraintValid(target.indexer.value, source, diagnosticTarget, relationCache); } else { if (source.indexer === undefined || source.indexer.key !== target.indexer.key) { return [ - false, + Related.false, [ createDiagnostic({ code: "missing-index", @@ -5206,7 +5272,12 @@ export function createChecker(program: Program): Checker { ], ]; } - return isTypeAssignableTo(source.indexer.value!, target.indexer.value, diagnosticTarget); + return isTypeAssignableToInternal( + source.indexer.value!, + target.indexer.value, + diagnosticTarget, + relationCache + ); } } /** @@ -5217,8 +5288,9 @@ export function createChecker(program: Program): Checker { function isIndexConstraintValid( constraintType: Type, type: Model, - diagnosticTarget: DiagnosticTarget - ): [boolean, readonly Diagnostic[]] { + diagnosticTarget: DiagnosticTarget, + relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related> + ): [Related, readonly Diagnostic[]] { for (const prop of type.properties.values()) { const [related, diagnostics] = isTypeAssignableTo( prop.type, @@ -5226,7 +5298,7 @@ export function createChecker(program: Program): Checker { diagnosticTarget ); if (!related) { - return [false, diagnostics]; + return [Related.false, diagnostics]; } } @@ -5234,23 +5306,25 @@ export function createChecker(program: Program): Checker { const [related, diagnostics] = isIndexConstraintValid( constraintType, type.baseModel, - diagnosticTarget + diagnosticTarget, + relationCache ); if (!related) { - return [false, diagnostics]; + return [Related.false, diagnostics]; } } - return [true, []]; + return [Related.true, []]; } function isTupleAssignableToTuple( source: Tuple, target: Tuple, - diagnosticTarget: DiagnosticTarget - ): [boolean, readonly Diagnostic[]] { + diagnosticTarget: DiagnosticTarget, + relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related> + ): [Related, readonly Diagnostic[]] { if (source.values.length !== target.values.length) { return [ - false, + Related.false, [ createDiagnostic({ code: "unassignable", @@ -5267,48 +5341,59 @@ export function createChecker(program: Program): Checker { } for (const [index, sourceItem] of source.values.entries()) { const targetItem = target.values[index]; - const [related, diagnostics] = isTypeAssignableTo(sourceItem, targetItem, diagnosticTarget); + const [related, diagnostics] = isTypeAssignableToInternal( + sourceItem, + targetItem, + diagnosticTarget, + relationCache + ); if (!related) { - return [false, diagnostics]; + return [Related.false, diagnostics]; } } - return [true, []]; + return [Related.true, []]; } function isAssignableToUnion( source: Type, target: Union, - diagnosticTarget: DiagnosticTarget - ): [boolean, Diagnostic[]] { + diagnosticTarget: DiagnosticTarget, + relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related> + ): [Related, Diagnostic[]] { for (const option of target.variants.values()) { - const [related] = isTypeAssignableTo(source, option.type, diagnosticTarget); + const [related] = isTypeAssignableToInternal( + source, + option.type, + diagnosticTarget, + relationCache + ); if (related) { - return [true, []]; + return [Related.true, []]; } } - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } function isAssignableToEnum( source: Type, target: Enum, diagnosticTarget: DiagnosticTarget - ): [boolean, Diagnostic[]] { + ): [Related, Diagnostic[]] { switch (source.kind) { case "Enum": if (source === target) { - return [true, []]; + return [Related.true, []]; } else { - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } case "EnumMember": if (source.enum === target) { - return [true, []]; + return [Related.true, []]; } else { - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } default: - return [false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; + return [Related.false, [createUnassignableDiagnostic(source, target, diagnosticTarget)]]; } } @@ -5822,3 +5907,9 @@ const ReflectionNameToKind = { } as const; const _assertReflectionNameToKind: Record = ReflectionNameToKind; + +enum Related { + false = 0, + true = 1, + maybe = 2, +} diff --git a/packages/compiler/test/checker/relation.test.ts b/packages/compiler/test/checker/relation.test.ts index 24eeefc987d..ddaf5bcf995 100644 --- a/packages/compiler/test/checker/relation.test.ts +++ b/packages/compiler/test/checker/relation.test.ts @@ -633,6 +633,35 @@ describe("compiler: checker: type relations", () => { } ); }); + + describe("recursive models", () => { + it("compare recursive models", async () => { + await expectTypeAssignable({ + source: "A", + target: "B", + commonCode: ` + model A { a: A } + model B { a: B } + `, + }); + }); + + it("emit diagnostic if they don't match", async () => { + const { related, diagnostics } = await checkTypeAssignable({ + source: "A", + target: "B", + commonCode: ` + model A { a: A } + model B { a: B, b: B } + `, + }); + ok(!related); + expectDiagnostics(diagnostics, { + code: "missing-property", + message: "Property 'b' is missing on type 'A' but required in 'B'", + }); + }); + }); }); describe("Array target", () => { From 33c11b29b9b45439f2b02876c562f9ee422af108 Mon Sep 17 00:00:00 2001 From: Microsoft Auto Changeset Bot Date: Wed, 6 Sep 2023 14:08:36 -0700 Subject: [PATCH 2/3] changelog --- .../fix-relation-recursive-types_2023-09-06-21-08.json | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json diff --git a/common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json b/common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json new file mode 100644 index 00000000000..17b5c1c922b --- /dev/null +++ b/common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/compiler", + "comment": "Fix: Crash with when comparing relation between recursive types", + "type": "none" + } + ], + "packageName": "@typespec/compiler" +} \ No newline at end of file From ee9076ddf28521a51268a1fca5f4b70cdc9aa8bc Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Wed, 6 Sep 2023 14:11:21 -0700 Subject: [PATCH 3/3] Update common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json Co-authored-by: Brian Terlson --- .../compiler/fix-relation-recursive-types_2023-09-06-21-08.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json b/common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json index 17b5c1c922b..be7a54a37c0 100644 --- a/common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json +++ b/common/changes/@typespec/compiler/fix-relation-recursive-types_2023-09-06-21-08.json @@ -2,7 +2,7 @@ "changes": [ { "packageName": "@typespec/compiler", - "comment": "Fix: Crash with when comparing relation between recursive types", + "comment": "Fix: Crash with when relating recursive types", "type": "none" } ],