From 85b5abf934a7e9ed0a292c340499c96ef2f26afe Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Fri, 31 Mar 2023 13:08:22 -0700 Subject: [PATCH] Prevent use of augment decorators on template instantiations (#1744) * Initial work. * Fix #1354. * Progress? * Tests pass. * Code review feedback. --- .../fixTemplateTarget_2023-03-27-17-15.json | 10 + packages/compiler/core/checker.ts | 14 +- .../test/checker/augment-decorators.test.ts | 193 +++++++++++++----- 3 files changed, 159 insertions(+), 58 deletions(-) create mode 100644 common/changes/@typespec/compiler/fixTemplateTarget_2023-03-27-17-15.json diff --git a/common/changes/@typespec/compiler/fixTemplateTarget_2023-03-27-17-15.json b/common/changes/@typespec/compiler/fixTemplateTarget_2023-03-27-17-15.json new file mode 100644 index 0000000000..2fad6f58d4 --- /dev/null +++ b/common/changes/@typespec/compiler/fixTemplateTarget_2023-03-27-17-15.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/compiler", + "comment": "Prevent use of augment decorators on instantiated templates.", + "type": "none" + } + ], + "packageName": "@typespec/compiler" +} \ No newline at end of file diff --git a/packages/compiler/core/checker.ts b/packages/compiler/core/checker.ts index 717dd1a910..077f1cd47e 100644 --- a/packages/compiler/core/checker.ts +++ b/packages/compiler/core/checker.ts @@ -467,6 +467,15 @@ export function createChecker(program: Program): Checker { for (const decNode of augmentDecorators) { const ref = resolveTypeReferenceSym(decNode.targetType, undefined); if (ref) { + let args: readonly Expression[] = []; + if (ref.declarations[0].kind === SyntaxKind.AliasStatement) { + const aliasNode = ref.declarations[0] as AliasStatementNode; + if (aliasNode.value.kind === SyntaxKind.TypeReference) { + args = aliasNode.value.arguments; + } + } else { + args = decNode.targetType.arguments; + } if (ref.flags & SymbolFlags.Namespace) { const links = getSymbolLinks(getMergedSymbol(ref)); const type: Type & DecoratedType = links.type! as any; @@ -475,7 +484,7 @@ export function createChecker(program: Program): Checker { type.decorators.push(decApp); applyDecoratorToType(program, decApp, type); } - } else if (ref.flags & SymbolFlags.LateBound) { + } else if (args.length > 0 || ref.flags & SymbolFlags.LateBound) { reportCheckerDiagnostic( createDiagnostic({ code: "augment-decorator-target", @@ -3070,8 +3079,9 @@ export function createChecker(program: Program): Checker { ) { const sym = isMemberNode(node) ? getSymbolForMember(node) ?? node.symbol : node.symbol; const decorators: DecoratorApplication[] = []; + const augmentDecoratorNodes = augmentDecoratorsForSym.get(sym) ?? []; const decoratorNodes = [ - ...((sym && augmentDecoratorsForSym.get(sym)) ?? []), // the first decorator will be executed at last, so augmented decorator should be placed at first. + ...augmentDecoratorNodes, // the first decorator will be executed at last, so augmented decorator should be placed at first. ...node.decorators, ]; for (const decNode of decoratorNodes) { diff --git a/packages/compiler/test/checker/augment-decorators.test.ts b/packages/compiler/test/checker/augment-decorators.test.ts index 9e4bfd7655..c2e3b16dd2 100644 --- a/packages/compiler/test/checker/augment-decorators.test.ts +++ b/packages/compiler/test/checker/augment-decorators.test.ts @@ -1,6 +1,6 @@ -import { strictEqual } from "assert"; -import { Type } from "../../core/types.js"; -import { createTestHost, TestHost } from "../../testing/index.js"; +import { deepEqual, strictEqual } from "assert"; +import { Model, Operation, Type } from "../../core/types.js"; +import { createTestHost, expectDiagnosticEmpty, TestHost } from "../../testing/index.js"; describe("compiler: checker: augment decorators", () => { let testHost: TestHost; @@ -144,9 +144,10 @@ describe("compiler: checker: augment decorators", () => { ` ); - const { target } = await testHost.compile("test.tsp"); - strictEqual(runOnTarget?.kind, target.kind); - strictEqual(runOnTarget, target); + const [result, diagnostics] = await testHost.compileAndDiagnose("test.tsp"); + expectDiagnosticEmpty(diagnostics); + strictEqual(runOnTarget?.kind, result.target.kind); + strictEqual(runOnTarget, result.target); strictEqual(customName, "FooCustom"); } @@ -170,37 +171,116 @@ describe("compiler: checker: augment decorators", () => { it("interface", () => expectTarget(`@test("target") interface Foo { }`, "Foo")); it("operation in interface", () => expectTarget(`interface Foo { @test("target") list(): void }`, "Foo.list")); - }); + it("uninstantiated template", async () => { + testHost.addJsFile("test.js", { + $customName(_: any, t: Type, n: string) { + const runOnTarget: Type | undefined = t; + const customName: string | undefined = n; + if (runOnTarget) { + } + if (customName) { + } + }, + }); - describe("augment location", () => { - async function expectAugmentTarget(code: string) { - let customName: string | undefined; - let runOnTarget: Type | undefined; + testHost.addTypeSpecFile( + "test.tsp", + ` + import "./test.js"; + + model Foo { + testProp: T; + }; + + @test + op stringTest(): Foo; + + @@customName(Foo, "Some foo thing"); + @@customName(Foo.testProp, "Some test prop"); + ` + ); + const [results, diagnostics] = await testHost.compileAndDiagnose("test.tsp"); + expectDiagnosticEmpty(diagnostics); + const stringTest = results.stringTest as Operation; + strictEqual(stringTest.kind, "Operation"); + deepEqual((stringTest.returnType as Model).decorators[0].args[0].value, { + kind: "String", + value: "Some foo thing", + }); + for (const prop of (stringTest.returnType as Model).properties) { + deepEqual(prop[1].decorators[0].args[0].value, { + kind: "String", + value: "Some test prop", + }); + } + }); + it("emit diagnostic if target is instantiated template", async () => { testHost.addJsFile("test.js", { $customName(_: any, t: Type, n: string) { - runOnTarget = t; - customName = n; + const runOnTarget: Type | undefined = t; + const customName: string | undefined = n; + if (runOnTarget) { + } + if (customName) { + } }, }); testHost.addTypeSpecFile( "test.tsp", ` + import "./test.js"; + + model Foo { + testProp: T; + }; + + alias StringFoo = Foo; + + @test + op stringTest(): Foo; + + @@customName(Foo, "A string Foo"); + @@customName(StringFoo, "A string Foo"); + ` + ); + const diagnostics = await testHost.diagnose("test.tsp"); + strictEqual(diagnostics.length, 2); + for (const diagnostic of diagnostics) { + strictEqual(diagnostic.message, "Cannot reference template instances"); + } + }); + + describe("augment location", () => { + async function expectAugmentTarget(code: string) { + let customName: string | undefined; + let runOnTarget: Type | undefined; + + testHost.addJsFile("test.js", { + $customName(_: any, t: Type, n: string) { + runOnTarget = t; + customName = n; + }, + }); + + testHost.addTypeSpecFile( + "test.tsp", + ` import "./test.js"; ${code} ` - ); + ); - const { target } = await testHost.compile("test.tsp"); - strictEqual(runOnTarget?.kind, target.kind); - strictEqual(runOnTarget, target); - strictEqual(customName, "FooCustom"); - } + const { target } = await testHost.compile("test.tsp"); + strictEqual(runOnTarget?.kind, target.kind); + strictEqual(runOnTarget, target); + strictEqual(customName, "FooCustom"); + } - it("augment type in another namespace", async () => { - await expectAugmentTarget(` + it("augment type in another namespace", async () => { + await expectAugmentTarget(` namespace Lib { @test("target") model Foo {} } @@ -209,72 +289,73 @@ describe("compiler: checker: augment decorators", () => { @@customName(Lib.Foo, "FooCustom") } `); - }); + }); - it("augment type in another file checked before", async () => { - testHost.addTypeSpecFile("lib.tsp", `@test("target") model Foo {} `); + it("augment type in another file checked before", async () => { + testHost.addTypeSpecFile("lib.tsp", `@test("target") model Foo {} `); - await expectAugmentTarget(` + await expectAugmentTarget(` import "./lib.tsp"; @@customName(Foo, "FooCustom") `); - }); + }); - it("augment type in another file checked after", async () => { - testHost.addTypeSpecFile("lib.tsp", `@@customName(Foo, "FooCustom") `); + it("augment type in another file checked after", async () => { + testHost.addTypeSpecFile("lib.tsp", `@@customName(Foo, "FooCustom") `); - await expectAugmentTarget(` + await expectAugmentTarget(` import "./lib.tsp"; @test("target") model Foo {} `); - }); - }); - - describe("augment order", () => { - async function expectAugmentTarget(code: string) { - let customName: string | undefined; - let runOnTarget: Type | undefined; - - testHost.addJsFile("test.js", { - $customName(_: any, t: Type, n: string) { - runOnTarget = t; - customName = n; - }, }); + }); - testHost.addTypeSpecFile( - "test.tsp", - ` + describe("augment order", () => { + async function expectAugmentTarget(code: string) { + let customName: string | undefined; + let runOnTarget: Type | undefined; + + testHost.addJsFile("test.js", { + $customName(_: any, t: Type, n: string) { + runOnTarget = t; + customName = n; + }, + }); + + testHost.addTypeSpecFile( + "test.tsp", + ` import "./test.js"; ${code} ` - ); + ); - const { target } = await testHost.compile("test.tsp"); - strictEqual(runOnTarget?.kind, target.kind); - strictEqual(runOnTarget, target); - strictEqual(customName, "FooCustom"); - } + const { target } = await testHost.compile("test.tsp"); + strictEqual(runOnTarget?.kind, target.kind); + strictEqual(runOnTarget, target); + strictEqual(customName, "FooCustom"); + } - it("augment decorator should be applied at last", async () => { - await expectAugmentTarget(` + it("augment decorator should be applied at last", async () => { + await expectAugmentTarget(` @test("target") @customName("Foo") model Foo {} @@customName(Foo, "FooCustom") `); - }); + }); - it("augment decorator - last win", async () => { - await expectAugmentTarget(` + it("augment decorator - last win", async () => { + await expectAugmentTarget(` @test("target") @customName("Foo") model Foo {} @@customName(Foo, "NonCustom") @@customName(Foo, "FooCustom") `); + }); }); }); });