Skip to content

Commit b784a42

Browse files
CyrusNajmabadiCyrusNajmabadi
committed
Merge pull request #2205 from Microsoft/errorSpans
Never use the entire span of a function declaration or function expression when reporting a checker error.
2 parents 1c64260 + d367c96 commit b784a42

File tree

63 files changed

+344
-395
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

63 files changed

+344
-395
lines changed

‎src/compiler/checker.ts

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8605,7 +8605,7 @@ module ts {
86058605
// since LHS will be block scoped name instead of function scoped
86068606
if (!namesShareScope) {
86078607
var name = symbolToString(localDeclarationSymbol);
8608-
error(getErrorSpanForNode(node), Diagnostics.Cannot_initialize_outer_scoped_variable_0_in_the_same_scope_as_block_scoped_declaration_1, name, name);
8608+
error(node, Diagnostics.Cannot_initialize_outer_scoped_variable_0_in_the_same_scope_as_block_scoped_declaration_1, name, name);
86098609
}
86108610
}
86118611
}
@@ -11817,19 +11817,11 @@ module ts {
1181711817
return sourceFile.parseDiagnostics.length > 0;
1181811818
}
1181911819

11820-
function scanToken(scanner: Scanner, pos: number) {
11821-
scanner.setTextPos(pos);
11822-
scanner.scan();
11823-
var start = scanner.getTokenPos();
11824-
return start;
11825-
}
11826-
1182711820
function grammarErrorOnFirstToken(node: Node, message: DiagnosticMessage, arg0?: any, arg1?: any, arg2?: any): boolean {
1182811821
var sourceFile = getSourceFileOfNode(node);
1182911822
if (!hasParseDiagnostics(sourceFile)) {
11830-
var scanner = createScanner(languageVersion, /*skipTrivia*/ true, sourceFile.text);
11831-
var start = scanToken(scanner, node.pos);
11832-
diagnostics.add(createFileDiagnostic(sourceFile, start, scanner.getTextPos() - start, message, arg0, arg1, arg2));
11823+
var span = getSpanOfTokenAtPosition(sourceFile, node.pos);
11824+
diagnostics.add(createFileDiagnostic(sourceFile, span.start, span.length, message, arg0, arg1, arg2));
1183311825
return true;
1183411826
}
1183511827
}
@@ -11844,9 +11836,7 @@ module ts {
1184411836
function grammarErrorOnNode(node: Node, message: DiagnosticMessage, arg0?: any, arg1?: any, arg2?: any): boolean {
1184511837
var sourceFile = getSourceFileOfNode(node);
1184611838
if (!hasParseDiagnostics(sourceFile)) {
11847-
var span = getErrorSpanForNode(node);
11848-
var start = span.end > span.pos ? skipTrivia(sourceFile.text, span.pos) : span.pos;
11849-
diagnostics.add(createFileDiagnostic(sourceFile, start, span.end - start, message, arg0, arg1, arg2));
11839+
diagnostics.add(createDiagnosticForNode(node, message, arg0, arg1, arg2));
1185011840
return true;
1185111841
}
1185211842
}
@@ -11983,9 +11973,8 @@ module ts {
1198311973
function grammarErrorAfterFirstToken(node: Node, message: DiagnosticMessage, arg0?: any, arg1?: any, arg2?: any): boolean {
1198411974
var sourceFile = getSourceFileOfNode(node);
1198511975
if (!hasParseDiagnostics(sourceFile)) {
11986-
var scanner = createScanner(languageVersion, /*skipTrivia*/ true, sourceFile.text);
11987-
scanToken(scanner, node.pos);
11988-
diagnostics.add(createFileDiagnostic(sourceFile, scanner.getTextPos(), 0, message, arg0, arg1, arg2));
11976+
var span = getSpanOfTokenAtPosition(sourceFile, node.pos);
11977+
diagnostics.add(createFileDiagnostic(sourceFile, textSpanEnd(span), /*length*/ 0, message, arg0, arg1, arg2));
1198911978
return true;
1199011979
}
1199111980
}

‎src/compiler/program.ts

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -423,21 +423,19 @@ module ts {
423423
return;
424424
}
425425

426-
var firstExternalModule = forEach(files, f => isExternalModule(f) ? f : undefined);
427-
if (firstExternalModule && !options.module) {
428-
// We cannot use createDiagnosticFromNode because nodes do not have parents yet
429-
var externalModuleErrorSpan = getErrorSpanForNode(firstExternalModule.externalModuleIndicator);
430-
var errorStart = skipTrivia(firstExternalModule.text, externalModuleErrorSpan.pos);
431-
var errorLength = externalModuleErrorSpan.end - errorStart;
432-
diagnostics.add(createFileDiagnostic(firstExternalModule, errorStart, errorLength, Diagnostics.Cannot_compile_external_modules_unless_the_module_flag_is_provided));
426+
var firstExternalModuleSourceFile = forEach(files, f => isExternalModule(f) ? f : undefined);
427+
if (firstExternalModuleSourceFile && !options.module) {
428+
// We cannot use createDiagnosticFromNode because nodes do not have parents yet
429+
var span = getErrorSpanForNode(firstExternalModuleSourceFile, firstExternalModuleSourceFile.externalModuleIndicator);
430+
diagnostics.add(createFileDiagnostic(firstExternalModuleSourceFile, span.start, span.length, Diagnostics.Cannot_compile_external_modules_unless_the_module_flag_is_provided));
433431
}
434432

435433
// there has to be common source directory if user specified --outdir || --sourcRoot
436434
// if user specified --mapRoot, there needs to be common source directory if there would be multiple files being emitted
437435
if (options.outDir || // there is --outDir specified
438436
options.sourceRoot || // there is --sourceRoot specified
439437
(options.mapRoot && // there is --mapRoot Specified and there would be multiple js files generated
440-
(!options.out || firstExternalModule !== undefined))) {
438+
(!options.out || firstExternalModuleSourceFile !== undefined))) {
441439

442440
var commonPathComponents: string[];
443441
forEach(files, sourceFile => {

‎src/compiler/utilities.ts

Lines changed: 33 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -218,32 +218,35 @@ module ts {
218218
}
219219

220220
export function createDiagnosticForNode(node: Node, message: DiagnosticMessage, arg0?: any, arg1?: any, arg2?: any): Diagnostic {
221-
node = getErrorSpanForNode(node);
222-
var file = getSourceFileOfNode(node);
223-
224-
var start = getTokenPosOfNode(node, file);
225-
var length = node.end - start;
226-
227-
return createFileDiagnostic(file, start, length, message, arg0, arg1, arg2);
221+
var sourceFile = getSourceFileOfNode(node);
222+
var span = getErrorSpanForNode(sourceFile, node);
223+
return createFileDiagnostic(sourceFile, span.start, span.length, message, arg0, arg1, arg2);
228224
}
229225

230226
export function createDiagnosticForNodeFromMessageChain(node: Node, messageChain: DiagnosticMessageChain): Diagnostic {
231-
node = getErrorSpanForNode(node);
232-
var file = getSourceFileOfNode(node);
233-
var start = skipTrivia(file.text, node.pos);
234-
var length = node.end - start;
227+
var sourceFile = getSourceFileOfNode(node);
228+
var span = getErrorSpanForNode(sourceFile, node);
235229
return {
236-
file,
237-
start,
238-
length,
230+
file: sourceFile,
231+
start: span.start,
232+
length: span.length,
239233
code: messageChain.code,
240234
category: messageChain.category,
241235
messageText: messageChain.next ? messageChain : messageChain.messageText
242236
};
243237
}
244238

245-
export function getErrorSpanForNode(node: Node): Node {
246-
var errorSpan: Node;
239+
/* @internal */
240+
export function getSpanOfTokenAtPosition(sourceFile: SourceFile, pos: number): TextSpan {
241+
var scanner = createScanner(sourceFile.languageVersion, /*skipTrivia*/ true, sourceFile.text);
242+
scanner.setTextPos(pos);
243+
scanner.scan();
244+
var start = scanner.getTokenPos();
245+
return createTextSpanFromBounds(start, scanner.getTextPos());
246+
}
247+
248+
export function getErrorSpanForNode(sourceFile: SourceFile, node: Node): TextSpan {
249+
var errorNode = node;
247250
switch (node.kind) {
248251
// This list is a work in progress. Add missing node kinds to improve their error
249252
// spans.
@@ -254,16 +257,23 @@ module ts {
254257
case SyntaxKind.ModuleDeclaration:
255258
case SyntaxKind.EnumDeclaration:
256259
case SyntaxKind.EnumMember:
257-
errorSpan = (<Declaration>node).name;
260+
case SyntaxKind.FunctionDeclaration:
261+
case SyntaxKind.FunctionExpression:
262+
errorNode = (<Declaration>node).name;
258263
break;
259264
}
265+
266+
if (errorNode === undefined) {
267+
// If we don't have a better node, then just set the error on the first token of
268+
// construct.
269+
return getSpanOfTokenAtPosition(sourceFile, node.pos);
270+
}
271+
272+
var pos = nodeIsMissing(errorNode)
273+
? errorNode.pos
274+
: skipTrivia(sourceFile.text, errorNode.pos);
260275

261-
// We now have the ideal error span, but it may be a node that is optional and absent
262-
// (e.g. the name of a function expression), in which case errorSpan will be undefined.
263-
// Alternatively, it might be required and missing (e.g. the name of a module), in which
264-
// case its pos will equal its end (length 0). In either of these cases, we should fall
265-
// back to the original node that the error was issued on.
266-
return errorSpan && errorSpan.pos < errorSpan.end ? errorSpan : node;
276+
return createTextSpanFromBounds(pos, errorNode.end);
267277
}
268278

269279
export function isExternalModule(file: SourceFile): boolean {

‎tests/baselines/reference/ambientErrors.errors.txt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
tests/cases/conformance/ambient/ambientErrors.ts(2,15): error TS1039: Initializers are not allowed in ambient contexts.
2-
tests/cases/conformance/ambient/ambientErrors.ts(6,1): error TS2382: Specialized overload signature is not assignable to any non-specialized signature.
2+
tests/cases/conformance/ambient/ambientErrors.ts(6,18): error TS2382: Specialized overload signature is not assignable to any non-specialized signature.
33
tests/cases/conformance/ambient/ambientErrors.ts(17,22): error TS2371: A parameter initializer is only allowed in a function or constructor implementation.
44
tests/cases/conformance/ambient/ambientErrors.ts(20,24): error TS1184: An implementation cannot be declared in ambient contexts.
55
tests/cases/conformance/ambient/ambientErrors.ts(24,5): error TS1066: Ambient enum elements can only have integer literal initializers.
@@ -25,7 +25,7 @@ tests/cases/conformance/ambient/ambientErrors.ts(57,5): error TS2309: An export
2525
// Ambient functions with invalid overloads
2626
declare function fn(x: number): string;
2727
declare function fn(x: 'foo'): number;
28-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
28+
~~
2929
!!! error TS2382: Specialized overload signature is not assignable to any non-specialized signature.
3030

3131
// Ambient functions with duplicate signatures

‎tests/baselines/reference/anyIdenticalToItself.errors.txt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
1-
tests/cases/compiler/anyIdenticalToItself.ts(1,1): error TS2394: Overload signature is not compatible with function implementation.
1+
tests/cases/compiler/anyIdenticalToItself.ts(1,10): error TS2394: Overload signature is not compatible with function implementation.
22
tests/cases/compiler/anyIdenticalToItself.ts(6,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
33
tests/cases/compiler/anyIdenticalToItself.ts(10,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
44

55

66
==== tests/cases/compiler/anyIdenticalToItself.ts (3 errors) ====
77
function foo(x: any);
8-
~~~~~~~~~~~~~~~~~~~~~
8+
~~~
99
!!! error TS2394: Overload signature is not compatible with function implementation.
1010
function foo(x: any);
1111
function foo(x: any, y: number) { }

‎tests/baselines/reference/assignLambdaToNominalSubtypeOfFunction.errors.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ tests/cases/compiler/assignLambdaToNominalSubtypeOfFunction.ts(8,4): error TS234
1616
!!! error TS2345: Argument of type '(a: any, b: any) => boolean' is not assignable to parameter of type 'IResultCallback'.
1717
!!! error TS2345: Property 'x' is missing in type '(a: any, b: any) => boolean'.
1818
fn(function (a, b) { return true; })
19-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
19+
~~~~~~~~
2020
!!! error TS2345: Argument of type '(a: any, b: any) => boolean' is not assignable to parameter of type 'IResultCallback'.
2121
!!! error TS2345: Property 'x' is missing in type '(a: any, b: any) => boolean'.
2222

‎tests/baselines/reference/collisionSuperAndLocalFunctionInAccessors.errors.txt

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(4,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
22
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(9,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
33
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(15,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
4-
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(16,9): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
4+
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(16,18): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
55
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(20,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
6-
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(21,9): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
6+
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(21,18): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
77
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(26,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
8-
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(28,13): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
8+
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(28,22): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
99
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(33,9): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
10-
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(35,13): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
10+
tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(35,22): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
1111

1212

1313
==== tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts (10 errors) ====
@@ -33,20 +33,18 @@ tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(35,13): error
3333
~~~~~
3434
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
3535
function _super() { // Should be error
36-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
37-
}
38-
~~~~~~~~~
36+
~~~~~~
3937
!!! error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
38+
}
4039
return 10;
4140
}
4241
set prop2(val: number) {
4342
~~~~~
4443
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
4544
function _super() { // Should be error
46-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
47-
}
48-
~~~~~~~~~
45+
~~~~~~
4946
!!! error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
47+
}
5048
}
5149
}
5250
class c extends Foo {
@@ -55,10 +53,9 @@ tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(35,13): error
5553
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
5654
var x = () => {
5755
function _super() { // Should be error
58-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
59-
}
60-
~~~~~~~~~~~~~
56+
~~~~~~
6157
!!! error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
58+
}
6259
}
6360
return 10;
6461
}
@@ -67,10 +64,9 @@ tests/cases/compiler/collisionSuperAndLocalFunctionInAccessors.ts(35,13): error
6764
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
6865
var x = () => {
6966
function _super() { // Should be error
70-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
71-
}
72-
~~~~~~~~~~~~~
67+
~~~~~~
7368
!!! error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
69+
}
7470
}
7571
}
7672
}

‎tests/baselines/reference/collisionSuperAndLocalFunctionInConstructor.errors.txt

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
tests/cases/compiler/collisionSuperAndLocalFunctionInConstructor.ts(12,9): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
2-
tests/cases/compiler/collisionSuperAndLocalFunctionInConstructor.ts(20,13): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
1+
tests/cases/compiler/collisionSuperAndLocalFunctionInConstructor.ts(12,18): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
2+
tests/cases/compiler/collisionSuperAndLocalFunctionInConstructor.ts(20,22): error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
33

44

55
==== tests/cases/compiler/collisionSuperAndLocalFunctionInConstructor.ts (2 errors) ====
@@ -15,21 +15,19 @@ tests/cases/compiler/collisionSuperAndLocalFunctionInConstructor.ts(20,13): erro
1515
constructor() {
1616
super();
1717
function _super() { // Should be error
18-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
19-
}
20-
~~~~~~~~~
18+
~~~~~~
2119
!!! error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
20+
}
2221
}
2322
}
2423
class c extends Foo {
2524
constructor() {
2625
super();
2726
var x = () => {
2827
function _super() { // Should be error
29-
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
30-
}
31-
~~~~~~~~~~~~~
28+
~~~~~~
3229
!!! error TS2401: Duplicate identifier '_super'. Compiler uses '_super' to capture base class reference.
30+
}
3331
}
3432
}
3533
}

0 commit comments

Comments
 (0)