Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add source mappings for serialized properties with available declaration #60005

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add source mappings for serialized properties with available declaration #60005

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
11ebae9
Add source mappings for serialized properties with available declaration
Andarist Sep 19, 2024
c5c2fe7
update baselines
Andarist Sep 19, 2024
3a84cca
Merge remote-tracking branch 'origin/main' into fix/mapped-members-ma…
Andarist Sep 19, 2024
5a8bc8b
revert change to writeFile
Andarist Sep 19, 2024
aeaede2
use `protocolFileLocationFromSubstring`
Andarist Sep 20, 2024
4b57ecb
use `setTextRangeWorker`
Andarist Sep 25, 2024
c25efdc
Merge remote-tracking branch 'origin/main' into fix/mapped-members-ma…
Andarist Sep 25, 2024
ff71ac9
Revert "use `setTextRangeWorker`"
Andarist Sep 25, 2024
5dac6a6
move the fix to `getPropertyNameNodeForSymbol`
Andarist Sep 25, 2024
c8ab65c
Merge remote-tracking branch 'origin/main' into fix/mapped-members-ma…
Andarist Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,7 @@ import {
setNodeFlags,
setOriginalNode,
jakebailey marked this conversation as resolved.
Show resolved Hide resolved
setParent,
setSourceMapRange,
setSyntheticLeadingComments,
setTextRange as setTextRangeWorker,
setTextRangePosEnd,
Expand Down Expand Up @@ -7176,8 +7177,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
context.tracker.reportNonSerializableProperty(symbolToString(propertySymbol));
}
}
context.enclosingDeclaration = propertySymbol.valueDeclaration || propertySymbol.declarations?.[0] || saveEnclosingDeclaration;
const propertyDeclaration = propertySymbol.valueDeclaration || propertySymbol.declarations?.[0];
context.enclosingDeclaration = propertyDeclaration || saveEnclosingDeclaration;
const propertyName = getPropertyNameNodeForSymbol(propertySymbol, context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can this just be done in getPropertyNameNodeForSymbol?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(maybe #59675?)

Copy link
Contributor Author

@Andarist Andarist Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (propertyDeclaration && (isPropertyAssignment(propertyDeclaration) || isShorthandPropertyAssignment(propertyDeclaration) || isMethodDeclaration(propertyDeclaration) || isMethodSignature(propertyDeclaration) || isPropertySignature(propertyDeclaration) || isPropertyDeclaration(propertyDeclaration) || isGetOrSetAccessorDeclaration(propertyDeclaration))) {
setSourceMapRange(propertyName, propertyDeclaration.name);
}
context.enclosingDeclaration = saveEnclosingDeclaration;
context.approximateLength += symbolName(propertySymbol).length + 1;

Expand Down
79 changes: 64 additions & 15 deletions src/testRunner/unittests/tsserver/projectReferencesSourcemap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import * as ts from "../../_namespaces/ts.js";
import { dedent } from "../../_namespaces/Utils.js";
import { jsonToReadableText } from "../helpers.js";
import {
baselineTsserverLogs,
closeFilesForSession,
createHostWithSolutionBuild,
openFilesForSession,
protocolFileLocationFromSubstring,
TestSession,
TestSessionRequest,
} from "../helpers/tsserver.js";
Expand All @@ -14,11 +16,11 @@ import {
} from "../helpers/virtualFileSystemWithWatch.js";

describe("unittests:: tsserver:: projectReferencesSourcemap:: with project references and tsbuild source map", () => {
const dependecyLocation = `/user/username/projects/myproject/dependency`;
const dependecyDeclsLocation = `/user/username/projects/myproject/decls`;
const dependencyLocation = `/user/username/projects/myproject/dependency`;
const dependencyDeclsLocation = `/user/username/projects/myproject/decls`;
const mainLocation = `/user/username/projects/myproject/main`;
const dependencyTs: File = {
path: `${dependecyLocation}/FnS.ts`,
path: `${dependencyLocation}/FnS.ts`,
content: `export function fn1() { }
export function fn2() { }
export function fn3() { }
Expand All @@ -27,7 +29,7 @@ export function fn5() { }
`,
};
const dependencyConfig: File = {
path: `${dependecyLocation}/tsconfig.json`,
path: `${dependencyLocation}/tsconfig.json`,
content: jsonToReadableText({ compilerOptions: { composite: true, declarationMap: true, declarationDir: "../decls" } }),
};

Expand Down Expand Up @@ -64,8 +66,8 @@ fn5();
path: `/user/username/projects/myproject/random/tsconfig.json`,
content: "{}",
};
const dtsLocation = `${dependecyDeclsLocation}/FnS.d.ts`;
const dtsMapLocation = `${dependecyDeclsLocation}/FnS.d.ts.map`;
const dtsLocation = `${dependencyDeclsLocation}/FnS.d.ts`;
const dtsMapLocation = `${dependencyDeclsLocation}/FnS.d.ts.map`;

const files = [dependencyTs, dependencyConfig, mainTs, mainConfig, randomFile, randomConfig];

Expand Down Expand Up @@ -142,7 +144,7 @@ fn5();
}

type OnHostCreate = (host: TestServerHost) => void;
function createSessionWithoutProjectReferences(onHostCreate?: OnHostCreate) {
function createSessionWithoutProjectReferences(files: File[], onHostCreate?: OnHostCreate) {
const host = createHostWithSolutionBuild(files, [mainConfig.path]);
// Erase project reference
writeConfigWithoutProjectReferences(host);
Expand All @@ -159,13 +161,13 @@ fn5();
);
}

function createSessionWithProjectReferences(onHostCreate?: OnHostCreate) {
function createSessionWithProjectReferences(files: File[], onHostCreate?: OnHostCreate) {
const host = createHostWithSolutionBuild(files, [mainConfig.path]);
onHostCreate?.(host);
return new TestSession(host);
}

function createSessionWithDisabledProjectReferences(onHostCreate?: OnHostCreate) {
function createSessionWithDisabledProjectReferences(files: File[], onHostCreate?: OnHostCreate) {
const host = createHostWithSolutionBuild(files, [mainConfig.path]);
// Erase project reference
WithDisabledProjectReferences(host);
Expand Down Expand Up @@ -227,16 +229,16 @@ fn5();
});
}

function createSession(type: SessionType, onHostCreate?: OnHostCreate) {
return type === SessionType.NoReference ? createSessionWithoutProjectReferences(onHostCreate) :
type === SessionType.ProjectReference ? createSessionWithProjectReferences(onHostCreate) :
function createSession(type: SessionType, files: File[], onHostCreate?: OnHostCreate) {
return type === SessionType.NoReference ? createSessionWithoutProjectReferences(files, onHostCreate) :
type === SessionType.ProjectReference ? createSessionWithProjectReferences(files, onHostCreate) :
type === SessionType.DisableSourceOfProjectReferenceRedirect ?
createSessionWithDisabledProjectReferences(onHostCreate) :
createSessionWithDisabledProjectReferences(files, onHostCreate) :
ts.Debug.assertNever(type);
}

function setup(type: SessionType, openFiles: readonly File[], action: Action | Action[], max?: number, onHostCreate?: OnHostCreate) {
const session = createSession(type, onHostCreate);
const session = createSession(type, files, onHostCreate);
openFilesForSession(openFiles, session);
runActions(session, action, max);
return session;
Expand Down Expand Up @@ -510,7 +512,7 @@ fn5();

verifyForAllSessionTypes(type => {
it("goto Definition in usage and rename locations, deleting config file", () => {
const session = createSession(type);
const session = createSession(type, files);
openFilesForSession([mainTs], session);
session.executeCommandSeq<ts.server.protocol.RenameRequest>({
command: ts.server.protocol.CommandTypes.Rename,
Expand Down Expand Up @@ -549,4 +551,51 @@ fn5();
});
}, /*options*/ undefined);
});

verifyForAllSessionTypes(type => {
it("goto Definition in usage of a property with mapped type origin", () => {
const dependencyTs: File = {
path: `${dependencyLocation}/api.ts`,
content: dedent`
type ValidateShape<T> = {
[K in keyof T]: T[K];
};

function getApi<T>(arg: ValidateShape<T>) {
function createCaller<T>(arg: T): () => {
[K in keyof T]: () => T[K];
} {
return null as any;
}
return {
createCaller: createCaller(arg),
};
}

const obj = getApi({
foo: 1,
bar: "",
});

export const createCaller = obj.createCaller;
`,
};
const mainTs: File = {
path: `${mainLocation}/main.ts`,
content: dedent`
import { createCaller } from "../decls/api";
const caller = createCaller();
caller.foo;
`,
};
const files = [dependencyTs, dependencyConfig, mainTs, mainConfig];
const session = createSession(type, files);
openFilesForSession([mainTs], session);
session.executeCommandSeq<ts.server.protocol.DefinitionAndBoundSpanRequest>({
command: ts.server.protocol.CommandTypes.DefinitionAndBoundSpan,
arguments: protocolFileLocationFromSubstring(mainTs, "foo"),
});
baselineTsserverLogs("projectReferencesSourcemap", `dependencyAndUsage/${type}/goto Definition in usage of a property with mapped type origin`, session);
});
}, /*options*/ undefined);
});
8 changes: 4 additions & 4 deletions tests/baselines/reference/declarationMapsMultifile.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 21 additions & 5 deletions tests/baselines/reference/declarationMapsMultifile.sourcemap.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,27 @@ sourceFile:a.ts
1 >Emitted(4, 6) Source(2, 27) + SourceIndex(0)
---
>>> b: number;
1->^^^^^^^^
2 > ^
1->) {
> return {
2 > b
1->Emitted(5, 9) Source(3, 17) + SourceIndex(0)
2 >Emitted(5, 10) Source(3, 18) + SourceIndex(0)
---
>>> };
>>> static make(): Foo;
1->^^^^
1 >^^^^
2 > ^^^^^^
3 > ^
4 > ^^^^
1->) {
> return {b: x.a};
1 >: x.a};
> }
>
2 > static
3 >
4 > make
1->Emitted(7, 5) Source(5, 5) + SourceIndex(0)
1 >Emitted(7, 5) Source(5, 5) + SourceIndex(0)
2 >Emitted(7, 11) Source(5, 11) + SourceIndex(0)
3 >Emitted(7, 12) Source(5, 12) + SourceIndex(0)
4 >Emitted(7, 16) Source(5, 16) + SourceIndex(0)
Expand Down Expand Up @@ -166,11 +173,20 @@ sourceFile:index.ts
4 >Emitted(3, 21) Source(6, 13) + SourceIndex(0)
---
>>> b: number;
1 >^^^^
2 > ^
1 >
2 > ;
1 >Emitted(4, 5) Source(4, 19) + SourceIndex(0)
2 >Emitted(4, 6) Source(4, 20) + SourceIndex(0)
---
>>>};
1 >^
2 > ^
3 > ^^^^^^^^^^^^^^^^^->
1 > = c.doThing({a: 12})
1 >
>
>export let x = c.doThing({a: 12})
2 > ;
1 >Emitted(5, 2) Source(6, 34) + SourceIndex(0)
2 >Emitted(5, 3) Source(6, 35) + SourceIndex(0)
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/declarationMapsOutFile.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 23 additions & 7 deletions tests/baselines/reference/declarationMapsOutFile.sourcemap.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,20 +64,27 @@ sourceFile:a.ts
1 >Emitted(5, 10) Source(2, 27) + SourceIndex(0)
---
>>> b: number;
1->^^^^^^^^^^^^
2 > ^
1->) {
> return {
2 > b
1->Emitted(6, 13) Source(3, 17) + SourceIndex(0)
2 >Emitted(6, 14) Source(3, 18) + SourceIndex(0)
---
>>> };
>>> static make(): Foo;
1->^^^^^^^^
1 >^^^^^^^^
2 > ^^^^^^
3 > ^
4 > ^^^^
1->) {
> return {b: x.a};
1 >: x.a};
> }
>
2 > static
3 >
4 > make
1->Emitted(8, 9) Source(5, 5) + SourceIndex(0)
1 >Emitted(8, 9) Source(5, 5) + SourceIndex(0)
2 >Emitted(8, 15) Source(5, 11) + SourceIndex(0)
3 >Emitted(8, 16) Source(5, 12) + SourceIndex(0)
4 >Emitted(8, 20) Source(5, 16) + SourceIndex(0)
Expand Down Expand Up @@ -158,13 +165,22 @@ sourceFile:index.ts
5 >Emitted(14, 17) Source(6, 13) + SourceIndex(1)
---
>>> b: number;
1->^^^^^^^^
2 > ^
1->
2 > ;
1->Emitted(15, 9) Source(4, 19) + SourceIndex(1)
2 >Emitted(15, 10) Source(4, 20) + SourceIndex(1)
---
>>> };
1->^^^^^
1 >^^^^^
2 > ^
3 > ^^^^^^^^^^^^^^^^^->
1-> = c.doThing({a: 12})
1 >
>
>export let x = c.doThing({a: 12})
2 > ;
1->Emitted(16, 6) Source(6, 34) + SourceIndex(1)
1 >Emitted(16, 6) Source(6, 34) + SourceIndex(1)
2 >Emitted(16, 7) Source(6, 35) + SourceIndex(1)
---
>>> export { c, Foo };
Expand Down
Loading