oxc icon indicating copy to clipboard operation
oxc copied to clipboard

[oxc_linter] eslint(no-this-before-super) better error diagnostics

Open TzviPM opened this issue 1 year ago • 3 comments

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:

  1. super is called conditionally, so there are code paths in which this is used before super.
    • We could help the user determine which code paths cause the error.
  2. 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.
  3. 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 if super is not called at all, but this is used? Do we have 2 lints? Just one? I think in this case, it's fine to simply show the error with the this usage and let constructor-super tell the user that super isn't called at all.
  4. this or super are referenced multiple times before super is called.
    • Should we highlight each violation of the rule? Or only the first one?
  5. this is referenced twice: once before super 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.
  6. I'm wondering if a super.a is used before super() 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?

TzviPM avatar Feb 04 '24 04:02 TzviPM

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.

TzviPM avatar Feb 04 '24 04:02 TzviPM

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.

TzviPM avatar Feb 04 '24 04:02 TzviPM

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.

TzviPM avatar Feb 04 '24 04:02 TzviPM