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.

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

Declaration maps and transparent goto definition using them #22658

Merged
merged 34 commits into from
Mar 26, 2018
Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Mar 16, 2018

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
Copy link
Member Author

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
Copy link
Member

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

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

Choose a reason for hiding this comment

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

Typo

Copy link
Member

@RyanCavanaugh RyanCavanaugh left a comment

Choose a reason for hiding this comment

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

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

@mjbvz
Copy link
Contributor

mjbvz commented Mar 16, 2018

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
Copy link
Member Author

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

@mjbvz
Copy link
Contributor

mjbvz commented Mar 16, 2018

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
Copy link
Member Author

@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
Copy link
Member Author

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

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

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

None,
File,
Inline,
DeclarationFile
Copy link
Member

Choose a reason for hiding this comment

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

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.

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

Should this also be string | undefined?

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

Choose a reason for hiding this comment

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

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

@@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

if (options.declarationMaps) {
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary nested if, I would just merge the conditions.

}

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

Choose a reason for hiding this comment

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

nit: createSourceFileLikeCache seems more appropriate.

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

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

I would recommend using .charCodeAt and CharacterCodes

return decodedMappings || (decodedMappings = calculateDecodedMappings());
}

function getReverseSortedMappings() {
Copy link
Member

Choose a reason for hiding this comment

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

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

};

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

Choose a reason for hiding this comment

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

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.

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

function getOriginalPosition(loc: SourceMappableLocation): SourceMappableLocation {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

return forwardSortedMappings || (forwardSortedMappings = getDecodedMappings().slice().sort(compareProcessedSpanEmittedPositions));
}

function calculateDecodedMappings(): ProcessedSourceMapSpan[] {
Copy link
Member

Choose a reason for hiding this comment

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

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.

name?: string;
}

interface RawSourceMapSpan {
Copy link
Member

Choose a reason for hiding this comment

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

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

return state.decodingIndex === state.encodedText.length;
}

function decodeSingleSpan<T>(state: DecoderState<T>): void {
Copy link
Member

Choose a reason for hiding this comment

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

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

A few more minor comments.

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

function setSourceFileDeclarationMaps(node: SourceFile) {
Copy link
Member

Choose a reason for hiding this comment

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

This function name isn't very clear.

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

Choose a reason for hiding this comment

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

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

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

const Buffer: typeof global.Buffer = require("buffer").Buffer;
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

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.

return output;
}

export function base64decode(host: { base64decode?(input: string): string }, input: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

You have your condition and incrementer swapped.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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,(.+)$)?/

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

@weswigham
Copy link
Member Author

@rbuckton 👍 👎?

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

Choose a reason for hiding this comment

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

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

@michaelaird
Copy link

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
Copy link
Contributor

mhegazy commented Jun 11, 2018

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

@michaelaird
Copy link

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
Copy link
Contributor

mhegazy commented Jun 11, 2018

mind filing a new ticket with some repro steps?

@HolgerJeromin
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants