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
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
ab7cb27
Add compiler option to enable declaration sourcemaps
weswigham Feb 15, 2018
91cc19c
Transparent goto definition for sourcemapped declaration files
weswigham Feb 17, 2018
c291828
Post-rebase touchups
weswigham Mar 16, 2018
2b26a27
Rename API methods
weswigham Mar 16, 2018
ec44da5
Fix lints
weswigham Mar 16, 2018
ffb7f85
Fix typo in name XD
weswigham Mar 16, 2018
589ac28
Log sourcemap decode errors
weswigham Mar 16, 2018
5584a2d
Share the cache more, but also invalidate it more
weswigham Mar 16, 2018
238ba98
Remove todo
weswigham Mar 16, 2018
ff158cf
Enable mapping on go to implementation as well
weswigham Mar 17, 2018
b9f6149
Allow fourslash to test declaration maps mroe easily
weswigham Mar 19, 2018
53f72de
more test
weswigham Mar 19, 2018
29c732e
Merge branch 'master' into decl-maps
weswigham Mar 19, 2018
6070108
Handle sourceRoot
weswigham Mar 19, 2018
0a63553
Add tests documenting current behavior with other sourcemapping flags
weswigham Mar 19, 2018
d8480b2
Ignore inline options for declaration file maps, simplify dispatch in…
weswigham Mar 20, 2018
ca88d5e
Change program diagnostic
weswigham Mar 20, 2018
2b231d7
Fix nit
weswigham Mar 20, 2018
fed6132
Use charCodeAt
weswigham Mar 20, 2018
509e4f3
Rename internal methods + veriables
weswigham Mar 20, 2018
0c44ba1
Avoid filter
weswigham Mar 20, 2018
c8620e3
span -> position
weswigham Mar 20, 2018
5687e10
Use character codes
weswigham Mar 20, 2018
e64e73d
Dont parse our sourcemap names until we need to start using them
weswigham Mar 20, 2018
47e701a
zero-index parsed positions
weswigham Mar 20, 2018
6a467ba
Handle sourceMappingURL comments, including base64 encoded ones
weswigham Mar 20, 2018
f2c8d3f
Unittest b64 decoder, make mroe robust to handle unicode properly
weswigham Mar 20, 2018
bd9cccf
Fix lint
weswigham Mar 20, 2018
c7a5296
declarationMaps -> declarationMap
weswigham Mar 20, 2018
f6e81f2
Merge branch 'master' into decl-maps
weswigham Mar 24, 2018
d0a5e28
Even more feedback
weswigham Mar 24, 2018
943dc12
USE Mroe lenient combined regexp
weswigham Mar 26, 2018
265cc1a
only match base64 characters
weswigham Mar 26, 2018
e34a6bd
Fix nit
weswigham Mar 26, 2018
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: 7 additions & 0 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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?

type: "boolean",
showInSimplifiedHelpView: true,
category: Diagnostics.Basic_Options,
description: Diagnostics.Generates_a_sourcemap_for_each_corresponding_d_ts_file,
},
{
name: "emitDeclarationOnly",
type: "boolean",
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2066,6 +2066,10 @@ namespace ts {
return moduleResolution;
}

export function getAreDeclarationMapsEnabled(options: CompilerOptions) {
return !!(options.declaration && options.declarationMaps);
}

export function getAllowSyntheticDefaultImports(compilerOptions: CompilerOptions) {
const moduleKind = getEmitModuleKind(compilerOptions);
return compilerOptions.allowSyntheticDefaultImports !== undefined
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -2768,6 +2768,10 @@
"code": 5068
},

"Generates a sourcemap for each corresponding '.d.ts' file.": {
"category": "Message",
"code": 6000
},
"Concatenate and emit output to single file.": {
"category": "Message",
"code": 6001
Expand Down
57 changes: 41 additions & 16 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,17 @@ namespace ts {
const jsFilePath = options.outFile || options.out;
const sourceMapFilePath = getSourceMapFilePath(jsFilePath, options);
const declarationFilePath = (forceDtsPaths || options.declaration) ? removeFileExtension(jsFilePath) + Extension.Dts : undefined;
return { jsFilePath, sourceMapFilePath, declarationFilePath };
const declarationMapPath = getAreDeclarationMapsEnabled(options) ? declarationFilePath + ".map" : undefined;
return { jsFilePath, sourceMapFilePath, declarationFilePath, declarationMapPath };
}
else {
const jsFilePath = getOwnEmitOutputFilePath(sourceFile, host, getOutputExtension(sourceFile, options));
const sourceMapFilePath = getSourceMapFilePath(jsFilePath, options);
// For legacy reasons (ie, we have baselines capturing the behavior), js files don't report a .d.ts output path - this would only matter if `declaration` and `allowJs` were both on, which is currently an error
const isJs = isSourceFileJavaScript(sourceFile);
const declarationFilePath = ((forceDtsPaths || options.declaration) && !isJs) ? getDeclarationEmitOutputFilePath(sourceFile, host) : undefined;
return { jsFilePath, sourceMapFilePath, declarationFilePath };
const declarationMapPath = getAreDeclarationMapsEnabled(options) ? declarationFilePath + ".map" : undefined;
return { jsFilePath, sourceMapFilePath, declarationFilePath, declarationMapPath };
}
}

Expand All @@ -83,12 +85,19 @@ namespace ts {
return Extension.Js;
}

const enum SourceMapEmitKind {
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.

}

/*@internal*/
// targetSourceFile is when users only want one file in entire project to be emitted. This is used in compileOnSave feature
export function emitFiles(resolver: EmitResolver, host: EmitHost, targetSourceFile: SourceFile, emitOnlyDtsFiles?: boolean, transformers?: TransformerFactory<SourceFile>[]): EmitResult {
const compilerOptions = host.getCompilerOptions();
const moduleKind = getEmitModuleKind(compilerOptions);
const sourceMapDataList: SourceMapData[] = compilerOptions.sourceMap || compilerOptions.inlineSourceMap ? [] : undefined;
const sourceMapDataList: SourceMapData[] = (compilerOptions.sourceMap || compilerOptions.inlineSourceMap || getAreDeclarationMapsEnabled(compilerOptions)) ? [] : undefined;
const emittedFilesList: string[] = compilerOptions.listEmittedFiles ? [] : undefined;
const emitterDiagnostics = createDiagnosticCollection();
const newLine = host.getNewLine();
Expand All @@ -113,9 +122,9 @@ namespace ts {
sourceMaps: sourceMapDataList
};

function emitSourceFileOrBundle({ jsFilePath, sourceMapFilePath, declarationFilePath }: EmitFileNames, sourceFileOrBundle: SourceFile | Bundle) {
function emitSourceFileOrBundle({ jsFilePath, sourceMapFilePath, declarationFilePath, declarationMapPath }: EmitFileNames, sourceFileOrBundle: SourceFile | Bundle) {
emitJsFileOrBundle(sourceFileOrBundle, jsFilePath, sourceMapFilePath);
emitDeclarationFileOrBundle(sourceFileOrBundle, declarationFilePath);
emitDeclarationFileOrBundle(sourceFileOrBundle, declarationFilePath, declarationMapPath);

if (!emitSkipped && emittedFilesList) {
if (!emitOnlyDtsFiles) {
Expand Down Expand Up @@ -162,13 +171,14 @@ namespace ts {
onSetSourceFile: setSourceFile,
});

printSourceFileOrBundle(jsFilePath, sourceMapFilePath, isSourceFile(sourceFileOrBundle) ? transform.transformed[0] : createBundle(transform.transformed), printer);
const sourcemapKind = compilerOptions.inlineSourceMap ? SourceMapEmitKind.Inline : compilerOptions.sourceMap ? SourceMapEmitKind.File : SourceMapEmitKind.None;
printSourceFileOrBundle(jsFilePath, sourceMapFilePath, isSourceFile(sourceFileOrBundle) ? transform.transformed[0] : createBundle(transform.transformed), printer, sourcemapKind);

// Clean up emit nodes on parse tree
transform.dispose();
}

function emitDeclarationFileOrBundle(sourceFileOrBundle: SourceFile | Bundle, declarationFilePath: string | undefined) {
function emitDeclarationFileOrBundle(sourceFileOrBundle: SourceFile | Bundle, declarationFilePath: string | undefined, declarationMapPath: string | undefined) {
if (!(declarationFilePath && !isInJavaScriptFile(sourceFileOrBundle))) {
return;
}
Expand All @@ -186,25 +196,34 @@ namespace ts {
// resolver hooks
hasGlobalName: resolver.hasGlobalName,

// sourcemap hooks
onEmitSourceMapOfNode: sourceMap.emitNodeWithSourceMap,
onEmitSourceMapOfToken: sourceMap.emitTokenWithSourceMap,
onEmitSourceMapOfPosition: sourceMap.emitPos,
onSetSourceFile: setSourceFile,

// transform hooks
onEmitNode: declarationTransform.emitNodeWithNotification,
substituteNode: declarationTransform.substituteNode,
});
const declBlocked = (!!declarationTransform.diagnostics && !!declarationTransform.diagnostics.length) || !!host.isEmitBlocked(declarationFilePath) || !!compilerOptions.noEmit;
emitSkipped = emitSkipped || declBlocked;
if (!declBlocked || emitOnlyDtsFiles) {
const previousState = sourceMap.setState(/*disabled*/ true);
printSourceFileOrBundle(declarationFilePath, /*sourceMapFilePath*/ undefined, declarationTransform.transformed[0], declarationPrinter, /*shouldSkipSourcemap*/ true);
sourceMap.setState(previousState);
printSourceFileOrBundle(declarationFilePath, declarationMapPath, declarationTransform.transformed[0], declarationPrinter, getAreDeclarationMapsEnabled(compilerOptions) ? SourceMapEmitKind.DeclarationFile : SourceMapEmitKind.None);
}
declarationTransform.dispose();
}

function printSourceFileOrBundle(jsFilePath: string, sourceMapFilePath: string, sourceFileOrBundle: SourceFile | Bundle, printer: Printer, shouldSkipSourcemap = false) {
function printSourceFileOrBundle(jsFilePath: string, sourceMapFilePath: string | undefined, sourceFileOrBundle: SourceFile | Bundle, printer: Printer, sourcemapKind: SourceMapEmitKind) {
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.

sourceMap.initialize(jsFilePath, sourceMapFilePath, sourceFileOrBundle);
}
else {
sourceMap.setState(/*disabled*/ true);
}

if (bundle) {
bundledHelpers = createMap<boolean>();
Expand All @@ -219,25 +238,31 @@ namespace ts {
writer.writeLine();

const sourceMappingURL = sourceMap.getSourceMappingURL();
if (!shouldSkipSourcemap && sourceMappingURL) {
if (sourceMappingURL) {
writer.write(`//# ${"sourceMappingURL"}=${sourceMappingURL}`); // Sometimes tools can sometimes see this line as a source mapping url comment
}

// Write the source map
if (!shouldSkipSourcemap && compilerOptions.sourceMap && !compilerOptions.inlineSourceMap) {
if (sourceMapFilePath && (sourcemapKind === SourceMapEmitKind.File || sourcemapKind === SourceMapEmitKind.DeclarationFile)) {
writeFile(host, emitterDiagnostics, sourceMapFilePath, sourceMap.getText(), /*writeByteOrderMark*/ false, sourceFiles);
}

// Record source map data for the test harness.
if (!shouldSkipSourcemap && sourceMapDataList) {
if (sourcemapKind !== SourceMapEmitKind.None) {
sourceMapDataList.push(sourceMap.getSourceMapData());
}

// Write the output file
writeFile(host, emitterDiagnostics, jsFilePath, writer.getText(), compilerOptions.emitBOM, sourceFiles);

// Reset state
sourceMap.reset();

if (sourcemapKind !== SourceMapEmitKind.None) {
sourceMap.reset();
}
else {
sourceMap.setState(/*disabled*/ false);
}
writer.clear();

currentSourceFile = undefined;
Expand Down
8 changes: 7 additions & 1 deletion src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2122,7 +2122,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".

// Error to specify --mapRoot without --sourcemap
createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "mapRoot", "sourceMap");
}
Expand All @@ -2136,6 +2136,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.

if (!options.declaration) {
createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "declarationMaps", "declaration");
}
}

if (options.lib && options.noLib) {
createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_with_option_1, "lib", "noLib");
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ namespace ts {
return result;
}

export function getPositionOfLineAndCharacter(sourceFile: SourceFile, line: number, character: number): number {
export function getPositionOfLineAndCharacter(sourceFile: SourceFileLike, line: number, character: number): number {
return computePositionOfLineAndCharacter(getLineStarts(sourceFile), line, character, sourceFile.text);
}

Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4085,6 +4085,7 @@ namespace ts {
/** configFile is set as non enumerable property so as to avoid checking of json source files */
/* @internal */ readonly configFile?: JsonSourceFile;
declaration?: boolean;
declarationMaps?: boolean;
emitDeclarationOnly?: boolean;
declarationDir?: string;
/* @internal */ diagnostics?: boolean;
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2807,7 +2807,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?

declarationFilePath: string;
declarationFilePath: string | undefined;
declarationMapPath: string | undefined;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/harness/compilerRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ class CompilerBaselineRunner extends RunnerBase {

// Source maps?
it("Correct sourcemap content for " + fileName, () => {
if (options.sourceMap || options.inlineSourceMap) {
if (options.sourceMap || options.inlineSourceMap || options.declarationMaps) {
Harness.Baseline.runBaseline(justName.replace(/\.tsx?$/, ".sourcemap.txt"), () => {
const record = result.getSourceMapRecord();
if ((options.noEmitOnError && result.errors.length !== 0) || record === undefined) {
Expand Down
15 changes: 10 additions & 5 deletions src/harness/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1632,14 +1632,15 @@ namespace Harness {
}

export function doSourcemapBaseline(baselinePath: string, options: ts.CompilerOptions, result: CompilerResult, harnessSettings: TestCaseParser.CompilerSettings) {
const declMaps = ts.getAreDeclarationMapsEnabled(options);
if (options.inlineSourceMap) {
if (result.sourceMaps.length > 0) {
if (result.sourceMaps.length > 0 && !declMaps) {
throw new Error("No sourcemap files should be generated if inlineSourceMaps was set.");
}
return;
}
else if (options.sourceMap) {
if (result.sourceMaps.length !== result.files.length) {
else if (options.sourceMap || declMaps) {
if (result.sourceMaps.length !== (result.files.length * (declMaps && options.sourceMap ? 2 : 1))) {
throw new Error("Number of sourcemap files should be same as js files.");
}

Expand Down Expand Up @@ -1806,6 +1807,10 @@ namespace Harness {
return ts.endsWith(fileName, ".js.map") || ts.endsWith(fileName, ".jsx.map");
}

export function isDTSMap(fileName: string) {
return ts.endsWith(fileName, ".d.ts.map");
}

/** Contains the code and errors of a compilation and some helper methods to check its status. */
export class CompilerResult {
public files: GeneratedFile[] = [];
Expand All @@ -1826,7 +1831,7 @@ namespace Harness {
// .js file, add to files
this.files.push(emittedFile);
}
else if (isJSMap(emittedFile.fileName)) {
else if (isJSMap(emittedFile.fileName) || isDTSMap(emittedFile.fileName)) {
this.sourceMaps.push(emittedFile);
}
else {
Expand All @@ -1839,7 +1844,7 @@ namespace Harness {

public getSourceMapRecord() {
if (this.sourceMapData && this.sourceMapData.length > 0) {
return SourceMapRecorder.getSourceMapRecord(this.sourceMapData, this.program, this.files);
return SourceMapRecorder.getSourceMapRecord(this.sourceMapData, this.program, this.files, this.declFilesCode);
}
}
}
Expand Down
21 changes: 19 additions & 2 deletions src/harness/sourceMapRecorder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,14 +434,31 @@ namespace Harness.SourceMapRecorder {
}
}

export function getSourceMapRecord(sourceMapDataList: ts.SourceMapData[], program: ts.Program, jsFiles: Compiler.GeneratedFile[]) {
export function getSourceMapRecord(sourceMapDataList: ts.SourceMapData[], program: ts.Program, jsFiles: Compiler.GeneratedFile[], declarationFiles: Compiler.GeneratedFile[]) {
const sourceMapRecorder = new Compiler.WriterAggregator();

for (let i = 0; i < sourceMapDataList.length; i++) {
const sourceMapData = sourceMapDataList[i];
let prevSourceFile: ts.SourceFile;
let currentFile: Compiler.GeneratedFile;
if (ts.endsWith(sourceMapData.sourceMapFile, ts.Extension.Dts)) {
if (sourceMapDataList.length > jsFiles.length) {
currentFile = declarationFiles[Math.floor(i / 2)]; // When both kinds of source map are present, they alternate js/dts
}
else {
currentFile = declarationFiles[i];
}
}
else {
if (sourceMapDataList.length > jsFiles.length) {
currentFile = jsFiles[Math.floor(i / 2)];
}
else {
currentFile = jsFiles[i];
}
}

SourceMapSpanWriter.initializeSourceMapSpanWriter(sourceMapRecorder, sourceMapData, jsFiles[i]);
SourceMapSpanWriter.initializeSourceMapSpanWriter(sourceMapRecorder, sourceMapData, currentFile);
for (const decodedSourceMapping of sourceMapData.sourceMapDecodedMappings) {
const currentSourceFile = program.getSourceFile(sourceMapData.inputSourceFileNames[decodedSourceMapping.sourceIndex]);
if (currentSourceFile !== prevSourceFile) {
Expand Down
Loading