Skip to content

Add type for diagnostics where location is defined #23686

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

Merged
5 commits merged into from
May 22, 2018
Merged

Add type for diagnostics where location is defined #23686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0301fed
Add type for diagnostics where location is defined
Apr 25, 2018
7ade816
getSemanticDiagnostics may return global diagnostics
May 18, 2018
919cfc0
Merge branch 'master' into diagnosticWithLocation
May 18, 2018
b372285
Reduce array creation
May 18, 2018
919cb05
Merge branch 'master' into diagnosticWithLocation
May 22, 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
2 changes: 1 addition & 1 deletion src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ namespace ts {
* If so, the node _must_ be in the current file (as that's the only way anything could have traversed to it to yield it as the error node)
* This version of `createDiagnosticForNode` uses the binder's context to account for this, and always yields correct diagnostics even in these situations.
*/
function createDiagnosticForNode(node: Node, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): Diagnostic {
function createDiagnosticForNode(node: Node, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): DiagnosticWithLocation {
return createDiagnosticForNodeInSourceFile(getSourceFileOfNode(node) || file, node, message, arg0, arg1, arg2);
}

Expand Down
10 changes: 5 additions & 5 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,11 +313,11 @@ namespace ts {

getSuggestionDiagnostics: file => {
return (suggestionDiagnostics.get(file.fileName) || emptyArray).concat(getUnusedDiagnostics());
function getUnusedDiagnostics(): ReadonlyArray<Diagnostic> {
function getUnusedDiagnostics(): ReadonlyArray<DiagnosticWithLocation> {
if (file.isDeclarationFile) return emptyArray;

checkSourceFile(file);
const diagnostics: Diagnostic[] = [];
const diagnostics: DiagnosticWithLocation[] = [];
Debug.assert(!!(getNodeLinks(file).flags & NodeCheckFlags.TypeChecked));
checkUnusedIdentifiers(getPotentiallyUnusedIdentifiers(file), (kind, diag) => {
if (!unusedIsError(kind)) {
Expand Down Expand Up @@ -481,7 +481,7 @@ namespace ts {

const diagnostics = createDiagnosticCollection();
// Suggestion diagnostics must have a file. Keyed by source file name.
const suggestionDiagnostics = createMultiMap<Diagnostic>();
const suggestionDiagnostics = createMultiMap<DiagnosticWithLocation>();

const enum TypeFacts {
None = 0,
Expand Down Expand Up @@ -628,7 +628,7 @@ namespace ts {
Local,
Parameter,
}
type AddUnusedDiagnostic = (type: UnusedKind, diagnostic: Diagnostic) => void;
type AddUnusedDiagnostic = (type: UnusedKind, diagnostic: DiagnosticWithLocation) => void;

const builtinGlobals = createSymbolTable();
builtinGlobals.set(undefinedSymbol.escapedName, undefinedSymbol);
Expand Down Expand Up @@ -824,7 +824,7 @@ namespace ts {
diagnostics.add(diagnostic);
}

function addErrorOrSuggestion(isError: boolean, diagnostic: Diagnostic) {
function addErrorOrSuggestion(isError: boolean, diagnostic: DiagnosticWithLocation) {
if (isError) {
diagnostics.add(diagnostic);
}
Expand Down
8 changes: 4 additions & 4 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ namespace ts {
return pathIsRelative(moduleName) || isRootedDiskPath(moduleName);
}

export function sortAndDeduplicateDiagnostics(diagnostics: ReadonlyArray<Diagnostic>): Diagnostic[] {
return sortAndDeduplicate(diagnostics, compareDiagnostics);
export function sortAndDeduplicateDiagnostics<T extends Diagnostic>(diagnostics: ReadonlyArray<T>): T[] {
return sortAndDeduplicate<T>(diagnostics, compareDiagnostics);
}
}

Expand Down Expand Up @@ -1619,8 +1619,8 @@ namespace ts {
return localizedDiagnosticMessages && localizedDiagnosticMessages[message.key] || message.message;
}

export function createFileDiagnostic(file: SourceFile, start: number, length: number, message: DiagnosticMessage, ...args: (string | number)[]): Diagnostic;
export function createFileDiagnostic(file: SourceFile, start: number, length: number, message: DiagnosticMessage): Diagnostic {
export function createFileDiagnostic(file: SourceFile, start: number, length: number, message: DiagnosticMessage, ...args: (string | number)[]): DiagnosticWithLocation;
export function createFileDiagnostic(file: SourceFile, start: number, length: number, message: DiagnosticMessage): DiagnosticWithLocation {
Debug.assertGreaterThanOrEqual(start, 0);
Debug.assertGreaterThanOrEqual(length, 0);

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ namespace ts {
// tslint:enable variable-name

let sourceFile: SourceFile;
let parseDiagnostics: Diagnostic[];
let parseDiagnostics: DiagnosticWithLocation[];
let syntaxCursor: IncrementalParser.SyntaxCursor;

let currentToken: SyntaxKind;
Expand Down
64 changes: 36 additions & 28 deletions src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,8 +394,8 @@ namespace ts {
return resolutions;
}

interface DiagnosticCache {
perFile?: Map<Diagnostic[]>;
interface DiagnosticCache<T extends Diagnostic> {
perFile?: Map<T[]>;
allDiagnostics?: Diagnostic[];
}

Expand Down Expand Up @@ -454,7 +454,7 @@ namespace ts {

export function getConfigFileParsingDiagnostics(configFileParseResult: ParsedCommandLine): ReadonlyArray<Diagnostic> {
return configFileParseResult.options.configFile ?
configFileParseResult.options.configFile.parseDiagnostics.concat(configFileParseResult.errors) :
[...configFileParseResult.options.configFile.parseDiagnostics, ...configFileParseResult.errors] :
configFileParseResult.errors;
}

Expand Down Expand Up @@ -517,8 +517,8 @@ namespace ts {
let classifiableNames: UnderscoreEscapedMap<true>;
let modifiedFilePaths: Path[] | undefined;

const cachedSemanticDiagnosticsForFile: DiagnosticCache = {};
const cachedDeclarationDiagnosticsForFile: DiagnosticCache = {};
const cachedSemanticDiagnosticsForFile: DiagnosticCache<Diagnostic> = {};
const cachedDeclarationDiagnosticsForFile: DiagnosticCache<DiagnosticWithLocation> = {};

let resolvedTypeReferenceDirectives = createMap<ResolvedTypeReferenceDirective>();
let fileProcessingDiagnostics = createDiagnosticCollection();
Expand Down Expand Up @@ -1313,10 +1313,10 @@ namespace ts {
return filesByName.get(path);
}

function getDiagnosticsHelper(
function getDiagnosticsHelper<T extends Diagnostic>(
sourceFile: SourceFile,
getDiagnostics: (sourceFile: SourceFile, cancellationToken: CancellationToken) => ReadonlyArray<Diagnostic>,
cancellationToken: CancellationToken): ReadonlyArray<Diagnostic> {
getDiagnostics: (sourceFile: SourceFile, cancellationToken: CancellationToken) => ReadonlyArray<T>,
cancellationToken: CancellationToken): ReadonlyArray<T> {
if (sourceFile) {
return getDiagnostics(sourceFile, cancellationToken);
}
Expand All @@ -1328,15 +1328,15 @@ namespace ts {
}));
}

function getSyntacticDiagnostics(sourceFile: SourceFile, cancellationToken: CancellationToken): ReadonlyArray<Diagnostic> {
function getSyntacticDiagnostics(sourceFile: SourceFile, cancellationToken: CancellationToken): ReadonlyArray<DiagnosticWithLocation> {
return getDiagnosticsHelper(sourceFile, getSyntacticDiagnosticsForFile, cancellationToken);
}

function getSemanticDiagnostics(sourceFile: SourceFile, cancellationToken: CancellationToken): ReadonlyArray<Diagnostic> {
return getDiagnosticsHelper(sourceFile, getSemanticDiagnosticsForFile, cancellationToken);
}

function getDeclarationDiagnostics(sourceFile: SourceFile, cancellationToken: CancellationToken): ReadonlyArray<Diagnostic> {
function getDeclarationDiagnostics(sourceFile: SourceFile, cancellationToken: CancellationToken): ReadonlyArray<DiagnosticWithLocation> {
const options = program.getCompilerOptions();
// collect diagnostics from the program only once if either no source file was specified or out/outFile is set (bundled emit)
if (!sourceFile || options.out || options.outFile) {
Expand All @@ -1347,7 +1347,7 @@ namespace ts {
}
}

function getSyntacticDiagnosticsForFile(sourceFile: SourceFile): ReadonlyArray<Diagnostic> {
function getSyntacticDiagnosticsForFile(sourceFile: SourceFile): ReadonlyArray<DiagnosticWithLocation> {
// For JavaScript files, we report semantic errors for using TypeScript-only
// constructs from within a JavaScript file as syntactic errors.
if (isSourceFileJavaScript(sourceFile)) {
Expand Down Expand Up @@ -1382,7 +1382,7 @@ namespace ts {
}
}

function getSemanticDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] {
function getSemanticDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): ReadonlyArray<Diagnostic> {
return getAndCacheDiagnostics(sourceFile, cancellationToken, cachedSemanticDiagnosticsForFile, getSemanticDiagnosticsForFileNoCache);
}

Expand All @@ -1403,15 +1403,22 @@ namespace ts {
// By default, only type-check .ts, .tsx, 'Deferred' and 'External' files (external files are added by plugins)
const includeBindAndCheckDiagnostics = sourceFile.scriptKind === ScriptKind.TS || sourceFile.scriptKind === ScriptKind.TSX ||
sourceFile.scriptKind === ScriptKind.External || isCheckJs || sourceFile.scriptKind === ScriptKind.Deferred;
const bindDiagnostics = includeBindAndCheckDiagnostics ? sourceFile.bindDiagnostics : emptyArray;
const bindDiagnostics: ReadonlyArray<Diagnostic> = includeBindAndCheckDiagnostics ? sourceFile.bindDiagnostics : emptyArray;
Copy link
Member

Choose a reason for hiding this comment

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

Can you instead use concatenate on the diagnostics instead of using empty array assignment to sub arrays to be concatenated, that way multiple emptyArray concatenate wont create new array with length 0 but use empty array in the end?

Copy link
Author

@ghost ghost May 18, 2018 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

emptyArray is a const and not a function call -- it doesn't create a new empty array, it just refers to a global empty array. So shouldn't be any issue of too many allocations.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/Microsoft/TypeScript/pull/23686/files#diff-08a3cc4f1f9a51dbb468c2810f5229d3R1410 will allocate new array.

const checkDiagnostics = includeBindAndCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) : emptyArray;
const fileProcessingDiagnosticsInFile = fileProcessingDiagnostics.getDiagnostics(sourceFile.fileName);
const programDiagnosticsInFile = programDiagnostics.getDiagnostics(sourceFile.fileName);
let diagnostics = bindDiagnostics.concat(checkDiagnostics, fileProcessingDiagnosticsInFile, programDiagnosticsInFile);
if (isCheckJs) {
diagnostics = concatenate(diagnostics, sourceFile.jsDocDiagnostics);

let diagnostics: Diagnostic[] | undefined;
for (const diags of [bindDiagnostics, checkDiagnostics, fileProcessingDiagnosticsInFile, programDiagnosticsInFile, isCheckJs ? sourceFile.jsDocDiagnostics : undefined]) {
if (diags) {
for (const diag of diags) {
if (shouldReportDiagnostic(diag)) {
diagnostics = append(diagnostics, diag);
}
}
}
}
return filter(diagnostics, shouldReportDiagnostic);
return diagnostics;
});
}

Expand Down Expand Up @@ -1440,9 +1447,9 @@ namespace ts {
return true;
}

function getJavaScriptSyntacticDiagnosticsForFile(sourceFile: SourceFile): Diagnostic[] {
function getJavaScriptSyntacticDiagnosticsForFile(sourceFile: SourceFile): DiagnosticWithLocation[] {
return runWithCancellationToken(() => {
const diagnostics: Diagnostic[] = [];
const diagnostics: DiagnosticWithLocation[] = [];
let parent: Node = sourceFile;
walk(sourceFile);

Expand Down Expand Up @@ -1610,20 +1617,20 @@ namespace ts {
}
}

function createDiagnosticForNodeArray(nodes: NodeArray<Node>, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): Diagnostic {
function createDiagnosticForNodeArray(nodes: NodeArray<Node>, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): DiagnosticWithLocation {
const start = nodes.pos;
return createFileDiagnostic(sourceFile, start, nodes.end - start, message, arg0, arg1, arg2);
}

// Since these are syntactic diagnostics, parent might not have been set
// this means the sourceFile cannot be infered from the node
function createDiagnosticForNode(node: Node, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): Diagnostic {
function createDiagnosticForNode(node: Node, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): DiagnosticWithLocation {
return createDiagnosticForNodeInSourceFile(sourceFile, node, message, arg0, arg1, arg2);
}
});
}

function getDeclarationDiagnosticsWorker(sourceFile: SourceFile | undefined, cancellationToken: CancellationToken): Diagnostic[] {
function getDeclarationDiagnosticsWorker(sourceFile: SourceFile, cancellationToken: CancellationToken): ReadonlyArray<DiagnosticWithLocation> {
return getAndCacheDiagnostics(sourceFile, cancellationToken, cachedDeclarationDiagnosticsForFile, getDeclarationDiagnosticsForFileNoCache);
}

Expand All @@ -1635,23 +1642,24 @@ namespace ts {
});
}

function getAndCacheDiagnostics(
function getAndCacheDiagnostics<T extends Diagnostic>(
sourceFile: SourceFile | undefined,
cancellationToken: CancellationToken,
cache: DiagnosticCache,
getDiagnostics: (sourceFile: SourceFile, cancellationToken: CancellationToken) => Diagnostic[]) {
cache: DiagnosticCache<T>,
getDiagnostics: (sourceFile: SourceFile, cancellationToken: CancellationToken) => T[],
): ReadonlyArray<T> {

const cachedResult = sourceFile
? cache.perFile && cache.perFile.get(sourceFile.path)
: cache.allDiagnostics;
: cache.allDiagnostics as T[];

if (cachedResult) {
return cachedResult;
}
const result = getDiagnostics(sourceFile, cancellationToken) || emptyArray;
if (sourceFile) {
if (!cache.perFile) {
cache.perFile = createMap<Diagnostic[]>();
cache.perFile = createMap<T[]>();
}
cache.perFile.set(sourceFile.path, result);
}
Expand All @@ -1661,7 +1669,7 @@ namespace ts {
return result;
}

function getDeclarationDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] {
function getDeclarationDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): ReadonlyArray<DiagnosticWithLocation> {
return sourceFile.isDeclarationFile ? [] : getDeclarationDiagnosticsWorker(sourceFile, cancellationToken);
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/transformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ namespace ts {
let onSubstituteNode: TransformationContext["onSubstituteNode"] = (_, node) => node;
let onEmitNode: TransformationContext["onEmitNode"] = (hint, node, callback) => callback(hint, node);
let state = TransformationState.Uninitialized;
const diagnostics: Diagnostic[] = [];
const diagnostics: DiagnosticWithLocation[] = [];

// The transformation context is provided to each transformer as part of transformer
// initialization.
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/transformers/declarations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*@internal*/
namespace ts {
export function getDeclarationDiagnostics(host: EmitHost, resolver: EmitResolver, file: SourceFile | undefined): Diagnostic[] {
export function getDeclarationDiagnostics(host: EmitHost, resolver: EmitResolver, file: SourceFile | undefined): DiagnosticWithLocation[] {
if (file && isSourceFileJavaScript(file)) {
return []; // No declaration diagnostics for js for now
}
Expand Down
Loading