oxc
oxc copied to clipboard
[oxc_linter] eslint(no-this-before-super) better error diagnostics
General advice on error spans https://oxc-project.github.io/docs/contribute/linter.html#general-advice:
General Advice
Pin point the error message to the shortest code span
We want the user to focus on the problematic code rather than deciphering the error message to identify which part of the code is erroneous.
Currently, we are pointing to the entire method:
1 │ class A extends B { constructor() { this.c = 0; } }
· _____________________________
Biome uses a compound diagnostic for this. For example:
correctness/noUnreachableSuper.js:2:5 [lint/correctness/noUnreachableSuper](https://biomejs.dev/docs/linter/rules/no-unreachable-super) ━━━━━━━━━━━━━━━━━━━━━━━━━━
✖ This constructor has code paths accessing `this` without calling `super()` first.
1 │ class A extends B {
> 2 │ constructor(value) {
│ ^^^^^^^^^^^^^^^^^^^^
> 3 │ this.prop = value;
> 4 │ super();
> 5 │ }
│ ^
6 │ }
7 │
ℹ `this` is accessed here:
1 │ class A extends B {
2 │ constructor(value) {
> 3 │ this.prop = value;
│ ^^^^
4 │ super();
While this seems better than what we have now, it can sometimes still feel wanting. For example:
correctness/noUnreachableSuper.js:2:5 [lint/correctness/noUnreachableSuper](https://biomejs.dev/docs/linter/rules/no-unreachable-super) ━━━━━━━━━━━━━━━━━━━━━━━━━━
✖ This constructor has code paths that return without calling `super()`.
1 │ class A extends B {
> 2 │ constructor(cond) {
│ ^^^^^^^^^^^^^^^^^^^
> 3 │ if(cond) {
> 4 │ super();
> 5 │ }
> 6 │ }
│ ^
7 │ }
8 │
ℹ If this is intentional, add an explicit throw statement in unsupported paths.
I would love to see something similar to the rust compiler's lifetime errors:
error[E0499]: cannot borrow `s` as mutable more than once at a time
--> src/main.rs:5:14
|
4 | let r1 = &mut s;
| ------ first mutable borrow occurs here
5 | let r2 = &mut s;
| ^^^^^^ second mutable borrow occurs here
6 |
7 | println!("{}, {}", r1, r2);
| -- first borrow later used here
For more information about this error, try `rustc --explain E0499`.
I'm wondering how smart we should be here. Here are some cases to consider:
- super is called conditionally, so there are code paths in which
this
is used beforesuper
.- We could help the user determine which code paths cause the error.
- super is called, but only later in the constructor.
- We could draw attention to this to help the user see the cause of the error.
- super was not called at all in the constructor.
- In this case, we have another lint
constructor-super
that should catch this. What should happen ifsuper
is not called at all, butthis
is used? Do we have 2 lints? Just one? I think in this case, it's fine to simply show the error with thethis
usage and letconstructor-super
tell the user thatsuper
isn't called at all.
- In this case, we have another lint
-
this
orsuper
are referenced multiple times beforesuper
is called.- Should we highlight each violation of the rule? Or only the first one?
-
this
is referenced twice: once beforesuper
and once after.- Should we mention the second use of
this
at all? I think if we call attention to the first usage in context and mention where super is called, the user should not get confused.
- Should we mention the second use of
- I'm wondering if a
super.a
is used beforesuper()
if the error might be confusing.- Should we add a note that super must be called as a function before it can be used in a different context?
I'm thinking something like this
⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access.
╭─[no_this_before_super.tsx:2:17]
1 │ class A extends B {
2 │ ╭─▶ constructor(value) {
3 │ │ this.prop = value;
· │ ──── this is accessed here
4 │ │ super();
· │ ─────── super is not called until here
5 │ ╰─▶ }
6 │ }
╰────
help: Call super() before this/super property access.
This extends nicely to multiple uses:
⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access.
╭─[no_this_before_super.tsx:2:17]
1 │ class A extends B {
2 │ ╭─▶ constructor(value) {
3 │ │ this.prop = value;
· │ ──── this is accessed here
4 │ │ if (value === SOMETHING_SPECIAL) {
5 │ │ this.special = true;
· │ ──── this is accessed here
6 │ │ }
7 │ │ super();
· │ ─────── super is not called until here
8 │ ╰─▶ }
9 │ }
╰────
help: Call super() before this/super property access.
This could also work for conditional calls to super()
:
⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access.
╭─[no_this_before_super.tsx:2:17]
1 │ class A extends B {
2 │ ╭─▶ constructor(value) {
3 │ │ if (value === SOMETHING_SPECIAL) {
4 │ │ super();
· │ ─────── super is called here conditionally
5 │ │ this.special = true;
6 │ │ }
7 │ │ this.prop = value;
· │ ──── this is accessed here
8 │ ╰─▶ }
9 │ }
╰────
help: Call super() before this/super property access.