Fix missing undefined checks

[justsml]

Fixes #32851
Fixes #32864

Added defensive null checks to prevent my VS Code's 'window logs' from filling up with TypeScript Server Error (3.7.3) error messages every few minutes.

Tasks

• Add/Verify null checks for all calls to node.emitNode in src/compiler/factoryPublic.ts
• Verify tests.
• Checking for other related bugs in issues.

Update: All tests pass.

[msftclas]

All CLA requirements met.

[RyanCavanaugh]

@rbuckton please review, thanks!

[justsml]

Updates: I've noticed that this bug can cause a memory leak, and restarting the typescript process frees up the RAM.

Please let me know if there's anything I can do to help move this along.

I understand that this is not a very visible (or critical) bug. Reply whenever you can. 😇

I really appreciate your taking the time to review.
❤️

[sandersn]

@justsml one of the best things you can do is to provide a small repro of the problem. I will let @rbuckton review this PR in-depth, but generally we avoid fixes that skip a problem and prefer to fix root causes.

[justsml]

@sandersn
Thanks for the advice. And normally I'd provide 1 (or more) ways to reproduce.

The reason I didn't here: I'm not sure how to reproduce it consistently. I've tried.

Latest Observations

1. Might be triggered by viewing long .ts files w/ several layers of inheritance. "Quick fix" option in VS Code may also play a role.
2. Enabling tracing/logging on the TS Lang Server uses more RAM+CPU, making it hard to find performance-related issues. and I'm not experienced enough triaging between the 'layers'/services here. I'd appreciate any links to dev resources if anyone has them handy. 💙

I wish I had an easy reproduction, but these sorts of issues that only creep up after day(s) are hard to write steps for.
I'm willing to try if anyone has tips, it's just an area in QA I've struggled with in the past.

Impact (low)

1. Noisy logs. It can make it difficult to find messages you care about in VS Code's output log.
2. Seems to correlate with increase memory usage and TSServer timeouts.
3. Happens on a clean VS Code install. (Eventually. Leaving editor open for a 24+ hours seems to "help" get the error.)

[justsml]

And I agree, I'd prefer to find the root cause too. 😸

I realize this is just moving the 'Null Exception' up the stack. I'm hopeful it'll lead to more helpful stack traces closer to where the issue actually resides.

[jscheid]

I don't know about #32851, but for #32864 I have a reproduction here: https://github.com/jscheid/typescript-32864

[rbuckton]

While I understand the desire to add these checks, this isn't the source of the problem. Rather, somewhere there is an unsound cast to a non-nullable type. I'd much rather find that root cause if possible as it is likely to cause other instabilities elsewhere.

[rbuckton]

We shouldn't need this. We initialize the value if it isn't set two lines before this.

[rbuckton]

If we're getting an error here its because we are somehow being passed a node that is undefined however we expect a defined node.

[weswigham]

Probably a symbol.valueDeclaration via some means, since that's a known hole in our current type definitions.

[rbuckton]

Both #32851 and #32864 seem to be caused by this unchecked assertion in itemInfoForParameters (signatureHelp.ts):

  const args = createNodeArray(candidateSignature.typeParameters.map(p =>
      checker.typeParameterToDeclaration(p, enclosingDeclaration)!));
//                                                               ^

[rbuckton]

It seems the reason for this is that we return undefined when we encounter an error when attempting to create the node. I would propose that the correct solution would be to modify itemInfoForParameters to just create a dummy type parameter for this case, something like:

...
checker.typeParameterToDeclaration(p, enclosingDeclaration) ??
    createTypeParameterDeclaration('?')
...

[rbuckton]

@justsml Do want to take on my proposed solution, or should I go ahead with a separate PR to fix this instead?

[weswigham]

@rbuckton didn't #38273 fix those?

[rbuckton]

Ah, I didn't see that. Yes, it looks like it does cover this case.

[justsml]

@rbuckton
Thanks, I wish I coulda taken a swing at that solution. Next time though!

And thanks @weswigham for solving the root cause. 😻