Skip to content

Commit

Permalink
Prevent use of augment decorators on template instantiations (#1744)
Browse files Browse the repository at this point in the history 
* Initial work.

* Fix #1354.

* Progress?

* Tests pass.

* Code review feedback.
  • Loading branch information
@tjprescott
tjprescott authored Mar 31, 2023
1 parent 2c6edb4 commit 85b5abf
Show file tree
Hide file tree
Showing 3 changed files with 159 additions and 58 deletions.
10 changes: 10 additions & 0 deletions common/changes/@typespec/compiler/fixTemplateTarget_2023-03-27-17-15.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/compiler",
"comment": "Prevent use of augment decorators on instantiated templates.",
"type": "none"
}
],
"packageName": "@typespec/compiler"
}
14 changes: 12 additions & 2 deletions packages/compiler/core/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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",
Expand Down Expand Up @@ -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) {
Expand Down
193 changes: 137 additions & 56 deletions packages/compiler/test/checker/augment-decorators.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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");
}

Expand All @@ -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<T> {
testProp: T;
};
@test
op stringTest(): Foo<string>;
@@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<T> {
testProp: T;
};
alias StringFoo = Foo<string>;
@test
op stringTest(): Foo<string>;
@@customName(Foo<string>, "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 {}
}
Expand All @@ -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")
`);
});
});
});
});

0 comments on commit 85b5abf

Please sign in to comment.