JS/TS rule S6606 suggestion to replace || with ? is ambiguous

SonarQube for IDE
,
1

Hello thank you for providing such a helpful tool.

Rule S6606 (JS/TS) suggests to “Prefer using nullish coalescing operator (??) instead of a logical or (||), as it is a safer operator”.

The operators have different behavior.

false ?? 'text' returns false
false || 'text' returns 'text'

They are not interchangeable. Maybe some advice in the short info and the Rule Description would be helpful.

“This rule reports when disjunctions (||) and conditionals (?) can be safely replaced with coalescing (??).”

This is not the case.

Example TypeScript:
Because x can be a bool false the operators can not be safely replaced.

const x: null | undefined | boolean | string;

const y = x || '';

In this case a safe replacement is possible

const x: null | undefined | string;

const y = x || '';

In TypeScript the rule can be fitted to just take effect in cases where false is not possible, I don’t see that in JavaScript, maybe this should be a TS-only rule an be deactivated for JS.

Sincerely Paco

  • Operating system: Ubuntu 22.04
  • IDE name and flavor/env: VS Code
2 Likes
4

Hello @paco,

thanks for reporting this. Indeed seems the rule is not properly checking the whole union. I’ve opened a ticket to fix this behavior.

Thanks
Victor

2 Likes
6

Hi, I think in typing, you mixed up the return values for this part of your post.
It should be the below instead.
It would be nice if you modify it so as to minimize confusion for others. Thanks.

false ?? 'text' returns false
false || 'text' returns 'text'
2 Likes
7

That’s correct, unfortunately I am not able to modify or edit the post.
Maybe because it is my first post or something.

Funny enough I can edit this reply.

8

Edited, thanks for the heads up @itunuloluwa.

Anyway we got the message :slight_smile:

9

Super! Thanks, everyone.

1 Like
10

This is also an issue when using a ternary to check if something is exactly undefined or exactly null:

let x = null; // intentionally null, rather than undefined
let y = x === undefined ? 100 : x.value; // causes S6606 but should not

There are times when it’s important to check if a value is exactly undefined vs. being specifically null.

11

I’m also seeing this. The irony being suggesting the replacement is safer but actually making this change is often dangerous and changes behaviour.

1 Like
12

FYI: FP suggesting nullish coalescing operator - #7 by michal.zgliczynski

13

So now it’s a couple of years later and nothing has happened. I can’t see why anyone would prefer a empty string for instance when somevariable ?? ‘default’ , while somevariable || ‘default’ works as expected. And those that really want ‘’ then simply variable||‘’ !
I would hope at least check for string variable should be done and not suggest ?? in that case.

14

Hey @Sten_W

I believe a PR was merged to fix this 17 days ago

So we shouldn’t be far away from cutting a new version of SonarJS, which will then roll onto SonarQube Cloud and SonarQube Server/Community Build (probably in that order)

Sorry it has taken a while, but hopefully this is all resolved soon!

2 Likes
15

A new version of the analyzer will be released this week, so you should not see this issue after it has been deployed (mid-week probably).

sorry it took so long to have this fixed

1 Like
16

thanks. I found them removed now.

1 Like