Harden node builder APIs to no longer return undefined for a node when NodeBuilderFlags.IgnoreErrors is provided
#38273
Conversation
…hen `NodeBuilderFlags.IgnoreErrors` is provided
| @@ -2040,23 +2040,23 @@ declare namespace ts { | |||
| getNonNullableType(type: Type): Type; | |||
| getTypeArguments(type: TypeReference): readonly Type[]; | |||
| /** Note that the resulting nodes cannot be checked. */ | |||
| typeToTypeNode(type: Type, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): TypeNode | undefined; | |||
| typeToTypeNode(type: Type, enclosingDeclaration: Node | undefined, flags: NodeBuilderFlags | undefined): TypeNode | undefined; | |||
There was a problem hiding this comment.
This would be a break if we want it for 3.9. Any way we can avoid it?
There was a problem hiding this comment.
Easily - now that I've used it to audit our internal usages, swapping it back is pretty simple; changing it is just for safety at callsites (so we may still want it for the 4.0 branch, where we may eventually add more).
| @@ -4211,8 +4211,12 @@ namespace ts { | |||
| context.flags &= ~NodeBuilderFlags.InTypeAlias; | |||
|
|
|||
| if (!type) { | |||
| context.encounteredError = true; | |||
| return undefined!; // TODO: GH#18217 | |||
| if (!(context.flags & NodeBuilderFlags.AllowEmptyUnionOrIntersection)) { | |||
There was a problem hiding this comment.
As we discussed offline, I think this (and below) would be clearer as IgnoreErrors, but I can live with this version if it's more idiomatic.
There was a problem hiding this comment.
Ah, I struggled to enumerate it earlier, but if we check for NodeBuilderFlags.IgnoreErrors, then if any of the AllowX flags are set, the error would be suppressed (as that's how binary & works) - it's much neater to tie it to a specific flag (I'd have been tempted to add new ones, but we're actually entirely our of flag space in this enum).
|
@typescript-bot cherry-pick this to release-3.9 (once the PR is up, I'll edit it to undo the API changes) |
|
Heya @weswigham, I've started to run the task to cherry-pick this into |
Component commits: c80c177 Harden node builder APIs to no longer return `undefined` for a node when `NodeBuilderFlags.IgnoreErrors` is provided
|
Hey @weswigham, I've opened #38333 for you. |
…e-3.9 (#38333) * Cherry-pick PR #38273 into release-3.9 Component commits: c80c177 Harden node builder APIs to no longer return `undefined` for a node when `NodeBuilderFlags.IgnoreErrors` is provided * Undo API changes for release branch Co-authored-by: Wesley Wigham <t-weswig@microsoft.com> Co-authored-by: Wesley Wigham <wwigham@gmail.com>
|
This may also fix #32851 |
Fixes #32864
The root cause of the crash in #32864 was a very trivial error - at a callsite in
signatureHelp.ts, we neglected to pass in our normal set ofNodeBuilderFlagsthat's supposed to guarantee we always get a node back. Upon seeing this, I've done two things:flagsparameter is required (it can still explicitly beundefined, and is such in many codefixes, which hopefully can handle the resulting possiblyundefinedresult)context.encounteredErrorin the node builder to ensure all places where we set it are guarded by a check of a flag included inNodeBuilderFlags.IgnoreErrors, this way we know that if we provide that flag, even if the results might not be valid JS, they will, at least, exist as a node treeI've confirmed this fixes the repro provided in #32864, and have an appropriately similar fourslash test. Interestingly, it points out that our signature help in the presence of overloads is a little odd - the signature that is the currently "chosen" overload has its type parameters erased, while other overloads have their type parameters left in-place (a requirement for triggering the crash was that the signature must have had type parameters, which, because of this quirk, can only happen on overloaded signatures).