From 2c3156d4e5244d6bd84731c130a641a2a9ab28e9 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 11 Jul 2024 12:30:10 -0700 Subject: [PATCH 1/4] Do not cast model expression to object value if the constraint is allowing the type --- packages/compiler/src/core/checker.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index fe52cdf802..31c4444ebd 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -1548,7 +1548,14 @@ export function createChecker(program: Program): Checker { ? finalMap.get(param.constraint.type)! : param.constraint; - if (isType(type) && param.constraint?.valueType) { + if ( + isType(type) && + param.constraint?.valueType && + !( + param.constraint.type && + ignoreDiagnostics(isTypeAssignableTo(type, param.constraint.type, type)) + ) + ) { const converted = legacy_tryTypeToValueCast( type, { kind: "argument", type: param.constraint.valueType }, From 64f89d914060149e7a460d14fbea70a5857dff37 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 11 Jul 2024 12:34:20 -0700 Subject: [PATCH 2/4] Create fix-expression-cast-to-object-2024-6-11-19-32-52.md --- .../fix-expression-cast-to-object-2024-6-11-19-32-52.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .chronus/changes/fix-expression-cast-to-object-2024-6-11-19-32-52.md diff --git a/.chronus/changes/fix-expression-cast-to-object-2024-6-11-19-32-52.md b/.chronus/changes/fix-expression-cast-to-object-2024-6-11-19-32-52.md new file mode 100644 index 0000000000..968edd1ca6 --- /dev/null +++ b/.chronus/changes/fix-expression-cast-to-object-2024-6-11-19-32-52.md @@ -0,0 +1,8 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: fix +packages: + - "@typespec/compiler" +--- + +Do not cast model expression to object value if the constraint is allowing the type From 30c1e5634be1f7517d419653e32c4e86affbaf12 Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 11 Jul 2024 12:42:12 -0700 Subject: [PATCH 3/4] fix --- packages/compiler/src/core/checker.ts | 23 +++++++++++-------- .../test/checker/values/object-values.test.ts | 6 +++++ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/packages/compiler/src/core/checker.ts b/packages/compiler/src/core/checker.ts index 31c4444ebd..4bd74c586d 100644 --- a/packages/compiler/src/core/checker.ts +++ b/packages/compiler/src/core/checker.ts @@ -971,6 +971,18 @@ export function createChecker(program: Program): Checker { kind: "argument" | "assignment"; type: Type; } + + function canTryLegacyCast( + target: Type, + constraint: MixedParameterConstraint | undefined + ): constraint is MixedParameterConstraint & + Required> { + return Boolean( + constraint?.valueType && + !(constraint.type && ignoreDiagnostics(isTypeAssignableTo(target, constraint.type, target))) + ); + } + /** * Gets a type or value depending on the node and current constraint. * For nodes that can be both type or values(e.g. string), the value will be returned if the constraint expect a value of that type even if the constrain also allows the type. @@ -986,7 +998,7 @@ export function createChecker(program: Program): Checker { if (entity === null) { return entity; } else if (isType(entity)) { - if (valueConstraint) { + if (canTryLegacyCast(entity, constraint?.constraint)) { return legacy_tryTypeToValueCast(entity, valueConstraint, node); } else { return entity; @@ -1548,14 +1560,7 @@ export function createChecker(program: Program): Checker { ? finalMap.get(param.constraint.type)! : param.constraint; - if ( - isType(type) && - param.constraint?.valueType && - !( - param.constraint.type && - ignoreDiagnostics(isTypeAssignableTo(type, param.constraint.type, type)) - ) - ) { + if (isType(type) && canTryLegacyCast(type, param.constraint)) { const converted = legacy_tryTypeToValueCast( type, { kind: "argument", type: param.constraint.valueType }, diff --git a/packages/compiler/test/checker/values/object-values.test.ts b/packages/compiler/test/checker/values/object-values.test.ts index 2cfa75562d..af59f703b7 100644 --- a/packages/compiler/test/checker/values/object-values.test.ts +++ b/packages/compiler/test/checker/values/object-values.test.ts @@ -163,6 +163,12 @@ describe("(LEGACY) cast model to object value", () => { strictEqual(b.value, "bar"); }); + it("doesn't cast or emit diagnostic if constraint also allow models", async () => { + const entity = await compileValueOrType(`{a: string} | valueof {a: string}`, `{a: "b"}`); + strictEqual(entity.entityKind, "Type"); + strictEqual(entity.kind, "Model"); + }); + it("emit a warning diagnostic", async () => { const { diagnostics, pos } = await diagnoseUsage(` model Test {} From 8c2f8e4863f1d77538d1331fb42dd07a1cc080fd Mon Sep 17 00:00:00 2001 From: Timothee Guerin Date: Thu, 11 Jul 2024 12:56:03 -0700 Subject: [PATCH 4/4] Tweak test and add array one --- .../test/checker/values/array-values.test.ts | 21 +++++++++++++++++-- .../test/checker/values/object-values.test.ts | 19 +++++++++++++---- .../compiler/test/checker/values/utils.ts | 11 ++++++---- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/packages/compiler/test/checker/values/array-values.test.ts b/packages/compiler/test/checker/values/array-values.test.ts index ebdb75147e..d5e1a59235 100644 --- a/packages/compiler/test/checker/values/array-values.test.ts +++ b/packages/compiler/test/checker/values/array-values.test.ts @@ -1,8 +1,14 @@ import { ok, strictEqual } from "assert"; import { describe, expect, it } from "vitest"; import { isValue } from "../../../src/index.js"; -import { expectDiagnostics } from "../../../src/testing/index.js"; -import { compileValue, compileValueOrType, diagnoseUsage, diagnoseValue } from "./utils.js"; +import { expectDiagnosticEmpty, expectDiagnostics } from "../../../src/testing/index.js"; +import { + compileAndDiagnoseValueOrType, + compileValue, + compileValueOrType, + diagnoseUsage, + diagnoseValue, +} from "./utils.js"; it("no values", async () => { const object = await compileValue(`#[]`); @@ -111,6 +117,17 @@ describe("(LEGACY) cast tuple to array value", () => { }); }); + it("doesn't cast or emit diagnostic if constraint also allow tuples", async () => { + const [entity, diagnostics] = await compileAndDiagnoseValueOrType( + `(valueof unknown) | unknown`, + `["foo"]`, + { disableDeprecatedSuppression: true } + ); + expectDiagnosticEmpty(diagnostics); + strictEqual(entity?.entityKind, "Type"); + strictEqual(entity.kind, "Tuple"); + }); + it("emit a error if element in tuple expression are not castable to value", async () => { const { diagnostics, pos } = await diagnoseUsage(` model Test {} diff --git a/packages/compiler/test/checker/values/object-values.test.ts b/packages/compiler/test/checker/values/object-values.test.ts index af59f703b7..12a52e03bd 100644 --- a/packages/compiler/test/checker/values/object-values.test.ts +++ b/packages/compiler/test/checker/values/object-values.test.ts @@ -1,8 +1,14 @@ import { ok, strictEqual } from "assert"; import { describe, expect, it } from "vitest"; import { isValue } from "../../../src/index.js"; -import { expectDiagnostics } from "../../../src/testing/index.js"; -import { compileValue, compileValueOrType, diagnoseUsage, diagnoseValue } from "./utils.js"; +import { expectDiagnosticEmpty, expectDiagnostics } from "../../../src/testing/index.js"; +import { + compileAndDiagnoseValueOrType, + compileValue, + compileValueOrType, + diagnoseUsage, + diagnoseValue, +} from "./utils.js"; it("no properties", async () => { const object = await compileValue(`#{}`); @@ -164,8 +170,13 @@ describe("(LEGACY) cast model to object value", () => { }); it("doesn't cast or emit diagnostic if constraint also allow models", async () => { - const entity = await compileValueOrType(`{a: string} | valueof {a: string}`, `{a: "b"}`); - strictEqual(entity.entityKind, "Type"); + const [entity, diagnostics] = await compileAndDiagnoseValueOrType( + `{a: string} | valueof {a: string}`, + `{a: "b"}`, + { disableDeprecatedSuppression: true } + ); + expectDiagnosticEmpty(diagnostics); + strictEqual(entity?.entityKind, "Type"); strictEqual(entity.kind, "Model"); }); diff --git a/packages/compiler/test/checker/values/utils.ts b/packages/compiler/test/checker/values/utils.ts index c1c895ca66..cf0fc7991b 100644 --- a/packages/compiler/test/checker/values/utils.ts +++ b/packages/compiler/test/checker/values/utils.ts @@ -65,7 +65,10 @@ export async function diagnoseValue(code: string, other?: string): Promise { const host = await createTestHost(); host.addJsFile("collect.js", { @@ -78,7 +81,7 @@ export async function compileAndDiagnoseValueOrType( import "./collect.js"; extern dec collect(target, value: ${constraint}); - #suppress "deprecated" "for testing" + ${disableDeprecatedSuppression ? "" : `#suppress "deprecated" "for testing"`} @collect(${code}) @test model Test {} ${other ?? ""} @@ -99,7 +102,7 @@ export async function compileValueOrType( code: string, other?: string ): Promise { - const [called, diagnostics] = await compileAndDiagnoseValueOrType(constraint, code, other); + const [called, diagnostics] = await compileAndDiagnoseValueOrType(constraint, code, { other }); expectDiagnosticEmpty(diagnostics); ok(called, "Decorator was not called"); @@ -111,6 +114,6 @@ export async function diagnoseValueOrType( code: string, other?: string ): Promise { - const [_, diagnostics] = await compileAndDiagnoseValueOrType(constraint, code, other); + const [_, diagnostics] = await compileAndDiagnoseValueOrType(constraint, code, { other }); return diagnostics; }