💅 noUselessElse recommendation is wrong

[anonrig]

Environment information

CLI:
  Version:                      1.4.0
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "alacritty"
  JS_RUNTIME_VERSION:           "v20.10.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/10.2.3"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 false

Workspace:
  Open Documents:               0

Rule name

noUselessElse

Playground link

https://github.com/getsentry/sentry-javascript/blob/develop/rollup/plugins/extractPolyfillsPlugin.js

Expected result

The following file has invalid "FIXABLE" output:

https://github.com/getsentry/sentry-javascript/blob/develop/rollup/plugins/extractPolyfillsPlugin.js

./rollup/plugins/extractPolyfillsPlugin.js:121:5 lint/style/noUselessElse  FIXABLE  ━━━━━━━━━━━━━━━━

  ✖ This else clause can be omitted.

    119 │     // or
    120 │     //   `const { dogs } = { dogs: () => "are great" }`
  > 121 │     else if (declarationId.type === 'ObjectPattern') {
        │     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  > 122 │       return declarationId.properties[0].key.name;
         ...
  > 126 │       return 'unknown variable';
  > 127 │     }
        │     ^
    128 │   }
    129 │

  ℹ This if statement uses an early breaking statement.

    112 │     // or
    113 │     //   `const dogs = () => "are great";
  > 114 │     if (declarationId.type === 'Identifier') {
        │     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  > 115 │       return declarationId.name;
         ...
  > 126 │       return 'unknown variable';
  > 127 │     }
        │     ^
    128 │   }
    129 │

  ℹ Unsafe fix: Omit the else clause.

    114 114 │       if (declarationId.type === 'Identifier') {
    115 115 │         return declarationId.name;
    116     │ - ····}
    117     │ - ····//·Declarations·of·the·form
    118     │ - ····//···`const·{·dogs·}·=·{·dogs:·function()·{·return·"are·great";·}·}`
    119     │ - ····//·or
    120     │ - ····//···`const·{·dogs·}·=·{·dogs:·()·=>·"are·great"·}`
    121     │ - ····else·if·(declarationId.type·===·'ObjectPattern')·{
        116 │ + ····}if·(declarationId.type·===·'ObjectPattern')·{
    122 117 │         return declarationId.properties[0].key.name;
    123 118 │       }

Code of Conduct

• I agree to follow Biome's Code of Conduct

[Conaclos]

Hi @anonrig!

Can you be a more specific about the bug / the expected result? Is it the removal of the comments?

[anonrig]

Hi @Conaclos, it removes the recommendations and also on line 116, it produces false JavaScript code:

}if (declarationId.type === 'ObjectPattern') {

[Conaclos]

recommendations

I don't see any issue with the emitted JavaScript (leaving apart the removed comments that should definitively be preserved). Do you mean it is not correctly formatted?

[spd789562]

I'm facing the same issue, but end up I'm realizing it because the main if is using early return like this in your code

if (declarationId.type === 'Identifier') {
  return declarationId.name;
}

So next line dont need to use else if, because if it true, it never run rest of code.
It mean if there has other case, just need to use if

replace this

if (type === 'case A') {
  return 'A'
} else if (type === 'case B') {
  return 'B'
} else {
  return 'C'
}

to this

if (type === 'case A') {
  return 'A'
}
if (type === 'case B') {
  return 'B'
}
return 'C'

[Conaclos]

@spd789562 Do you think the rules' diagnostics could be improved? Any suggestions?

[spd789562]

Not sure about it, I got this lint error when use VScode, and it just say

This else clause can be omitted.

But the problem is there has early breaking in that if statement, and some people tent to write else if even it's early breaking statement. So omit else if doesn't make sense at first glance.

#no-useless-else is giving the good example though.
And lint error also should not be too long, so I don't have good improve suggestion, sorry.

[ematipico]

Yeah maybe we can make our message better and explain why the else can be omitted. Users shouldn't spend too much time understanding the reason of a fix (regardless of its nature)

[Conaclos]

We could add an explanation:

This else clause can be omitted because previous branches break early.

[ematipico]

Sounds good to me!