Improve declaration emit type safety.

[dragomirtitian]

No description provided.

[h-joo]

I love these changes

[jakebailey]

@rbuckton Do you have any thoughts on this? It doesn't look like any of these visitors actually need the entire transformation context.

[tchetwin]

Hey @rbuckton, @jakebailey, we have some further changes that depend on these, could you please take a look?

[rbuckton]

I don't think anything actually uses CoreTransformationContext anymore (chainBundle does, but currently its only ever used in places where a full transformation context is available). I'd originally added it as part of the node factory API refactor in microsoft#35282, with the intent to leverage it in microsoft#39084, but ended up not needing it. It might be better to just do away with CoreTransformationContext entirely, and just have any customized TransformationContext impelementations patch in unsupported methods from nullTransformationContext.

[dragomirtitian]

nullTransformationContext throw exceptions for some of the methods:

{
    getEmitResolver: notImplemented,
    getEmitHost: notImplemented,
    getEmitHelperFactory: notImplemented,
    //....
 }

My problem is that this is not exactly safe. There are no guarantees that those methods will not get called.

This is especially problematic now in the declaration transform, as this will contain both paths that rely on getEmitHost and getEmitResolver (the regular TSC declaration emit) but also paths that should not call these (when called from transformaDeclaration). I would like to make sure that it's not possible to mistakenly depend on emitHost and emitResolver if they are not present.

[rbuckton]

You would get an error if you tried to depend on them and they didn't exist, though I can see your point about leveraging the lack of these methods to ensure you don't accidentally reach for a capability that isn't present. I'm mildly concerned that having too many versions of TransformationContext will have the potential to impact performance if the same functions in the declaration transformer witnesses different object shapes and ends up with polymorphic ICs. If there is a performance issue, it likely won't show up in our normal benchmarks since they're unlikely to exercise code paths that would trigger such an IC transition. It will be something to keep an eye on, though.

I'm fine with switching these to CoreTransformationContext.

[rbuckton]

Why does this need its own version of TransformationContext if both are marked @internal? TransformationContext already has getEmitResolver(), getCompilerOptions(), factory, and addDiagnostic. If the only reason is the useLocalInferenceTypePrint field, then I would argue that it doesn't belong on the context.

[dragomirtitian]

Yes but IsolatedTransformationContext provides an emit resolver that has fewer features than the full resolver (just the CoreEmitResolver). I would ideally want this to be obvious in the declaration transform.

We could add implementations that just throw for everything that is not supported, but that would be hard to follow and prone to mistakes in the declaration transform (someone might accidentally call one of these un-implemented methods)

Right now we have a discriminated union in declaration emit that ensures you need to check if what you actually have available before you use it (useLocalInferenceTypePrint and isolatedDeclarations both act as discriminants)

TypeScript/src/compiler/transformers/declarations.ts

Line 320 in f9aa1b6

const { localInferenceResolver, isolatedDeclarations, host, resolver, symbolTracker, ensureNoInitializer, useLocalInferenceTypePrint } = createTransformerServices();

[rbuckton]

This doesn't seem like it belongs on a TransformationContext. Can you provide some additional context on what this field implies and why it belongs here?

[dragomirtitian]

Currently the declaration transform can either use the full checker type printer or it can use a new simpler printer that infers information only from the local declaration.

Initially I considered using the minimal type printer always when isolatedDeclarations is enabled. @DanielRosenwasser suggested however that the new minimal type printer should not be used unless we are in transpileDeclaration which is the only context in which the checker might actually not exist. This useLocalInferenceTypePrint is the way to turn on or off the usage of the minimal type printer.

We could also put it as an extra optional parameter on transformDeclarations that switches this on. Or we can add it to CompilerOptions (as an internal property). Adding it directly to the context seemed nicer as it acts as a discriminant on the union of TransformationContext | IsolatedDeclarationContext

[rbuckton]

If both CoreEmitResolver and EmitResolver are internal, why do we need two interfaces? Is it just to allocate a special EmitResolver that doesn't implement the whole interface?

[dragomirtitian]

Yes exactly. From transformDeclaration we don't provide the whole resolver, only a small subset that can be reasonably implemented without the full type checker.

[rbuckton]

I don't think anything actually uses CoreTransformationContext anymore (chainBundle does, but currently its only ever used in places where a full transformation context is available). I'd originally added it as part of the node factory API refactor in microsoft#35282, with the intent to leverage it in microsoft#39084, but ended up not needing it. It might be better to just do away with CoreTransformationContext entirely, and just have any customized TransformationContext impelementations patch in unsupported methods from nullTransformationContext.

[rbuckton]

The overload seems a bit overcomplicated since we'll only ever create one nullTransformationContext. You could just leave nullTransformationContext as is with the exception of giving it the type NullTransformationContext and adding the kind, and then just use return { ...nullTransformationContext, kind: TransformationContext.IsolatedContext, <whatever overrides you want> }. I realize that's pretty much what you had before, but I think kind is actually a bit clearer than the boolean field you were using previously.

[rbuckton]

Removing these results in a different shape than a normal transformation context, which is observable to the runtime during normal compilation since the null context is used by the command line compiler. I'd prefer we keep these in for the null context.

[dragomirtitian]

Since some of the removed members throw an exception at runtime, I would not expose them on the NullTransformationContext type. If someone somewhere is actually using the nullTransformationContext as a full transformation context I kept the nullTransformationContext to have the same runtime members but hid it behind the more restrictive NullTransformationContext

Or did you have in mind that the NullTransformationContext type is more similar to a full TransformationContext. I don't particularly like that idea since some of them throw runtime exceptions so ideally the type system should not let you access those members.

[rbuckton]

NullTransformationContext has always just been a normal TransformationContext with the features that depended on a type checker or program disabled. Having the shape remain stable avoids V8 recompiling large chunks of tsc.js because the checker sees one shape when it creates synthetic nodes when producing diagnostics via typeToTypeNode and another shape when it sees a full checker when doing emit.

[rbuckton]

I don't particularly like that idea since some of them throw runtime exceptions so ideally the type system should not let you access those members.

If we want the type to follow the shape but limit access, we could always define a NullTransformationContext type that makes the return types of those methods never.

[dragomirtitian]

@rbuckton I changed NullTransformationContext to have the same shape as TransformationContext (just the not implemented methods return never)