Skip to content

Shorten error spans for errors reported on constructor declarations #58061

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

Merged
merged 6 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
10 changes: 10 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2268,6 +2268,16 @@ export function getErrorSpanForNode(sourceFile: SourceFile, node: Node): TextSpa
const pos = skipTrivia(sourceFile.text, (node as JSDocSatisfiesTag).tagName.pos);
return getSpanOfTokenAtPosition(sourceFile, pos);
}
case SyntaxKind.Constructor: {
const constructorDeclaration = node as ConstructorDeclaration;
const start = skipTrivia(sourceFile.text, constructorDeclaration.pos);
const scanner = createScanner(sourceFile.languageVersion, /*skipTrivia*/ true, sourceFile.languageVariant, sourceFile.text, /*onError*/ undefined, start);
while (scanner.scan() !== SyntaxKind.ConstructorKeyword) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems pretty hefty to need an entire rescan to understand this; the info above doesn't seem to do this either. Is the position not just after the modifiers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems pretty hefty to need an entire rescan to understand this;

this is actually what some of the other branches do here indirectly (see the calls to getSpanOfTokenAtPosition)

Is the position not just after the modifiers?

that would roughly be the start position of the constructor keyword but for some of the errors it makes sense to highlight the modifiers and I think there is little value in differentiating this span between different error types (highlighting modifiers is fine since they usually are placed on the same line anyway). The bigger problem is when it comes to finding the end position of this span. I've tried to use node.parameters.start but it wasn't too precise. That's how I landed on this solution

Copy link
Member

Choose a reason for hiding this comment

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

I have a fear that some bad parse recovery mechanism will possibly make this code end up in an endless loop. Consider bailing on an EOF token or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be highly-unlikely cause we know that we are already processing ConstructorDeclaration and I can't imagine a situation in which that would be created without the ConstructorKeyword. Being extra safe is probably for the better though - I applied the suggested change

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I didn't realize that getSpanOfTokenAtPosition creates a scanner each time. Definitely expensive but not new for this.

Why didn't getSpanOfTokenAtPosition work for this again? I guess I'm not sure why we'd want to highlight modifiers at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was worried about classConstructorOverloadsAccessibility but taking another look at it right now... it doesn't look bad if we only highlight the constructor keyword there:

git diff
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
index f13946d637..5965e78154 100644
--- a/src/compiler/utilities.ts
+++ b/src/compiler/utilities.ts
@@ -2274,12 +2274,12 @@ export function getErrorSpanForNode(sourceFile: SourceFile, node: Node): TextSpa
         }
         case SyntaxKind.Constructor: {
             const constructorDeclaration = node as ConstructorDeclaration;
-            const start = skipTrivia(sourceFile.text, constructorDeclaration.pos);
-            const scanner = createScanner(sourceFile.languageVersion, /*skipTrivia*/ true, sourceFile.languageVariant, sourceFile.text, /*onError*/ undefined, start);
+            const scanner = createScanner(sourceFile.languageVersion, /*skipTrivia*/ true, sourceFile.languageVariant, sourceFile.text, /*onError*/ undefined, skipTrivia(sourceFile.text, constructorDeclaration.pos));
             let token = scanner.scan();
             while (token !== SyntaxKind.ConstructorKeyword && token !== SyntaxKind.EndOfFileToken) {
                 token = scanner.scan();
             }
+            const start = skipTrivia(sourceFile.text, scanner.getTokenStart());
             const end = scanner.getTokenEnd();
             return createTextSpanFromBounds(start, end);
         }
diff --git a/tests/baselines/reference/classConstructorOverloadsAccessibility.errors.txt b/tests/baselines/reference/classConstructorOverloadsAccessibility.errors.txt
index e939262b02..1b0723b506 100644
--- a/tests/baselines/reference/classConstructorOverloadsAccessibility.errors.txt
+++ b/tests/baselines/reference/classConstructorOverloadsAccessibility.errors.txt
@@ -1,15 +1,15 @@
-classConstructorOverloadsAccessibility.ts(2,2): error TS2385: Overload signatures must all be public, private or protected.
-classConstructorOverloadsAccessibility.ts(3,2): error TS2385: Overload signatures must all be public, private or protected.
-classConstructorOverloadsAccessibility.ts(11,2): error TS2385: Overload signatures must all be public, private or protected.
+classConstructorOverloadsAccessibility.ts(2,9): error TS2385: Overload signatures must all be public, private or protected.
+classConstructorOverloadsAccessibility.ts(3,12): error TS2385: Overload signatures must all be public, private or protected.
+classConstructorOverloadsAccessibility.ts(11,12): error TS2385: Overload signatures must all be public, private or protected.
 
 
 ==== classConstructorOverloadsAccessibility.ts (3 errors) ====
     class A {
     	public constructor(a: boolean) // error
-    	~~~~~~~~~~~~~~~~~~
+    	       ~~~~~~~~~~~
 !!! error TS2385: Overload signatures must all be public, private or protected.
     	protected constructor(a: number) // error
-    	~~~~~~~~~~~~~~~~~~~~~
+    	          ~~~~~~~~~~~
 !!! error TS2385: Overload signatures must all be public, private or protected.
     	private constructor(a: string)
     	private constructor() { 
@@ -19,7 +19,7 @@ classConstructorOverloadsAccessibility.ts(11,2): error TS2385: Overload signatur
     
     class B {
     	protected constructor(a: number) // error
-    	~~~~~~~~~~~~~~~~~~~~~
+    	          ~~~~~~~~~~~
 !!! error TS2385: Overload signatures must all be public, private or protected.
     	constructor(a: string)
     	constructor() { 
diff --git a/tests/baselines/reference/parserConstructorDeclaration8.errors.txt b/tests/baselines/reference/parserConstructorDeclaration8.errors.txt
index 8b37b60364..c798654628 100644
--- a/tests/baselines/reference/parserConstructorDeclaration8.errors.txt
+++ b/tests/baselines/reference/parserConstructorDeclaration8.errors.txt
@@ -1,4 +1,4 @@
-parserConstructorDeclaration8.ts(3,3): error TS2390: Constructor implementation is missing.
+parserConstructorDeclaration8.ts(3,10): error TS2390: Constructor implementation is missing.
 parserConstructorDeclaration8.ts(3,21): error TS1005: '(' expected.
 
 
@@ -6,7 +6,7 @@ parserConstructorDeclaration8.ts(3,21): error TS1005: '(' expected.
     class C {
       // Not a constructor
       public constructor;
-      ~~~~~~~~~~~~~~~~~~
+             ~~~~~~~~~~~
 !!! error TS2390: Constructor implementation is missing.
                         ~
 !!! error TS1005: '(' expected.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean, yeah. Maybe it's best as-is. That or TS2385 should have been on the modifiers the whole time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That or TS2385 should have been on the modifiers the whole time?

I'm not sure what do u mean by "that" here :p TS2385 - that's the one I think might be worth keeping over modifiers too. I don't feel super strong about it though

Copy link
Member

Choose a reason for hiding this comment

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

I was just saying that we could either do:

  • This PR as is, squiggle over modifiers
  • Change TS2385 to only squiggle on modifiers, change this PR to only squiggle on constructor.

But the latter seems like work.

@DanielRosenwasser probably should say what feels best here; I don't know if the existing change is a problem, really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the latter seems like work.

Yeah, this isn't 100% straightforward because right now those errors are reported on nodes and u'd like to raise over a span or nodes array or something. I'm not sure if there is an existing infra for this at the higher level (closer to the checker - where the errors are raised). I'd prefer not to investigate this right now 😉

// empty
}
const end = scanner.getTokenEnd();
return createTextSpanFromBounds(start, end);
}
}

if (errorNode === undefined) {
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/ClassDeclaration10.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ ClassDeclaration10.ts(3,4): error TS2391: Function implementation is missing or
==== ClassDeclaration10.ts (2 errors) ====
class C {
constructor();
~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2390: Constructor implementation is missing.
foo();
~~~
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/ClassDeclaration11.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ ClassDeclaration11.ts(2,4): error TS2390: Constructor implementation is missing.
==== ClassDeclaration11.ts (1 errors) ====
class C {
constructor();
~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2390: Constructor implementation is missing.
foo() { }
}
2 changes: 1 addition & 1 deletion tests/baselines/reference/ClassDeclaration14.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ ClassDeclaration14.ts(3,4): error TS2390: Constructor implementation is missing.
~~~
!!! error TS2391: Function implementation is missing or not immediately following the declaration.
constructor();
~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2390: Constructor implementation is missing.
}
2 changes: 1 addition & 1 deletion tests/baselines/reference/ClassDeclaration8.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ ClassDeclaration8.ts(2,3): error TS2390: Constructor implementation is missing.
==== ClassDeclaration8.ts (1 errors) ====
class C {
constructor();
~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2390: Constructor implementation is missing.
}
6 changes: 2 additions & 4 deletions tests/baselines/reference/bases.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ bases.ts(18,9): error TS2339: Property 'y' does not exist on type 'C'.
!!! error TS2420: Property 'x' is missing in type 'C' but required in type 'I'.
!!! related TS2728 bases.ts:2:5: 'x' is declared here.
constructor() {
~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
this.x: any;
~~~~~~~~~~~~~~~~~~~~
~~~~
!!! error TS17009: 'super' must be called before accessing 'this' in the constructor of a derived class.
~
Expand All @@ -47,8 +47,6 @@ bases.ts(18,9): error TS2339: Property 'y' does not exist on type 'C'.
~~~
!!! error TS2693: 'any' only refers to a type, but is being used as a value here.
}
~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
}

new C().x;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ classConstructorOverloadsAccessibility.ts(11,2): error TS2385: Overload signatur
==== classConstructorOverloadsAccessibility.ts (3 errors) ====
class A {
public constructor(a: boolean) // error
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~
!!! error TS2385: Overload signatures must all be public, private or protected.
protected constructor(a: number) // error
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~
!!! error TS2385: Overload signatures must all be public, private or protected.
private constructor(a: string)
private constructor() {
Expand All @@ -19,7 +19,7 @@ classConstructorOverloadsAccessibility.ts(11,2): error TS2385: Overload signatur

class B {
protected constructor(a: number) // error
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~
!!! error TS2385: Overload signatures must all be public, private or protected.
constructor(a: string)
constructor() {
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/classUpdateTests.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ classUpdateTests.ts(113,1): error TS1128: Declaration or statement expected.

class F extends E {
constructor() {} // ERROR - super call required
~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,18 @@ classWithTwoConstructorDefinitions.ts(8,5): error TS2392: Multiple constructor i
==== classWithTwoConstructorDefinitions.ts (4 errors) ====
class C {
constructor() { } // error
~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
constructor(x) { } // error
~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
}

class D<T> {
constructor(x: T) { } // error
~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
constructor(x: T, y: T) { } // error
~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
}
16 changes: 6 additions & 10 deletions tests/baselines/reference/constructorOverloads1.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,21 @@ constructorOverloads1.ts(17,18): error TS2769: No overload matches this call.
==== constructorOverloads1.ts (6 errors) ====
class Foo {
constructor(s: string);
~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
constructor(n: number);
~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
constructor(x: any) {
~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.

}
~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
constructor(x: any) {
~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.

}
~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
bar1() { /*WScript.Echo("bar1");*/ }
bar2() { /*WScript.Echo("bar1");*/ }
}
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/constructorOverloads3.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ constructorOverloads3.ts(12,5): error TS2377: Constructors for derived classes m
constructor(n: number);
constructor(a: any);
constructor(x: any, y?: any) { }
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
bar1() { /*WScript.Echo("Yo");*/}
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/constructorOverloads8.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ constructorOverloads8.ts(3,5): error TS2392: Multiple constructor implementation
==== constructorOverloads8.ts (2 errors) ====
class C {
constructor(x) { }
~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
constructor(y, x) { } // illegal, 2 constructor implementations
~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2392: Multiple constructor implementations are not allowed.
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ constructorWithIncompleteTypeAnnotation.ts(261,1): error TS1128: Declaration or
private otherValue = 42;

constructor(private value: number, public name: string) : }
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2390: Constructor implementation is missing.
~~~~~~~~~~~~~~~~~~~~~
!!! error TS2369: A parameter property is only allowed in a constructor implementation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ constructorsWithSpecializedSignatures.ts(26,5): error TS2394: This overload sign
class D {
constructor(x: "hi");
constructor(x: "foo");
~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2394: This overload signature is not compatible with its implementation signature.
!!! related TS2750 constructorsWithSpecializedSignatures.ts:20:5: The implementation signature is declared here.
constructor(x: number);
Expand All @@ -32,7 +32,7 @@ constructorsWithSpecializedSignatures.ts(26,5): error TS2394: This overload sign
class D2 {
constructor(x: "hi");
constructor(x: "foo");
~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2394: This overload signature is not compatible with its implementation signature.
!!! related TS2750 constructorsWithSpecializedSignatures.ts:28:5: The implementation signature is declared here.
constructor(x: string);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ derivedClassConstructorWithoutSuperCall.ts(24,31): error TS2337: Super calls are

class Derived extends Base {
constructor() { // error
~~~~~~~~~~~~~~~~~~~~~~~~
}
~~~~~
~~~~~~~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
}
}

class Base2<T> {
Expand All @@ -26,26 +25,22 @@ derivedClassConstructorWithoutSuperCall.ts(24,31): error TS2337: Super calls are

class Derived2<T> extends Base2<T> {
constructor() { // error for no super call (nested scopes don't count)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
var r2 = () => super(); // error for misplaced super call (nested function)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~
!!! error TS2337: Super calls are not permitted outside constructors or in nested functions inside constructors.
}
~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
}

class Derived3<T> extends Base2<T> {
constructor() { // error
~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
var r = function () { super() } // error
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~
!!! error TS2337: Super calls are not permitted outside constructors or in nested functions inside constructors.
}
~~~~~
!!! error TS2377: Constructors for derived classes must contain a 'super' call.
}

class Derived4<T> extends Base2<T> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,20 +66,16 @@ derivedClassParameterProperties.ts(81,9): error TS17009: 'super' must be called
a = 1;
b: number;
constructor(y: string) {
~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2376: A 'super' call must be the first statement in the constructor to refer to 'super' or 'this' when a derived class contains initialized properties, parameter properties, or private identifiers.
this.a = 3;
~~~~~~~~~~~~~~~~~~~
~~~~
!!! error TS17009: 'super' must be called before accessing 'this' in the constructor of a derived class.
this.b = 3;
~~~~~~~~~~~~~~~~~~~
~~~~
!!! error TS17009: 'super' must be called before accessing 'this' in the constructor of a derived class.
super();
~~~~~~~~~~~~~~~~
}
~~~~~
!!! error TS2376: A 'super' call must be the first statement in the constructor to refer to 'super' or 'this' when a derived class contains initialized properties, parameter properties, or private identifiers.
}

class Derived8 extends Base {
Expand All @@ -99,20 +95,16 @@ derivedClassParameterProperties.ts(81,9): error TS17009: 'super' must be called
a = 1;
b: number;
constructor(y: string) {
~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~
!!! error TS2376: A 'super' call must be the first statement in the constructor to refer to 'super' or 'this' when a derived class contains initialized properties, parameter properties, or private identifiers.
this.a = 3;
~~~~~~~~~~~~~~~~~~~
~~~~
!!! error TS17009: 'super' must be called before accessing 'this' in the constructor of a derived class.
this.b = 3;
~~~~~~~~~~~~~~~~~~~
~~~~
!!! error TS17009: 'super' must be called before accessing 'this' in the constructor of a derived class.
super();
~~~~~~~~~~~~~~~~
}
~~~~~
!!! error TS2376: A 'super' call must be the first statement in the constructor to refer to 'super' or 'this' when a derived class contains initialized properties, parameter properties, or private identifiers.
}

class Derived10<T> extends Base2<T> {
Expand Down
Loading