Add source mappings for serialized properties with available declaration

[Andarist]

fixes #60004

[jakebailey]

I still feel at least a little uncomfortable with this; why isn't it declaration emit which can do this? The node builder produces lots of nodes, why doesn't this happen elsewhere?

[jakebailey]

Also, can this just be done in getPropertyNameNodeForSymbol?

(In any case, this PR is safe, since getPropertyNameNodeForSymbol is returning a new node.)

[Andarist]

I still feel at least a little uncomfortable with this; why isn't it declaration emit which can do this?

the required information is on the symbol, emit works based on the produced nodes and we need to pass somehow the information that it needs from here as this is the place where this information is being lost. This just leaves a breadcrumb that can be picked up by the emitter

Also, can this just be done in getPropertyNameNodeForSymbol?

maybe, im not sure sure ;p I'll try to analyze this

[weswigham]

For declaration emit nodes, just use setTextRange rather than setSourceMapRange - there's a helper in scope that takes care of a bunch of sanity checking for you that's absent here. But yeah, you should also be able to move this into getPropertyNameNodeForSymbol so it happens for all callers.

[Andarist]

Using setTextRange doesn't cut it and the results are as on the main branch. Am I using it wrong or missing something?

I have moved the fix to getPropertyNameNodeForSymbol though

[jakebailey]

Yeah, it doesn't seem to help. I don't understand why this isn't working:

propertyNameNode = setTextRange(context, propertyNameNode, declaration.name);

So I would expect the original node stuff to "just work". setTextRange doesn't call setSourceMapRange at all, though, so...

[jakebailey]

If you stick setSourceMapRange(range, location); into setTextRange after setOriginalNode, that actually also changes a whole bunch of other stuff kinda for the better? But then breaks some other JSDoc related code (which I think was another recent bugfix that may conflict?) Dunno if that's a good idea or not, I don't have a good mental model of this.

[jakebailey]

(maybe #59675?)

[Andarist]

Somewhat offtopic, somewhat not. I've wanted to verify some existing behaviors around this so I put this in the code:

diff --git a/src/compiler/factory/emitNode.ts b/src/compiler/factory/emitNode.ts
index beadc29d4c..b4d1e730eb 100644
--- a/src/compiler/factory/emitNode.ts
+++ b/src/compiler/factory/emitNode.ts
@@ -13,6 +13,7 @@ import {
     ImportSpecifier,
     InternalEmitFlags,
     isParseTreeNode,
+    isTypeNodeKind,
     Node,
     NodeArray,
     orderedRemoveItem,
@@ -137,6 +138,9 @@ export function getSourceMapRange(node: Node): SourceMapRange {
  * Sets a custom text range to use when emitting source maps.
  */
 export function setSourceMapRange<T extends Node>(node: T, range: SourceMapRange | undefined): T {
+    if (range && isTypeNodeKind(node.kind)) {
+        throw new Error("Type nodes cannot have source map ranges.");
+    }
     getOrCreateEmitNode(node).sourceMapRange = range;
     return node;
 }

and no existing test has thrown. So it seems that this has never been used on type nodes until now.

[Eigilak]

Status?

[Jacse]

@Andarist @jakebailey is this progressing?

[mamlzy]

sorry for tagging @Andarist, is this pr going to be merged?

[prometx11]

Is there any chance for this to be merged soon?
This is blocking go-to navigation in monorepos, and degrades DX quite a bit.

[jakebailey]

This needs main merged into it, for one thing.

The other thing I'm wondering is how trivial this is to port over to the Go codebase, just to make sure that this sticks.