🐛 Analyzer: too many diagnostics for`js/noDeadCode`

[Boshen]

Environment information

Playground

What happened?

Given dead code separated by control blocks, it outputs every single node/identifier/statement as a separate diagnostic.

function foo() {
  return

  if (x) {
    a
  }

  bar()

  while(y) {}

  b
}

warning[[js/noDeadCode](https://rome.tools/docs/lint/rules/noDeadCode/)]: This code is unreachable
  ┌─ main.js:4:7
  │
2 │   return
  │   ------ This statement will return from the function ...
3 │ 
4 │   if (x) {
  │       - ... before it can reach this code

warning[[js/noDeadCode](https://rome.tools/docs/lint/rules/noDeadCode/)]: This code is unreachable
  ┌─ main.js:5:5
  │
2 │   return
  │   ------ This statement will return from the function ...
  ·
5 │     a
  │     - ... before it can reach this code

warning[[js/noDeadCode](https://rome.tools/docs/lint/rules/noDeadCode/)]: This code is unreachable
  ┌─ main.js:8:3
  │
2 │   return
  │   ------ This statement will return from the function ...
  ·
8 │   bar()
  │   ----- ... before it can reach this code

warning[[js/noDeadCode](https://rome.tools/docs/lint/rules/noDeadCode/)]: This code is unreachable
   ┌─ main.js:10:9
   │
 2 │   return
   │   ------ This statement will return from the function ...
   ·
10 │   while(y) {}
   │         - ... before it can reach this code

warning[[js/noDeadCode](https://rome.tools/docs/lint/rules/noDeadCode/)]: This code is unreachable
   ┌─ main.js:12:3
   │
 2 │   return
   │   ------ This statement will return from the function ...
   ·
12 │   b
   │   - ... before it can reach this code

Expected result

Preferably a single diagnostic for a block of continuous statements.

Let me shamefully show the result of my implementation ...

Code of Conduct

• [X] I agree to follow Rome's Code of Conduct

[IWANABETHATGUY]

This is more likely a suggestion rather than a bug, @ematipico, what do you think? Do we need to add another issue type?

[ematipico]

This is more likely a suggestion rather than a bug, @ematipico, what do you think? Do we need to add another issue type?

For suggestions, we should use GitHub discussions. We have a "suggestions" category.

@Boshen

While I might agree that the messaging can be verbose, I would argue that the counter proposal is too slim for Rome's standards. We pledge to always explain the reason of an error, trying to point to the source of the error.

[Boshen]

It's actually not about the explanation.

If you look closely, Rome is currently reporting a separate "before it can reach this code" for every single node/identifier/statement. My suggestion is to combine these into a single reporting span.

I found this from a real case here in lodash, where we get a screenful of diagnostics ...

[leops]

This is partially due to a technical limitation in the way the Language Server Protocol represents diagnostics: in order to mark code as "useless" in the editor we need to emit a diagnostic covering that range, but a single diagnostic may only cover one continuous range at once. So when multiple statement or expressions are marked as unreachable in a function, the noDeadCode rule will merge contiguous diagnostics but if two unreachable ranges do not immediately follow each other the rule needs to emit multiple diagnostics instead. Now in this case I agree that the logical range of code that's unreachable is single unit covering everything after the return statement and we should try to improve the range merging algorithm to support this by propagating unreachable ranges to the outer node. This would need to be implemented on a per-node basis, but the corresponding logic for JsWhileStatement for instance would be that if both the test expression and the entire body are covered by an unreachable range, these two ranges get merged into a single one covering the entire while loop (and running this logic in a loop so this new range recursively gets merged with the unreachable range covering the rest of the function body)

[github-actions[bot]]

This issue is stale because it has been open 14 days with no activity.

[ematipico]

@leops is there something we can do to close this possible bug? If not a bug, then we should just close it

[leops]

@leops is there something we can do to close this possible bug? If not a bug, then we should just close it

I tried to come up with a solution to this a few weeks ago, and I think it would require more time to finish it. I still think this is important though, given how much work is required to implement the solution I would probably re-qualify this from a bug to a missing feature

[github-actions[bot]]

This issue is stale because it has been open 14 days with no activity.