Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

🐛 lint/rules/noUnreachableSuper triggers a false positive if the constructor contains an if statement that returns in both clauses #4616

Closed
1 task done
lgarron opened this issue Jun 25, 2023 · 3 comments · Fixed by biomejs/biome#97
Assignees
Labels
A-Linter Area: linter S-Bug: confirmed Status: report has been confirmed as a valid bug

Comments

@lgarron
Copy link

lgarron commented Jun 25, 2023

Environment information

(Excluded, as `npx rome rage` outputs a suuuuper verbose error related to LSP, and this is reproducible in the playground. This is reproducible using the CLI version `12.1.3-nightly.50eb45f`.)

What happened?

Run the linter on code like the following:

class A {
  constructor() {
    console.log("Constructing A!")
  }
}

// Note that this particular example can be "simplified" to avoid the linter bug. But it still serves to demonstrate what may happen in more complicated code.
class B extends A {
  n: number | undefined;
  constructor(n) {
    super();
    if (typeof n === "string") {
      if (n === "default") {
        return;
      } else {
        this.n = parseInt(n);
        return;
      }
    }
    this.n = n
  }
}

https://docs.rome.tools/playground/?code=YwBsAGEAcwBzACAAQQAgAHsACgAgACAAYwBvAG4AcwB0AHIAdQBjAHQAbwByACgAKQAgAHsACgAgACAAIAAgAGMAbwBuAHMAbwBsAGUALgBsAG8AZwAoACIAQwBvAG4AcwB0AHIAdQBjAHQAaQBuAGcAIABBACEAIgApAAoAIAAgAH0ACgB9AAoACgAvAC8AIABOAG8AdABlACAAdABoAGEAdAAgAHQAaABpAHMAIABwAGEAcgB0AGkAYwB1AGwAYQByACAAZQB4AGEAbQBwAGwAZQAgAGMAYQBuACAAYgBlACAAIgBzAGkAbQBwAGwAaQBmAGkAZQBkACIAIAB0AG8AIABhAHYAbwBpAGQAIAB0AGgAZQAgAGwAaQBuAHQAZQByACAAYgB1AGcALgAgAEIAdQB0ACAAaQB0ACAAcwB0AGkAbABsACAAcwBlAHIAdgBlAHMAIAB0AG8AIABkAGUAbQBvAG4AcwB0AHIAYQB0AGUAIAB3AGgAYQB0ACAAbQBhAHkAIABoAGEAcABwAGUAbgAgAGkAbgAgAG0AbwByAGUAIABjAG8AbQBwAGwAaQBjAGEAdABlAGQAIABjAG8AZABlAC4ACgBjAGwAYQBzAHMAIABCACAAZQB4AHQAZQBuAGQAcwAgAEEAIAB7AAoAIAAgAG4AOgAgAG4AdQBtAGIAZQByACAAfAAgAHUAbgBkAGUAZgBpAG4AZQBkADsACgAgACAAYwBvAG4AcwB0AHIAdQBjAHQAbwByACgAbgApACAAewAKACAAIAAgACAAcwB1AHAAZQByACgAKQA7AAoAIAAgACAAIABpAGYAIAAoAHQAeQBwAGUAbwBmACAAbgAgAD0APQA9ACAAIgBzAHQAcgBpAG4AZwAiACkAIAB7AAoAIAAgACAAIAAgACAAaQBmACAAKABuACAAPQA9AD0AIAAiAGQAZQBmAGEAdQBsAHQAIgApACAAewAKACAAIAAgACAAIAAgACAAIAByAGUAdAB1AHIAbgA7AAoAIAAgACAAIAAgACAAfQAgAGUAbABzAGUAIAB7AAoAIAAgACAAIAAgACAAIAAgAHQAaABpAHMALgBuACAAPQAgAHAAYQByAHMAZQBJAG4AdAAoAG4AKQA7AAoAIAAgACAAIAAgACAAIAAgAHIAZQB0AHUAcgBuADsACgAgACAAIAAgACAAIAB9AAoAIAAgACAAIAB9AAoAIAAgACAAIAB0AGgAaQBzAC4AbgAgAD0AIABuAAoAIAAgAH0ACgB9AAoA

For the real-world example that triggered this, see: https://github.com/cubing/cubing.js/blob/143181657b7010404bf8e2f8ea726dcb68434ed2/src/cubing/alg/alg-nodes/leaves/Move.ts#L140-L160

This errors with lint/correctness/noUnreachableSuper using the message: This constructor has code paths accessing this without calling super() first.

Expected result

No linter error.

Code of Conduct

  • I agree to follow Rome's Code of Conduct
@lgarron lgarron added the S-To triage Status: user report of a possible bug that needs to be triaged label Jun 25, 2023
lgarron added a commit to cubing/cubing.js that referenced this issue Jun 25, 2023
This rule has two bugs that cause false positives in our codebase:

- rome/tools#4615
- rome/tools#4616

We can re-enable the rule once those are resolved.
@nissy-dev
Copy link
Contributor

I think this issue was fixed by #4614

@Conaclos
Copy link
Contributor

@nissy-dev Not sure, the issue is still present on the playground.

Another issue is also with try-catch:

playground

class A extends B {
    constructor() {
    // This constructor has code paths accessing `this` without calling `super()` first.
        super();
        try {} catch {}
        this.prop = 0;
    }
}

playground

class A extends B {
    constructor() {
    // This constructor has code paths that return without calling `super()`.
        super();
        try {} catch {}
    }
}

lgarron added a commit to cubing/cubing.js that referenced this issue Jun 25, 2023
This rule has two bugs that cause false positives in our codebase:

- rome/tools#4615
- rome/tools#4616

We can re-enable the rule once those are resolved.
@nissy-dev
Copy link
Contributor

@Conaclos @lgarron
Oh, I hadn't checked correctly 🙇 I’m sorry for this.
I will look into this issue.

@nissy-dev nissy-dev added S-Bug: confirmed Status: report has been confirmed as a valid bug A-Linter Area: linter and removed S-To triage Status: user report of a possible bug that needs to be triaged labels Jun 26, 2023
@nissy-dev nissy-dev self-assigned this Jul 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter S-Bug: confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants