Skip to content
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

Do not cast model expression to object value if the constraint is allowing the type #3824

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -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
16 changes: 14 additions & 2 deletions packages/compiler/src/core/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Pick<MixedParameterConstraint, "valueType">> {
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.
Expand All @@ -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;
Expand Down Expand Up @@ -1548,7 +1560,7 @@ export function createChecker(program: Program): Checker {
? finalMap.get(param.constraint.type)!
: param.constraint;

if (isType(type) && param.constraint?.valueType) {
if (isType(type) && canTryLegacyCast(type, param.constraint)) {
const converted = legacy_tryTypeToValueCast(
type,
{ kind: "argument", type: param.constraint.valueType },
Expand Down
21 changes: 19 additions & 2 deletions packages/compiler/test/checker/values/array-values.test.ts
Original file line number Diff line number Diff line change
@@ -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(`#[]`);
Expand Down Expand Up @@ -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<T extends valueof string[]> {}
Expand Down
21 changes: 19 additions & 2 deletions packages/compiler/test/checker/values/object-values.test.ts
Original file line number Diff line number Diff line change
@@ -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(`#{}`);
Expand Down Expand Up @@ -163,6 +169,17 @@ 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, diagnostics] = await compileAndDiagnoseValueOrType(
`{a: string} | valueof {a: string}`,
`{a: "b"}`,
{ disableDeprecatedSuppression: true }
);
expectDiagnosticEmpty(diagnostics);
strictEqual(entity?.entityKind, "Type");
strictEqual(entity.kind, "Model");
});

it("emit a warning diagnostic", async () => {
const { diagnostics, pos } = await diagnoseUsage(`
model Test<T extends valueof {a: string}> {}
Expand Down
11 changes: 7 additions & 4 deletions packages/compiler/test/checker/values/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ export async function diagnoseValue(code: string, other?: string): Promise<reado
export async function compileAndDiagnoseValueOrType(
constraint: string,
code: string,
other?: string
{
other,
disableDeprecatedSuppression,
}: { other?: string; disableDeprecatedSuppression?: boolean }
): Promise<[Type | Value | undefined, readonly Diagnostic[]]> {
const host = await createTestHost();
host.addJsFile("collect.js", {
Expand All @@ -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 ?? ""}
Expand All @@ -99,7 +102,7 @@ export async function compileValueOrType(
code: string,
other?: string
): Promise<Value | Type> {
const [called, diagnostics] = await compileAndDiagnoseValueOrType(constraint, code, other);
const [called, diagnostics] = await compileAndDiagnoseValueOrType(constraint, code, { other });
expectDiagnosticEmpty(diagnostics);
ok(called, "Decorator was not called");

Expand All @@ -111,6 +114,6 @@ export async function diagnoseValueOrType(
code: string,
other?: string
): Promise<readonly Diagnostic[]> {
const [_, diagnostics] = await compileAndDiagnoseValueOrType(constraint, code, other);
const [_, diagnostics] = await compileAndDiagnoseValueOrType(constraint, code, { other });
return diagnostics;
}
Loading