Skip to content

Commit

Permalink
Fix: Augment decorators might not run if alias caused the type to be …
Browse files Browse the repository at this point in the history 
…resolved too early (#2603)

Issue is that when resolving symbol in the meta types and member
resolution when we resolve an alias or template instance we check the
type. However this makes it that every referenced typed from there on
has already been resolved by the time the augment decorator gets to do
the binding.

A fix for now is to have an option in the symnbol resolution to
checkTemplateTypes or not. We do not check them during the binding phase
but we do allow checking during the checking phase
[#2600](#2600)
  • Loading branch information
@timotheeguerin
timotheeguerin authored Oct 30, 2023
1 parent 553bfc4 commit 474dd12
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 25 deletions.
10 changes: 10 additions & 0 deletions common/changes/@typespec/compiler/fix-augment-symbol-already-checked_2023-10-25-00-16.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/compiler",
"comment": "Fix: Issue where referencing a template in an alias might cause augment decorators to not be applied on types referenced in the aliased type.",
"type": "none"
}
],
"packageName": "@typespec/compiler"
}
98 changes: 73 additions & 25 deletions packages/compiler/src/core/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1858,14 +1858,14 @@ export function createChecker(program: Program): Checker {
function resolveIdentifierInTable(
node: IdentifierNode,
table: SymbolTable | undefined,
resolveDecorator = false
options: SymbolResolutionOptions
): Sym | undefined {
if (!table) {
return undefined;
}
table = augmentedSymbolTables.get(table) ?? table;
let sym;
if (resolveDecorator) {
if (options.resolveDecorators) {
sym = table.get("@" + node.sv);
} else {
sym = table.get(node.sv);
Expand Down Expand Up @@ -1921,7 +1921,11 @@ export function createChecker(program: Program): Checker {
}

lateBindMembers(containerType, container);
sym = resolveIdentifierInTable(id, container.exports ?? container.members);
sym = resolveIdentifierInTable(
id,
container.exports ?? container.members,
defaultSymbolResolutionOptions
);
break;
case IdentifierKind.Other:
return undefined;
Expand Down Expand Up @@ -1976,7 +1980,7 @@ export function createChecker(program: Program): Checker {
let base = resolveTypeReferenceSym(identifier.parent.base, undefined, false);
if (base) {
if (base.flags & SymbolFlags.Alias) {
base = getAliasedSymbol(base, undefined);
base = getAliasedSymbol(base, undefined, defaultSymbolResolutionOptions);
}
if (base) {
if (isTemplatedNode(base.declarations[0])) {
Expand Down Expand Up @@ -2075,7 +2079,7 @@ export function createChecker(program: Program): Checker {
function resolveIdentifierInScope(
node: IdentifierNode,
mapper: TypeMapper | undefined,
resolveDecorator = false
options: SymbolResolutionOptions
): Sym | undefined {
compilerAssert(
node.parent?.kind !== SyntaxKind.MemberExpression || node.parent.id !== node,
Expand All @@ -2094,12 +2098,12 @@ export function createChecker(program: Program): Checker {
while (scope && scope.kind !== SyntaxKind.TypeSpecScript) {
if (scope.symbol && "exports" in scope.symbol) {
const mergedSymbol = getMergedSymbol(scope.symbol);
binding = resolveIdentifierInTable(node, mergedSymbol.exports, resolveDecorator);
binding = resolveIdentifierInTable(node, mergedSymbol.exports, options);
if (binding) return binding;
}

if ("locals" in scope) {
binding = resolveIdentifierInTable(node, scope.locals, resolveDecorator);
binding = resolveIdentifierInTable(node, scope.locals, options);
if (binding) return binding;
}

Expand All @@ -2110,7 +2114,7 @@ export function createChecker(program: Program): Checker {
// check any blockless namespace decls
for (const ns of scope.inScopeNamespaces) {
const mergedSymbol = getMergedSymbol(ns.symbol);
binding = resolveIdentifierInTable(node, mergedSymbol.exports, resolveDecorator);
binding = resolveIdentifierInTable(node, mergedSymbol.exports, options);

if (binding) return binding;
}
Expand All @@ -2119,11 +2123,11 @@ export function createChecker(program: Program): Checker {
const globalBinding = resolveIdentifierInTable(
node,
globalNamespaceNode.symbol.exports,
resolveDecorator
options
);

// check using types
const usingBinding = resolveIdentifierInTable(node, scope.locals, resolveDecorator);
const usingBinding = resolveIdentifierInTable(node, scope.locals, options);

if (globalBinding && usingBinding) {
reportAmbiguousIdentifier(node, [globalBinding, usingBinding]);
Expand All @@ -2146,20 +2150,25 @@ export function createChecker(program: Program): Checker {
function resolveTypeReferenceSym(
node: TypeReferenceNode | MemberExpressionNode | IdentifierNode,
mapper: TypeMapper | undefined,
resolveDecorator = false
options?: Partial<SymbolResolutionOptions> | boolean
): Sym | undefined {
if (mapper === undefined && referenceSymCache.has(node)) {
return referenceSymCache.get(node);
}
const sym = resolveTypeReferenceSymInternal(node, mapper, resolveDecorator);

const resolvedOptions: SymbolResolutionOptions =
typeof options === "boolean"
? { ...defaultSymbolResolutionOptions, resolveDecorators: options }
: { ...defaultSymbolResolutionOptions, ...(options ?? {}) };
const sym = resolveTypeReferenceSymInternal(node, mapper, resolvedOptions);
referenceSymCache.set(node, sym);
return sym;
}

function resolveTypeReferenceSymInternal(
node: TypeReferenceNode | MemberExpressionNode | IdentifierNode,
mapper: TypeMapper | undefined,
resolveDecorator = false
options: SymbolResolutionOptions
): Sym | undefined {
if (hasParseError(node)) {
// Don't report synthetic identifiers used for parser error recovery.
Expand All @@ -2168,7 +2177,7 @@ export function createChecker(program: Program): Checker {
}

if (node.kind === SyntaxKind.TypeReference) {
return resolveTypeReferenceSym(node.target, mapper, resolveDecorator);
return resolveTypeReferenceSym(node.target, mapper, options);
}

if (node.kind === SyntaxKind.MemberExpression) {
Expand All @@ -2179,21 +2188,21 @@ export function createChecker(program: Program): Checker {

// when resolving a type reference based on an alias, unwrap the alias.
if (base.flags & SymbolFlags.Alias) {
base = getAliasedSymbol(base, mapper);
base = getAliasedSymbol(base, mapper, options);
if (!base) {
return undefined;
}
}

if (node.selector === ".") {
return resolveMemberInContainer(node, base, mapper, resolveDecorator);
return resolveMemberInContainer(node, base, mapper, options);
} else {
return resolveMetaProperty(node, base);
}
}

if (node.kind === SyntaxKind.Identifier) {
const sym = resolveIdentifierInScope(node, mapper, resolveDecorator);
const sym = resolveIdentifierInScope(node, mapper, options);
if (!sym) return undefined;

return sym.flags & SymbolFlags.Using ? sym.symbolSource : sym;
Expand All @@ -2206,10 +2215,10 @@ export function createChecker(program: Program): Checker {
node: MemberExpressionNode,
base: Sym,
mapper: TypeMapper | undefined,
resolveDecorator: boolean
options: SymbolResolutionOptions
) {
if (base.flags & SymbolFlags.Namespace) {
const symbol = resolveIdentifierInTable(node.id, base.exports, resolveDecorator);
const symbol = resolveIdentifierInTable(node.id, base.exports, options);
if (!symbol) {
reportCheckerDiagnostic(
createDiagnostic({
Expand Down Expand Up @@ -2247,7 +2256,7 @@ export function createChecker(program: Program): Checker {

return undefined;
} else if (base.flags & SymbolFlags.MemberContainer) {
if (isTemplatedNode(base.declarations[0])) {
if (options.checkTemplateTypes && isTemplatedNode(base.declarations[0])) {
const type =
base.flags & SymbolFlags.LateBound
? base.type!
Expand All @@ -2256,7 +2265,7 @@ export function createChecker(program: Program): Checker {
lateBindMembers(type, base);
}
}
const sym = resolveIdentifierInTable(node.id, base.members!, resolveDecorator);
const sym = resolveIdentifierInTable(node.id, base.members!, options);
if (!sym) {
reportCheckerDiagnostic(
createDiagnostic({
Expand Down Expand Up @@ -2287,7 +2296,10 @@ export function createChecker(program: Program): Checker {
}

function resolveMetaProperty(node: MemberExpressionNode, base: Sym) {
const resolved = resolveIdentifierInTable(node.id, base.metatypeMembers);
const resolved = resolveIdentifierInTable(node.id, base.metatypeMembers, {
resolveDecorators: false,
checkTemplateTypes: false,
});
if (!resolved) {
reportCheckerDiagnostic(
createDiagnostic({
Expand Down Expand Up @@ -2326,8 +2338,23 @@ export function createChecker(program: Program): Checker {
* (i.e. they contain symbols we don't know until we've instantiated the type and the type is an
* instantiation) we late bind the container which creates the symbol that will hold its members.
*/
function getAliasedSymbol(aliasSymbol: Sym, mapper: TypeMapper | undefined): Sym | undefined {
const aliasType = getTypeForNode(aliasSymbol.declarations[0] as AliasStatementNode, mapper);
function getAliasedSymbol(
aliasSymbol: Sym,
mapper: TypeMapper | undefined,
options: SymbolResolutionOptions
): Sym | undefined {
const node = aliasSymbol.declarations[0];
const targetNode = node.kind === SyntaxKind.AliasStatement ? node.value : node;
const sym = resolveTypeReferenceSymInternal(targetNode as any, mapper, options);
if (sym === undefined) {
return undefined;
}
const resolvedTargetNode = sym.declarations[0];
if (!options.checkTemplateTypes || !isTemplatedNode(resolvedTargetNode)) {
return sym;
}

const aliasType = getTypeForNode(node as AliasStatementNode, mapper);
if (isErrorType(aliasType)) {
return undefined;
}
Expand Down Expand Up @@ -2827,7 +2854,9 @@ export function createChecker(program: Program): Checker {
table.set("parameters", node.signature.parameters.symbol);
table.set("returnType", node.signature.returnType.symbol);
} else {
const sig = resolveTypeReferenceSym(node.signature.baseOperation, undefined);
const sig = resolveTypeReferenceSym(node.signature.baseOperation, undefined, {
checkTemplateTypes: false,
});
if (sig) {
visit(sig.declarations[0], sig);
const sigTable = getOrCreateAugmentedSymbolTable(sig.metatypeMembers!);
Expand Down Expand Up @@ -6076,3 +6105,22 @@ enum Related {
true = 1,
maybe = 2,
}

interface SymbolResolutionOptions {
/**
* Should resolving the symbol lookup for decorators as well.
* @default false
*/
resolveDecorators: boolean;

/**
* Should the symbol resolution instantiate templates and do a late bind of symbols.
* @default true
*/
checkTemplateTypes: boolean;
}

const defaultSymbolResolutionOptions: SymbolResolutionOptions = {
resolveDecorators: false,
checkTemplateTypes: true,
};
31 changes: 31 additions & 0 deletions packages/compiler/test/checker/augment-decorators.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ describe("compiler: checker: augment decorators", () => {
},
});
});

it("can be defined at the root of document", async () => {
testHost.addTypeSpecFile(
"test.tsp",
Expand Down Expand Up @@ -119,6 +120,36 @@ describe("compiler: checker: augment decorators", () => {
const { Foo } = await testHost.compile("test.tsp");
strictEqual(Foo, blueThing);
});

// Regression for https://github.com/microsoft/typespec/issues/2600
it("alias of instantiated template doesn't interfere with augment decorators", async () => {
// Here we could have add an issue where `Foo` would have been checked before the `@@blue` augment decorator could be run
// As we resolve the member symbols and meta types early,
// alias `FactoryString` would have checked the template instance `Factory<string>`
// which would then have checked `Foo` and then `@@blue` wouldn't have been run
testHost.addTypeSpecFile(
"test.tsp",
`
import "./test.js";
@test model Foo {};
interface Factory<T> {
op Action(): Foo;
}
alias FactoryString = Factory<string>;
op test is FactoryString.Action;
@@doc(Foo, "This doc");
@@blue(Foo);
`
);

const { Foo } = await testHost.compile("test.tsp");
strictEqual(Foo, blueThing);
});
});

describe("augment types", () => {
Expand Down

0 comments on commit 474dd12

Please sign in to comment.