Fixed a printer crash caused by empty parameter modifiers

[Andarist]

fixes crash reported here: #60527 (comment)

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[Andarist]

The crash was caused by getPos rying to read .__pos that is meant to be added by onBeforeEmitNodeArray (called by emitDecoratorsAndModifiers, called by emitParameter). But since this list was empty it just bailed on it:

TypeScript/src/compiler/emitter.ts

Lines 4435 to 4436 in b3c67d3

	function emitDecoratorsAndModifiers(node: Node, modifiers: NodeArray<ModifierLike> | undefined, allowDecorators: boolean) {
	if (modifiers?.length) {

This fix tries to reuse existing p.modifiers (it feels like ensureParameter shouldn't try to los information). Since the parameters in question don't have any modifiers an undefined gets "reused". So then the printer doesn't crash on lacking .__pos because it doesn't even have see the related node array as it wasn't created in the first place.

This, of course, means that this can still crash, in a similar way, on empty modifiers if they are ever returned.

I think it would be best to keep the current fix (it avoids allocating a redundant empty array) but also call onBeforeEmitNodeArray on empty modifiers in emitDecoratorsAndModifiers. I glanced through the whole emitter.ts and it seems like it's the only function that avoids it. Do you think there is some extra test case to be written that would prove it needs to be done there?

This, of course, means that any similar

[Andarist]

@jakebailey perhaps you could take a look at this one?

[jakebailey]

Why not just set it to undefined?

Note that declaration emit does something entirely different (see the other ensureParameter).

[Andarist]

Why not just set it to undefined?

It felt safer to use p.modifiers since some modifiers could be present on this. updateParameterDeclaration isn't like a partial that updates only what has been passed in. It requires everything to be passed in at each invocation.

But knowing a little bit more about this now... this probably should use reuseNode or similar to cover for that possibility 🤔

Note that declaration emit does something entirely different (see the other ensureParameter).

That maps modifierMask to modifiers - it ultimately isn't that different, I think, from what happens here. It's just that here we actually might have access to the modifiers so there is no need to create them from a mask (which we don't have here).

I'll try to come up with a test case that would have a parameter modifier to see what happens.

[jakebailey]

What's here is already the empty array, so we can't have copied anything over to date.

I don't think there should be any parameter modifiers, honestly; I thought that could only be used for something like parameter properties, which are not emitted in d.ts files.

[Andarist]

yeah, you are right - using undefined here is better. I've also played around with constructor and modifiers but it's not even treated as a class member completion and it isn't suggested so it can lead to any funkiness here.