bug: false positive of `no-fallthrough` for `break` or `continue` inside nested switch inside loop

[KnorpelSenf]

Lint Name

no-fallthrough

Code Snippet

export function repro(...nums: number[]) {
  for (let i = 0; i < nums.length; i++) {
    switch (nums[i]) {
      case 0:
        switch (i) {
          case 0:
            continue;
          default:
            return 0;
        }
      case 1:
        return 1;
    }
  }
}

Expected Result

No linting errors

Actual Result

$ deno lint
error[no-fallthrough]: Fallthrough is not allowed
  --> /tmp/repro/main.ts:4:7
   | 
 4 |       case 0:
   |       ^^^^^^^
   | 
 5 |         switch (i) {
   | ^^^^^^^^^^^^^^^^^^^^
   | 
 6 |           case 0:
   | ^^^^^^^^^^^^^^^^^
   | 
 7 |             continue;
   | ^^^^^^^^^^^^^^^^^^^^^
   | 
 8 |           default:
   | ^^^^^^^^^^^^^^^^^^
   | 
 9 |             return 0;
   | ^^^^^^^^^^^^^^^^^^^^^
   | 
10 |         }
   | ^^^^^^^^^
   = hint: Add `break` or comment `/* falls through */` to your case statement

  docs: https://lint.deno.land/rules/no-fallthrough

Found 1 problem
Checked 1 file

Additional Info

The same problem can be observed when replacing continue by break.

Version

$ deno --version 
deno 1.46.3 (stable, release, x86_64-unknown-linux-gnu)
v8 12.9.202.5-rusty
typescript 5.5.2

This can also be seen with the RC of Deno 2.0.

$ deno --version 
deno 2.0.0-rc.4 (release candidate, release, x86_64-unknown-linux-gnu)
v8 12.9.202.13-rusty
typescript 5.6.2

[KnorpelSenf]

I tried looking into this issue but if I understand the current implementation correctly, then does not keep track of the complete lexical context when analysing the AST. Consider the following code:

foo: while (x) {
    bar: switch (y) {
        default:
            baz: switch (z) {
                default:
                    break bar;
            }
    }
    // (*)
}

Whether or not (*) will run depends on the label passed to break. If we break baz or bar, it will run. However, if the source contained break foo, then (*) is dead code. Similar examples can be crafted with arbitrarily deep nesting. This means that any line of code needs access to the complete path to the next higher function declaration in the AST (or the top level of the module).

So this is where I am stuck. I don't see a way to do this without performing big changes to the way nodes are visited. Please let me know if I'm missing something. If there is indeed a way to get this information, I am happy to try fixing this again.

[magurotuna]

@KnorpelSenf Thanks for looking into it. Actually, our control flow analyzer has some edge cases it can't handle properly. It makes sense to me that we overhaul the control flow analyzer. I believe other JS/TS linters in the ecosystem, such as biome or oxlint, might be very useful to get ideas from.

[KnorpelSenf]

That makes sense. I would have been fine with fixing a bug myself, but I don't think I want to go down the rabbit hole of writing a complete JS control flow analyzer in Rust just to get this resolved. I'd be happy to test a new implementation and provide feedback if that helps, but until somebody is able to work on this, I will simply suppress the lint.

[KnorpelSenf]

A bit of a radical idea but should we maybe investigate wrapping oxlint instead? If we're able to keep the same API without having to do the analysis anymore, then that will save a ton of time.

I understand that it made sense for Deno to ship its own linter and formatter etc in 2020 but a lot has happened since then. Especially with https://voidzero.dev it could be cool to align and converge and contribute back, rather than competing.

[bartlomieju]

@KnorpelSenf it's being considered, but that would be a big change and a big additional dependency to add. Might have more info at the end of the week.

[KnorpelSenf]

It would be a huge change, yes, but it would also be pretty impressive to pull this off. Deno could then use the OXC parser and formatter, too, which means we can get rid of SWC. That in turns improves performance. In your blog post you mentioned that a proper bundler would be added in the future, too. Switching over to an existing toolchain would give us this tooling for free. At the same time, the core team can focus on the actual runtime, JSR, and Deploy. Finally, not supporting npm from the start alienated a lot of people who primarily do frontend work, and embracing a toolchain that the same people are excited about will win some of them back. So regarding correctness, performance, feature set, focus, and devrel/community, it makes perfect sense. Those are just my 2c, looking forward to more info from you guys :)