[compiler] Refactor checker to use context, fixing template decorator bugs

[witemple-msft]

This PR fixes several bugs related to the logic around running decorators on templates. Current decorator logic is insufficient because it is based on whether the mapper is set and the node has template arguments, but we have cases where this logic breaks down, like:

alias F<T> = {
  @dec prop: T; // dec runs here and observes uninstantiated `T`!
}

model X<T1> {
  prop: Y<T1>;
}

alias Y<T2> = {
  // This dec runs _twice_, once in the context of `Y`, observing uninstantiated `T2`, and
  // then _again_ in the context of `X` -> `Y<T1>`, observing uninstantiated `T1`.
  @dec prop: T2;
}

This change fixes those problems by refactoring the checker to work with an immutable CheckContext that is passed through the checker API. Checking always begins with CheckContext.DEFAULT at any checker API entrypoint. The context carries (a) the current TypeMapper, (b) an integer set of flags (there is currently one flag: CheckFlags.InTemplateDeclaration), and (c) a set of observed template parameter instances in a certain scope.

There are no public API changes to the Checker, only internal changes to allow the Checker itself to pass CheckContext to functions that are also exposed in the public API without that option.

The context manages the state required to track whether the checker is currently declaring a template and how it should behave as it traverses references to declared types.

• When the checker visits a declaration, it activates CheckFlags.InTemplateDeclaration if there is no type mapper, and the node has template parameters.
• When the checker goes through a type reference, it deactivates CheckFlags.InTemplateDeclaration if the type reference is concrete (does not contain a template parameter).
• Under this model, logic around running decorators entirely collapses to ctx.hasFlags(CheckFlags.InTemplateDeclaration), and decorators will only run on fully concrete instances and will be inhibited on any type instance that references a template parameter.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/microsoft/typespec/@typespec/compiler@9588

commit: f48892a

[github-actions]

All changed packages have been documented.

• ✅ @typespec/compiler

Show changes

@typespec/compiler - fix ✏️

Fixed several checking errors around template instantiations that could cause TemplateParameter instances to leak into decorator calls.

[azure-sdk]

You can try these changes here

🛝 Playground	🌐 Website	🛝 VSCode Extension

[timotheeguerin]

Do you think it would be worth splitting this 2 flags(Declaration + Template) so you could easily have InTemplateInstance too?

[witemple-msft]

It wouldn't really be splitting this flag into two, since this flag would continue to have the same behavior. As far as I can tell, there isn't a use case for "am I within an instantiation of a template" as a predicate. I haven't studied it extremely deeply, though. I was only focusing on this "am I a type that is part of an uninstantiated template" question.

[timotheeguerin]

there is just quite a few checks in the checker where we want to know if we should finish the type and this involve checking if its either a non template declaration or a template instance

seems like its contained in this function

but I guess its an internal thing so we could always add the split later

[witemple-msft]

shouldRunDecorators is deleted in this branch and replaced with ctx.hasFlags(CheckFlags.InTemplateDeclaration) everywhere. Basically we always run decorators if the type is concrete, and never run decorators if the type is part of (is or references unbound parameters of) an uninstantiated template.

One semantic change to how the flag is calculated vs. how this function works in main is that it isn't good enough to check if t.kind === "TemplateParameter". That catches cases where the argument evaluates exactly to an unbound parameter, but not cases where the argument evaluates to something that contains an unbound parameter. The new logic addresses that among other things.

[timotheeguerin]

I think this one might be able to simplify to use the new flag

[witemple-msft]

I don't think it does, because this logic is for a templated operation declaration inside an interface instance.

interface A<T> {
  op b<U>(param: T): U;
}

// Activates codepath in question with mapper. InTemplateDeclaration is off
// when checking `A` because it is concrete, but reactivated when checking
// `b` within the instance because `b` is templated.
interface AA extends A<string>;

// Now `b` from the concrete instance of `A` is instantiated and the flag is off
// again.
op example is AA.b<numeric>;

Each concrete instance of A instantiates a new b that is itself a template. It inherits the mapper from the instance of A, but that mapper is partial because b is a template even in the context of a concrete instance of A. It's a bit of a different question.