[ID-Prep] PR4 - Preserve types nodes from type assertions in declaration emit

[dragomirtitian]

Fixes #57587

[dragomirtitian]

Changed this to preserve the original intent of the test (test what happens when huge type nodes need to be printed).

[jakebailey]

Feels weird to have this detail in the transform; right now the only options that are used are ones explicitly related to declaration emit (noResolve/stripInternal).

My impression is that isolatedDeclarations is not assumed to know the compiler options; is the case that we can treat strictNullChecks as true for emit with the assumption that undefined/null are just going to be ignored by non-strict users?

[dragomirtitian]

There is a bit of strictNullChecks that needs to be taken into account. This is used here in the context of inferring optional property declaration types. If strictNullChecks is on we need to add undefined to the type otherwise we don't. Without taking this into account any local inference optional declarations will not be possible:

class X { 
	// x?: 1 | 2 | undefined; with !strictNullChecks
	// x?: 1 | 2; // with !strictNullChecks
	x? = 1 as 1 | 2 
}

Playground Link

[jakebailey]

Sure but if a user doesn't have strictNullChecks enabled, aren't the two equivalent? Since the way that strictNullChecks=false works is to effectively treat null/undefined as though they don't exist.

[dragomirtitian]

They are usually, but if you take the declaration from a project with !strictNullChecks and use them in a project exactOptionalPropertyTypes then the missing | undefined will make a difference.

[dragomirtitian]

Thinking about it, it might actually be an improvement. Because a !strictNullChecks project should be fine with a undefined.

I can change this to always add | undefined but the second problem is that it will make emit different between the case when we take the type from the checker (which does not add undefined) and when we try to get the type from the assertion. The solution would be to add the |undefined always form the checker as well.

Maybe this should be a different PR though? One that always adds |undefined to optional properties even if !strictNullChecks

[jakebailey]

I guess I see what you mean, in that this PR is only trying to touch assertions and therefore making this code unconditional would not change any other declarations?

[dragomirtitian]

Exactly. Even it's final form, !isolatedDeclarations some types will still come from the checker, I don't think adding undefined just sometimes would be a good thing.

[jakebailey]

I still don't feel too great about declaration emit directly knowing about strictNullChecks as I'm not sure how we are expecting third party emitters to also know this, but, I guess it's fine.

[rbuckton]

I still don't feel too great about declaration emit directly knowing about strictNullChecks as I'm not sure how we are expecting third party emitters to also know this, but, I guess it's fine.

That's a good point. Aside from strictOptionalProperties, what would be the harm in adding | undefined regardless of strictNullChecks? In strictNullChecks: false we would ignore the undefined anyways.

[jakebailey]

I think the reason I think it's "fine" is that dependence is already present today, just that it's hidden by calls to the checker. So as a whole, this PR is just an emit improvement and not actually regressing in that way. Third party tools are already able to resolve tsconigs, I just think it's frustrating that declaration emit is sensitive to it at all (but I'm also in the camp of wanting to ditch strictNullChecks=false altogether!)

[dragomirtitian]

I still don't feel too great about declaration emit directly knowing about strictNullChecks as I'm not sure how we are expecting third party emitters to also know this, but, I guess it's fine.

That's a good point. Aside from strictOptionalProperties, what would be the harm in adding | undefined regardless of strictNullChecks? In strictNullChecks: false we would ignore the undefined anyways.

My biggest issue is that it will be added sometimes. When the type is extracted in declarations, we will add undefined but when it comes from the type checker it will not be added:

type P = number | undefined
declare function getP(): P
class X {
    f1? = getP() as P; // P | undefined  with this PR
    f2? = getP(); // P
}

This seems like a strange difference that will make people open issues.

I think our options are:

1. Always add | undefined to optional fields, even if the type is inferred - A big emit change.
2. Only add it for type that come from the declaration emit - An inconsistent emit change.
3. We can just let go of this and not extract types for optional fields in declaration emit (and make it an error in ID) - Worse DX in ID
4. Allow the declaration emit to know about strictNullChecks.

I am not 100% we can keep strictNullChecks completely out of declaration emit in future PRs. When we add inference for objects, something like let o = { n: null, u: undefined } is typed as { n: any; u: any; } without strictNullChecks and { n: null, u: undefined } with it so. So the declaration emit will need to know what type to use there (unless we ban null and undefined there too)

[rbuckton]

We'll discuss this PR in the design meeting on Friday and assuming there are no other concerns around strictNullChecks, should be able to merge then.

[rbuckton]

@typescript-bot pack this

[typescript-bot]

Starting jobs; this comment will be updated as builds start and complete.

Command	Status	Results
pack this	✅ Started

[typescript-bot]

Hey @rbuckton, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/160236/artifacts?artifactName=tgz&fileId=5EB08BDD23C04BCCF2F81BCC237A591E7B83FA0F429777473E0AAE6247CC3D5002&fileName=/typescript-5.5.0-insiders.20240308.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

[weswigham]

IMO, this is something we could/should have done even without isolated declarations, and we've done things like this in the past. We already have machinery for cloning/reusing input type nodes in new locations inside the emit resolver (usually for signatures copied from one location to another by structural inlining) - this logic should be moved into it, too, so all the guardrails around node copying are in one place (and there are a fair few). Over in createTypeOfDeclaration and createReturnTypeOfSignatureDeclaration they both defer to nodeBuilder.typeToTypeNode - within the node builder, we have a serializeExistingTypeNode (and parent function serializeTypeForDeclaration used extensively by the JS declaration emitter) function that adds a lot of guard rails onto this node copying process, because, well, there's a lot that needs to be checked for that currently isn't in this PR.

IMO, this logic should be exposed on the nodeBuilder and then used by the EmitResolver's createTypeOfDeclaration and createReturnTypeOfSignatureDeclaration instead of typeToTypeNode directly (rather than built directly into the declarations transformer). That way it's easy to leverage the existing caching and safe copying mechanisms we have, and to reuse the logic in the JS declaration emitter (which, I dunno if you wanna consider testing that out of scope or not).

Outdated; see Wesley's comment

[dragomirtitian]

@weswigham

The goal of the PRs to align emit is to extract more information directly from the initialisers of declarations that have them. To this end we will extract extract types from:

1. Assertions (current PR)
2. Function expressions / Arrow functions
3. Array literals in const assertions. (This includes any primitive literals used within)
4. Object literals - in const and mutable contexts. (This includes any primitive literals used within)

The general idea is that the types in these cases are trivial enough that by looking at the initialization expressions it is possible to derive the type node, without having to go through the type checker. This approach may yield a more readable type in some cases but not necessarily. The end goal is to allow external tools to perform the same transformations, and to be able to guarantee that these transformations are indeed just syntactical ones that don’t use any type information that is not local to the initialiser.

Option 1: Transform initialisers to type nodes in the type checker (node builder)

This approach will have all the logic for extracting the types in the same place as type nodes are synthesized from type checker types. Copying of existing type nodes already happens in serializeExistingTypeNode.

Advantages

1. The serializeExistingTypeNode already does copying of nodes and tracking their usage. So this new functionality can reuse the same logic
2. Can be used in JS declaration emit as well.
3. Benefits from caching ? (Not really sure about this one)

Disadvantages

The declaration transform will not be runnable without the type checker. This creates several problems:

1. It becomes harder for other implementers to reason about what is the scope and functionality of types from initialisers.
2. Colocation of type from initializer extraction with the type checker makes it easy for maintainers down the line to reach for type information that is not guaranteed to be syntactically derivable just the initialiser.
3. It becomes impossible to prove that TypeScript is actually living up to the guarantee of generating these types without needing the type information. So "bugs" may arise where we find a third-party emitter cannot reasonably replicate TypeScript's emit.
4. Any transpileDeclaration that is added (now or later) will have to instantiate the type checker (transpileModule already does this and is 3-6 times slower than just running the JS transforms in isolation). To be explicit: if we do not choose Option 1, we could easily introduce and expose a fast transpileDeclaration() in the initial release. The only reason it is not in scope today is due to the guidance we have received. We would love to ship this API if it were permitted/appreciated.
5. Any future implementation of transpileDeclaration() that does not rely on the type checker will need to duplicate the logic to extract type from the initialiser .

Option 2: Transform initialisers in the declaration emit

This is the approach in the current PR. This approach will have all the logic for extracting types from initialisers in the declaration transform. The declaration transform already does other purely syntactic transformation of original source to ensure the correct types in the declaration (ex: constructor class fields are transformed to parameters and fields, expressions in heritage clauses are moved to variables that are independently typed, default exports are moved to variables)

Advantages

1. Reuses machinery to track the type use that already is in the declaration transform. This already existed as it was necessary when copying existing type node from source (they do not go through serializeExistingTypeNode).
2. All the code that required for emit in isolated declaration mode is in the transform, so it’s easier for other implementer to understand.
3. Since any communication with the type checker needs to happen though the emit resolver it’s harder for someone to accidentally use forbidden type knowledge in isolated declaration mode
4. Allows us to easily compare output of typescript with and without the type checker, ensuring that the declarations are equivalent (or if they are not we can document why the difference occurs). This has been invaluable in identifying cases where typescript does more work than is obvious at first glance (such as triple slash references)
5. transpileDeclaration() can be fast as it does not need the type checker.
6. A transpileDeclaration() implementation will require reimplementing a much smaller subset of what the emit resolver does. We already have a working version of this that is the backbone for our testing

Disadvantages

1. JS declaration emit will not benefit from these changes
2. ?

Final thoughts

While for the current PR the extraction of the type happens from an assertion, for the other kinds of type from initializer transformations, there is no actual type node to extract from. But rather we go through the literal and create a new appropriate type if possible (reusing as much of the original AST). Putting this second way of getting the type in the typechecker does not really seem like a win. While in some cases the resulting types will be more faithful to the original source, the extra type information can also result in better types from the type checker.

Just having the errors without actually extracting the type is also an option, but that makes the validation of external emitters much harder, as TypeScript may emit different declaration files (even if they are mostly semantically equivalent).

Extraction from the initializer is needed for isolated declarations, but if it is always better for types to come from this source is not necessarily clear, and it’s a question that is out of scope for the current effort.

Option 3: Factor out the shared logic

If we still want to reuse some of this logic in the type checker, maybe a better approach would be to factor out the type node from initializer logic into a separate component that can be reused in both serializeExistingTypeNode and in the declaration emit. This would allow the declaration transform to still be invoked without a type checker, preserving the advantages of the second approach.

[dragomirtitian]

Closed in favor of #57772