Declaration maps and transparent goto definition using them #22658

weswigham · 2018-03-16T21:28:47Z

Adds one command line option: --declarationMap. When enabled alongside --declaration, it causes us to emit .d.ts.map files alongside the output .d.ts files. Services can also now understand these map files, and uses them to map declaration-file based definition locations to their original source, if possible.

Fixes #14479

There's a handful of TODOs that I want to talk about:

Do we need to recur and map again if the target file is also a declaration file? We won't do this, but tools that process TS may emit sourcemaps that indicate that we should.
Does the length of the textSpan for getTargetOfMappedDeclarationFile need to be mapped somehow?
Does the textSpan for getDefinitionAndBoundSpan need to be mapped in some way?
Do we need to report some kind of diagnostic (since this is in services, I'm not sure how we would) on invalid source map or silently fail (right now it silently fails)?
Should we read/emit inline declaration sourcemaps? (right now we do not)
Should we check a declaration file's sourceMappingURL comment? (probably, but right now we do not)
Should we support sourcemaps represented with the sections property (which would likely be beneficial for @RyanCavanaugh 's work)? We currently neither emit nor recognize them.

Additionally, this could still use some more tests exercising more of the many sourcemap options we have with declarationMaps. Do we have a better harness for testing things like this (ie, language service features that depend on compilation output)? Fourslash works (see declarationMapGoToDefinition.ts), but it's really tedious (and fragile), since I need to include the sourcemap within the test itself (rather than build the map for the test). Should I write a new harness/update an existing one?

weswigham · 2018-03-16T21:32:40Z

Oh, and @mjbvz do you have some concrete desires for this from the vscode perspective? I'd also like to use these maps to enable going to the JS associated with a .d.ts declaration, too, but that'll require a new LS command, I believe (and so I think will be left off of this PR).

RyanCavanaugh · 2018-03-16T21:45:55Z

Notes from offline discussion

Do we need to recur and map again if the target file is also a declaration file? We won't do this, but tools that process TS may emit sourcemaps that indicate that we should.

Yes - keep going until we hit a non-mappable intermediate

Does the length of the textSpan for getTargetOfMappedDeclarationFile need to be mapped somehow?

No - rename can just scanIdentifier to figure out the length and verify that we're renaming the right thing

Does the textSpan for getDefinitionAndBoundSpan need to be mapped in some way?

No - same as above

Do we need to report some kind of diagnostic (since this is in services, I'm not sure how we would) on invalid source map or silently fail (right now it silently fails)?

Report an error to the ts server log so we can diagnose by hand

Should we read/emit inline declaration sourcemaps? (right now we do not)

No

Should we check a declaration file's sourceMappingURL comment? (probably, but right now we do not)

Eventually

Should we support sourcemaps represented with the sections property (which would likely be beneficial for @RyanCavanaugh 's work)? We currently neither emit nor recognize them.

Support emit+read sections in a future PR

RyanCavanaugh · 2018-03-16T21:54:28Z

src/services/services.ts

+            if (file.sourceMapper) {
+                return file.sourceMapper;
+            }
+            // TODO (wewigham): Read sourcemappingurl from last line of .d.ts if present


RyanCavanaugh

Let's add the discussed testing methodology either here or in a future PR

mjbvz · 2018-03-16T22:29:23Z

On the vscode side, most of the requests have been to make go to implementation go to the actual TS source instead of the d.ts. Would this new map be used for that?

weswigham · 2018-03-16T22:33:56Z

@mjbvz Actually, it makes "Go to Definition" go to the original TS instead of the declaration file.

mjbvz · 2018-03-16T23:04:11Z

Ok, I think that makes sense. Is the idea that go to implementation will also use this too then? What about go to type definition?

weswigham · 2018-03-16T23:43:55Z

@mjbvz As is, this only affects getDefinitionAtPosition, getDefinitionAndBoundSpan, and getTypeDefinitionAtPosition, which I believe back go to definition, DefinitionAndBoundSpan (not sure what editor feature that maps to), and go to type definition, respectively. Is it appropriate to map locations for getImplementationAtPosition (which backs go to implementation) as well?

weswigham · 2018-03-17T00:05:47Z

@mjbvz I've enabled mappings for go to implementation as well. Are there any other endpoints it makes sense to map by default?

rbuckton

Early feedback. I need to spend some additional time reviewing the sourcemap support added to services.

rbuckton · 2018-03-19T21:41:54Z

src/compiler/emitter.ts

+        None,
+        File,
+        Inline,
+        DeclarationFile


Do we want to consider allowing inline declaration maps? The current behavior seems inconsistent because --inlineSourceMap seems to affect declaration maps in a different way than it affects --sourceMap in that we end up emitting both the inline comment and a separate map file. I'd prefer that we chose one of the two following behaviors:

--inlineSourceMap should have no impact on --declarationMaps, as we can introduce an --inlineDeclarationMap in the future if necessary.

--inlineSourceMap does affect --declarationMaps and we do not write a separate map file when set.

Barring feedback from other reviewers, I'd lean towards the former than the latter.

rbuckton · 2018-03-19T21:49:11Z

src/compiler/emitter.ts

            const bundle = sourceFileOrBundle.kind === SyntaxKind.Bundle ? sourceFileOrBundle : undefined;
            const sourceFile = sourceFileOrBundle.kind === SyntaxKind.SourceFile ? sourceFileOrBundle : undefined;
            const sourceFiles = bundle ? bundle.sourceFiles : [sourceFile];
-            sourceMap.initialize(jsFilePath, sourceMapFilePath, sourceFileOrBundle);
+            if (sourcemapKind !== SourceMapEmitKind.None) {


In sourcemap.ts we still set the initial state of disabled to !(compilerOptions.sourceMap || compilerOptions.inlineSourceMap). This seems unnecessary if we are conditionally enabling/disabling the state here.

rbuckton · 2018-03-19T21:50:26Z

src/compiler/utilities.ts

@@ -2819,7 +2819,8 @@ namespace ts {
    export interface EmitFileNames {
        jsFilePath: string;
        sourceMapFilePath: string;


Should this also be string | undefined?

rbuckton · 2018-03-19T21:51:45Z

src/compiler/commandLineParser.ts

@@ -194,6 +194,13 @@ namespace ts {
            category: Diagnostics.Basic_Options,
            description: Diagnostics.Generates_corresponding_d_ts_file,
        },
+        {
+            name: "declarationMaps",


Why is --declarationMaps plural when --sourceMap is singular?

rbuckton · 2018-03-19T21:52:23Z

src/compiler/program.ts

@@ -2111,7 +2111,7 @@ namespace ts {
                createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_with_option_1, "out", "outFile");
            }

-            if (options.mapRoot && !options.sourceMap) {
+            if (options.mapRoot && !(options.sourceMap || options.declarationMaps)) {


Though the condition has changed, the related diagnostic still only says "sourceMap".

rbuckton · 2018-03-19T21:52:41Z

src/compiler/program.ts

@@ -2125,6 +2125,12 @@ namespace ts {
                }
            }

+            if (options.declarationMaps) {


Unnecessary nested if, I would just merge the conditions.

rbuckton · 2018-03-19T22:09:36Z

src/services/services.ts

+    }
+
+    /* @internal */
+    export function getSourceFileLikeCache(host: { readFile?: (path: string) => string, fileExists?: (path: string) => boolean }): SourceFileLikeCache {


nit: createSourceFileLikeCache seems more appropriate.

rbuckton · 2018-03-19T22:17:46Z

src/services/services.ts

+            if (file.sourceMapper) {
+                return file.sourceMapper;
+            }
+            // TODO (weswigham): Read sourcemappingurl from last line of .d.ts if present


I would not recommend leaving this part unfinished. On the upside, if implemented its fairly easy to support inline declaration maps.

I would recommend you scan a file backwards until you encounter a newline. If that substring is not a sourceMapURL comment, continue with the preceding lines until you encounter a sourceMapURL comment. If you encounter a non-comment, non-whitespace line you break. If no match, you can attempt to look for a ".map" file in the same folder.

rbuckton · 2018-03-19T22:48:52Z

src/services/sourcemaps.ts

+
+    function decodeSingleSpan<T>(state: DecoderState<T>): void {
+        while (state.decodingIndex < state.encodedText.length) {
+            const char = state.encodedText.charAt(state.decodingIndex);


I would recommend using .charCodeAt and CharacterCodes

rbuckton · 2018-03-19T22:59:58Z

src/services/sourcemaps.ts

+            return decodedMappings || (decodedMappings = calculateDecodedMappings());
+        }
+
+        function getReverseSortedMappings() {


The names reverseSortedMappings and forwardSortedMappings aren't very clear. I'd prefer something more like sourceOrderedMappings and generatedOrderedMappings.

rbuckton · 2018-03-19T23:04:28Z

src/services/sourcemaps.ts

+        };
+
+        function getGeneratedPosition(loc: SourceMappableLocation): SourceMappableLocation {
+            const maps = filter(getForwardSortedMappings(), m => comparePaths(loc.fileName, m.sourcePath, sourceRoot) === 0);


Couldn't we binarySearch without the filter and then check whether the result points to a mapping in a different file? filter requires a full scan of the array prior to using the more efficient binarySearch.

rbuckton · 2018-03-19T23:04:34Z

src/services/sourcemaps.ts

+            return { fileName: toPath(map.file, sourceRoot, host.getCanonicalFileName), position: maps[targetIndex].emittedPosition }; // Closest span
+        }
+
+        function getOriginalPosition(loc: SourceMappableLocation): SourceMappableLocation {


Couldn't we binarySearch without the filter and then check whether the result points to a mapping in a different file? filter requires a full scan of the array prior to using the more efficient binarySearch.

Is this comment in error? The method it is on does not filter anything.

rbuckton · 2018-03-19T23:08:19Z

src/services/sourcemaps.ts

+            return forwardSortedMappings || (forwardSortedMappings = getDecodedMappings().slice().sort(compareProcessedSpanEmittedPositions));
+        }
+
+        function calculateDecodedMappings(): ProcessedSourceMapSpan[] {


This could probably be done without so many fields on state if you inline hasCompletedDecoding and decodeSingleSpan. It doesn't need to be done in this PR but it seems unnecessarily complex.

rbuckton · 2018-03-19T23:14:12Z

src/services/sourcemaps.ts

+        name?: string;
+    }
+
+    interface RawSourceMapSpan {


To avoid confusion, this should not be called "-Span" but "-Position".

rbuckton · 2018-03-19T23:14:28Z

src/services/sourcemaps.ts

+        return state.decodingIndex === state.encodedText.length;
+    }
+
+    function decodeSingleSpan<T>(state: DecoderState<T>): void {


To avoid confusion, this should not be called "-Span" but "-Position".

rbuckton · 2018-03-19T23:15:08Z

src/services/sourcemaps.ts

+                emittedPosition: getPositionOfLineAndCharacterUsingName(map.file, currentDirectory, span.emittedLine - 1, span.emittedColumn - 1),
+                sourcePosition: getPositionOfLineAndCharacterUsingName(sourcePath, sourceRoot, span.sourceLine - 1, span.sourceColumn - 1),
+                sourcePath,
+                name: span.nameIndex ? map.names[span.nameIndex] : undefined


We don't actually use names anywhere in the results, so this is unnecessary work.

rbuckton · 2018-03-19T23:21:28Z

src/services/sourcemaps.ts

+
+    function isSourceMappingSegmentEnd(encodedText: string, pos: number) {
+        return (pos === encodedText.length ||
+            encodedText.charAt(pos) === "," ||


It would be better to use .charCodeAt and CharacterCodes here.

rbuckton · 2018-03-19T23:26:46Z

src/services/sourcemaps.ts

+            }
+            // 5. Check if there is name:
+            if (!isSourceMappingSegmentEnd(state.encodedText, state.decodingIndex)) {
+                if (state.currentNameIndex === undefined) {


We don't use names, so should we care about the name index or whether it's invalid? We could just as easily advance the position until we hit the segment end. I'd rather air on the side of leniency for a better user experience.

… emitter

rbuckton

A few more minor comments.

rbuckton · 2018-03-23T23:40:00Z

src/compiler/emitter.ts

@@ -250,6 +258,11 @@ namespace ts {
            sourceMap.setSourceFile(node);
        }

+        function setSourceFileDeclarationMaps(node: SourceFile) {


This function name isn't very clear.

rbuckton · 2018-03-23T23:46:44Z

src/compiler/sys.ts

@@ -457,6 +457,7 @@ namespace ts {
        clearTimeout?(timeoutId: any): void;
        clearScreen?(): void;
        /*@internal*/ setBlocking?(): void;
+        base64decode?(input: string): string;


Shouldn't we also add an optional sys member for base64encode?

rbuckton · 2018-03-23T23:50:37Z

src/compiler/sys.ts

@@ -528,6 +529,8 @@ namespace ts {
              _crypto = undefined;
            }

+            const Buffer: typeof global.Buffer = require("buffer").Buffer;


Despite the fact that we ephemerally get the node.d.ts types due to the fact we're using them along with ts-node for our gulpfile, we don't depend on actual node types anywhere else in compiler. Yes, this makes this less type-safe but I don't necessary want to take the dependency for just one declaration in the entire project.

rbuckton · 2018-03-23T23:53:09Z

src/compiler/sys.ts

@@ -620,6 +623,9 @@ namespace ts {
                    if (process.stdout && process.stdout._handle && process.stdout._handle.setBlocking) {
                        process.stdout._handle.setBlocking(true);
                    }
+                },
+                base64decode: input => {
+                    return Buffer.from(input, "base64").toString("utf8");


Buffer.from was added in NodeJS v5.10.0. We should feature test for Buffer.from and either don't add base64decode if it isn't present, or fall back to the deprecated new Buffer(string, encoding) form that was used prior to NodeJS v6.0.0.

rbuckton · 2018-03-23T23:54:31Z

src/compiler/utilities.ts

+        return output;
+    }
+
+    export function base64decode(host: { base64decode?(input: string): string }, input: string): string {


We're inconsistent with naming between base64decode and convertToBase64 above it. Please choose a more consistent name.

rbuckton · 2018-03-23T23:58:40Z

src/services/services.ts

+                return;
+            }
+            const starts = getLineStarts(mappedFile);
+            for (let index = starts.length - 1; index--; index >= 0) {


You have your condition and incrementer swapped.

rbuckton · 2018-03-24T00:01:21Z

src/services/services.ts

+                return file.sourceMapper;
+            }
+            let mapFileName = scanForSourcemapURL(fileName);
+            if (mapFileName && dataURLRE.exec(mapFileName)) {


dataURLRE seems unnecessary. The same thing could be accomplished with a single RegExp.

I'm doing this to avoid bothering to look for a file at a data: URL we couldn't understand. It's likely the rest of the path machinery may handle arbitrary data URLs OK-ish... but do we want it to?

What I meant was that your regexp could be written like this /^data:(?:application\/json;charset=utf-8;base64,(.+)$)?/. Then you can exec the regexp once. If the result is non-null, its at least a data URL, but if it has a matches[1] then it's a valid base64 data URL.

Also, it might be better to be more forgiving in the regexp with respect to case sensitivity to the value of charset, since UTF-8 is just as acceptable as utf-8 (and is entirely optional). I'd recommend something only slightly more lenient like this:

/^data:(?:application\/json(?:;charset=[uU][tT][fF]-8)?;base64,(.+)$)?/

Also, if we wanted to be picky we could restrict the capturing group to ([A-Za-z0-9+\/=]+) (rather than (.+)).

Do we care if a different charset is supplied (like "US-ASCII" or "UTF-16"), or do we want to only allow UTF8?

I don't think we want to be in the game of supporting multiple encodings, and our builtin impl (ignoring what the platform can provide) only handles utf8.

Fair enough, though I do believe leveraging a single RegExp and possibly restricting the allowed values for the capturing group would be worthwhile.

rbuckton · 2018-03-24T00:02:21Z

src/services/services.ts

+
+        const sourceMapCommentRE = /^\/\/[@#] sourceMappingURL=(.+)$/gm;
+        const dataURLRE = /^data:/;
+        const base64URLRE = /^data:application\/json;charset=utf-8;base64,(.+)$/;


"URLRE" is a lot of sequential uppercase letters. I'd prefer a name like base64UrlRegExp.

weswigham · 2018-03-24T00:24:35Z

@rbuckton 👍 👎?

rbuckton · 2018-03-26T18:29:27Z

src/services/services.ts

-                if (b64EncodedMatch) {
-                    const base64Object = b64EncodedMatch[1];
+            let match: RegExpExecArray;
+            if (mapFileName && (match = base64UrlRegExp.exec(mapFileName))) {


nit: I'd just use a nested if to avoid having match declared outside of the block since it isn't used elsewhere.

michaelaird · 2018-06-11T21:46:35Z

Are there any editors that currently support this?

I tried in VS2017 (with Typescript 2.9.1 tooling installed) and VS Code (1.24.0) and both still go to the d.ts file on "go to definition".

mhegazy · 2018-06-11T21:48:19Z

were the .d.ts files built with --declarationMap?

michaelaird · 2018-06-11T21:51:05Z

Yes. At the bottom of the d.ts file there's a sourceMappingURL pointing at the emitted d.ts.map file.

That said, I am generating these with the latest build of gulp-typescript.

mhegazy · 2018-06-11T22:13:17Z

mind filing a new ticket with some repro steps?

HolgerJeromin · 2018-07-18T16:15:44Z

just as a reference: michael filed #25322 for that.

weswigham added 4 commits March 16, 2018 13:31

Add compiler option to enable declaration sourcemaps

ab7cb27

Transparent goto definition for sourcemapped declaration files

91cc19c

Post-rebase touchups

c291828

Rename API methods

2b26a27

weswigham requested review from rbuckton, RyanCavanaugh and mhegazy March 16, 2018 21:28

RyanCavanaugh reviewed Mar 16, 2018

View reviewed changes

RyanCavanaugh approved these changes Mar 16, 2018

View reviewed changes

weswigham added 2 commits March 16, 2018 15:07

Fix lints

ec44da5

Fix typo in name XD

ffb7f85

Log sourcemap decode errors

589ac28

weswigham added 2 commits March 16, 2018 16:38

Share the cache more, but also invalidate it more

5584a2d

Remove todo

238ba98

Enable mapping on go to implementation as well

ff158cf

weswigham added 5 commits March 19, 2018 12:48

Allow fourslash to test declaration maps mroe easily

b9f6149

more test

53f72de

Merge branch 'master' into decl-maps

29c732e

Handle sourceRoot

6070108

Add tests documenting current behavior with other sourcemapping flags

0a63553

rbuckton requested changes Mar 19, 2018

View reviewed changes

Ignore inline options for declaration file maps, simplify dispatch in…

d8480b2

… emitter

rbuckton requested changes Mar 23, 2018

View reviewed changes

rbuckton reviewed Mar 23, 2018

View reviewed changes

rbuckton reviewed Mar 24, 2018

View reviewed changes

weswigham added 2 commits March 23, 2018 17:05

Merge branch 'master' into decl-maps

f6e81f2

Even more feedback

d0a5e28

weswigham added 2 commits March 26, 2018 11:25

USE Mroe lenient combined regexp

943dc12

only match base64 characters

265cc1a

rbuckton approved these changes Mar 26, 2018

View reviewed changes

Fix nit

e34a6bd

weswigham merged commit 6af764c into master Mar 26, 2018

weswigham deleted the decl-maps branch March 26, 2018 19:16

ghost mentioned this pull request Mar 27, 2018

jake runtests-browser broken #22905

Closed

mhegazy mentioned this pull request Apr 5, 2018

Resolving multiple package.json "main" fields #21423

Open

This was referenced Apr 19, 2018

Enable Go to Implementation in Typescript Editors #6209

Closed

Support new flags (2.9) #23627

Closed

RyanCavanaugh mentioned this pull request May 2, 2018

Transparent go-to-def doesn't work in VS #23827

Closed

This was referenced May 7, 2018

[JavaScript/Node.js] Support for "Go to definition" of code located in "node_modules" #10808

Closed

Update schema for TS 2.9 flags SchemaStore/schemastore#451

Merged

michaelaird mentioned this pull request May 18, 2018

Request: Support Declaration maps ivogabe/gulp-typescript#574

Closed

kitsonk mentioned this pull request Jun 3, 2018

Incorrect assumption around host being defined in base64encode/base64decode #24638

Closed

mhegazy mentioned this pull request Jun 19, 2018

Support "medium-sized" projects #3469

Closed

aheber mentioned this pull request Apr 23, 2019

Support d.ts. type mapping so viewing reference goes back to original script aheber/sfdx-typegen#15

Open

jakebailey mentioned this pull request Mar 23, 2024

Remove sys.bufferFrom, duplicate Buffer declarations, etc #57916

Merged

Declaration maps and transparent goto definition using them #22658

Declaration maps and transparent goto definition using them #22658

Conversation

weswigham commented Mar 16, 2018 • edited Loading

weswigham commented Mar 16, 2018

RyanCavanaugh commented Mar 16, 2018

Choose a reason for hiding this comment

RyanCavanaugh left a comment

Choose a reason for hiding this comment

mjbvz commented Mar 16, 2018

weswigham commented Mar 16, 2018

mjbvz commented Mar 16, 2018

weswigham commented Mar 16, 2018

weswigham commented Mar 17, 2018

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Mar 24, 2018

Choose a reason for hiding this comment

michaelaird commented Jun 11, 2018

mhegazy commented Jun 11, 2018

michaelaird commented Jun 11, 2018

mhegazy commented Jun 11, 2018

HolgerJeromin commented Jul 18, 2018

weswigham commented Mar 16, 2018 •

edited

Loading