Skip to content

Commit

Permalink
Do not run decorators that have missing args (microsoft#2959)
Browse files Browse the repository at this point in the history
  • Loading branch information
timotheeguerin authored and markcowl committed Mar 8, 2024
1 parent 7a178fe commit e5d9422
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 5 deletions.
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"
---

Decorators that have missing arguments will not run. This is inline with passing invalid argument to a decorator that would prevent it from running.
6 changes: 6 additions & 0 deletions packages/compiler/src/core/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3605,6 +3605,12 @@ export function createChecker(program: Program): Checker {
: declaration.parameters.length;

if (args.length < minArgs || (maxArgs !== undefined && args.length > maxArgs)) {
// In the case we have too little args then this decorator is not applicable.
// If there is too many args then we can still run the decorator as long as the args are valid.
if (args.length < minArgs) {
hasError = true;
}

if (maxArgs === undefined) {
reportCheckerDiagnostic(
createDiagnostic({
Expand Down
22 changes: 17 additions & 5 deletions packages/compiler/test/checker/decorators.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,9 @@ describe("compiler: checker: decorators", () => {

describe("usage", () => {
let runner: BasicTestRunner;
let calledArgs: any[];
let calledArgs: any[] | undefined;
beforeEach(() => {
calledArgs = undefined;
testHost.addJsFile("test.js", {
$testDec: (...args: any[]) => (calledArgs = args),
});
Expand All @@ -115,6 +116,7 @@ describe("compiler: checker: decorators", () => {
});

function expectDecoratorCalledWith(target: unknown, ...args: unknown[]) {
ok(calledArgs, "Decorator was not called.");
strictEqual(calledArgs.length, 2 + args.length);
strictEqual(calledArgs[0].program, runner.program);
strictEqual(calledArgs[1], target);
Expand All @@ -123,6 +125,10 @@ describe("compiler: checker: decorators", () => {
}
}

function expectDecoratorNotCalled() {
strictEqual(calledArgs, undefined);
}

it("calls a decorator with no argument", async () => {
const { Foo } = await runner.compile(`
extern dec testDec(target: unknown);
Expand Down Expand Up @@ -183,20 +189,22 @@ describe("compiler: checker: decorators", () => {
code: "invalid-argument-count",
message: "Expected 2 arguments, but got 1.",
});
expectDecoratorNotCalled();
});

it("errors if not calling with too many arguments", async () => {
const diagnostics = await runner.diagnose(`
extern dec testDec(target: unknown, arg1: string, arg2?: string);
const [{ Foo }, diagnostics] = await runner.compileAndDiagnose(`
extern dec testDec(target: unknown, arg1: valueof string, arg2?: valueof string);
@testDec("one", "two", "three")
model Foo {}
@test model Foo {}
`);

expectDiagnostics(diagnostics, {
code: "invalid-argument-count",
message: "Expected 1-2 arguments, but got 3.",
});
expectDecoratorCalledWith(Foo, "one", "two");
});

it("errors if not calling with argument and decorator expect none", async () => {
Expand Down Expand Up @@ -225,6 +233,7 @@ describe("compiler: checker: decorators", () => {
code: "invalid-argument-count",
message: "Expected at least 1 arguments, but got 0.",
});
expectDecoratorNotCalled();
});

it("errors if target type is incorrect", async () => {
Expand All @@ -239,6 +248,7 @@ describe("compiler: checker: decorators", () => {
code: "decorator-wrong-target",
message: "Cannot apply @testDec decorator to Foo since it is not assignable to Union",
});
expectDecoratorNotCalled();
});

it("errors if argument is not assignable to parameter type", async () => {
Expand All @@ -253,6 +263,7 @@ describe("compiler: checker: decorators", () => {
code: "invalid-argument",
message: "Argument '123' is not assignable to parameter of type 'string'",
});
expectDecoratorNotCalled();
});

it("errors if argument is not assignable to rest parameter type", async () => {
Expand All @@ -273,6 +284,7 @@ describe("compiler: checker: decorators", () => {
message: "Argument '456' is not assignable to parameter of type 'string'",
},
]);
expectDecoratorNotCalled();
});

describe("value marshalling", () => {
Expand All @@ -284,7 +296,7 @@ describe("compiler: checker: decorators", () => {
@test
model Foo {}
`);
return calledArgs[2];
return calledArgs![2];
}

describe("passing a string literal", () => {
Expand Down

0 comments on commit e5d9422

Please sign in to comment.