SonarSource
Improve message for S1125 `no-redundant-boolean`
Rule should not simple recommend to remove the literal as negation might be required (e.g. if (x == false) -> if (!x))
Reported in https://community.sonarsource.com/t/sonarcloud-incorrectly-reports-that-a-boolean-literal-is-unnecessary/50558
It would probably not be just the message. If this rule is to determine redundancy of a boolean literal,
if (x == false)
.. is not redundant.
if (x == true)
.. is redundant.
Whether you prefer `"if x is false" or "if not x" is a code style choice and definitely not redundancy.
Hello @StingyJack,
We undestand the reasoning that you gave in both community and in here. However we will just change the message to be coherent with the rule description. The rule mentions specifically that boolean literals should be avoided. From that point of view, the message was wrong, as we cannot just suggest to always remove the literal. Instead we will just propose a refactoring. Totally agreed that this is a code style, but we must follow what the rule says.
Thanks
we must follow what the rule says
The rule says the same thing about redundancy. == false is not redundant. It is a style that reduces defects because it makes the intention more obvious.
Isn't Sonar's purpose to improve quality? Why would any of your rules work counter to that?
Hello @StingyJack ,
it says also reports on redundant boolean operations, which does not mean all are redundant. That's why we went for a more cautious message.
You mention that == false reduces defects. One could argue that could also lead to bugs or lack of readability as it could be confused with === false, as non-strict operators are less and less used. ! is the preferred option for falsy checks IMHO, but I understand these decisions are very subjective and very code style oriented.
This rule is about avoiding the use of Boolean literals (for all languages, not only js/ts), so we should keep the implementation coherent with the rule description. We consider this change to be the best approach to accomplish this.
=== false or == false either would be considered using a boolean literal. What is your reason for avoiding boolean literals in the first place?
I refer to the rule itself:
Boolean literals should be avoided in comparison expressions == and != to improve code readability.
=== false cannot be simplified with !, so it does not apply.
I repeat: agreed that this recommendation about refactoring == false to ! is subject to each project code style, so I would recommend to disable this rule in your project if you do not agree with the reasoning behind. As the rule is described at the moment, we consider the current implementation as adequate.
Pinging @gabriel-vivas-sonarsource see if he would like to add anything else.
I think == is always a smell in JS/TS, even with null and undefined.
The coercion that is implied by == is not obvious to the uninitiated, especially if coming from other languages.
I suggest always using === and moving on.
This rule won't bother you if you use strict equality.
This rule is about avoiding the use of Boolean literals (for all languages, not only js/ts), so we should keep the implementation coherent with the rule description
I know this is specifically calling out the TS/.JS rule but since you mention other languages, TBH this is more of a problem in C# for me since I use that most often and I dont really care about the difference between == and === in TypeScript since I dont have to really have worry about type coercion .
I refer to the rule itself: Boolean literals should be avoided in comparison expressions == and != to improve code readability.
You are arguing for a style that is less explicit and that relies on the readers knowledge of these default behaviors and language specifics. Being terse is good for the writer of the code, but the effect is usually that it leaves the reader less able to determine the intent. (RegEx is often cited as an example of where being terse makes the code unmaintainable). So I dont get how increasing the knowledge burden and cognitive skill needed for the reader to understand the code is somehow improving readability.
Microsoft agrees with me on the explicit approach being desirable
@StingyJack you're posting on the JS/TS static analyzer. Here's the link to the .NET one, I'm sure they can help: https://github.com/SonarSource/sonar-dotnet
BTW, I do not recommend writing TypeScript the same way you write C#, don't be deceived by the similarities!
@gabriel-vivas-sonarsource I'm not the one who brought up other languages, but the rule implementations are broken in the same way, and the rule in general is counter to the advice of the language author. Based on this and other interactions, it appears Sonar's definition of "code readability" only favors concision (the amount of text to read) and does not consider clarity of intent - the latter of which is far more important.
Programmers should be getting rewarded for writing code that can be easily understood and modified. They should not be getting penalized and coerced into using syntax shortcuts .
@StingyJack I think this is all a misunderstanding. I agree with you, being explicit is preferable and should be encouraged.
Using !x in JavaScript and TypeScript is ironically more explicit than using x == false.
The first one tells you that the writer enjoys coercion, the second one well, is basically a lie.
The code x == false is to be discouraged in JavaScript and TypeScript because it is misleading. Not because it is longer.
> false == false
true
> "" == false
true
> 0 == false
true
> [] == false
true
If you think you have that figured out, check these:
> {} == false
false
> undefined == false
false
> null == false
false
If you are looking to be explicit, you need to pay one more extra character and say x === false.
Which is even longer and honestly better:
> false === false
true
> "" === false
false
> 0 === false
false
> [] === false
false
> {} === false
false
> undefined === false
false
> null === false
false
Regarding C#, I truly don't know what is the reasoning.
I dont think I would ever try to write most of those in the first place. Coercion or not, asking the computer if 0 or an empty string is equal to false is silly. And something is null or is not null, I wouldnt even think to ask it if null was true or false.
In the examples I gave above for
if (x == false)
.. is not redundant.
and
if (x == true)
.. is redundant.
x will always be a boolean for me, sometimes its used like this...
if (isSomethingReady(someParam) == false)
... which is really the scenario that bothers me the most about this rule because it gets marked as "Boolean is redundant" and expects me to change the code to
if (!isSomethingReady(someParam))
... which is the less explicit syntax that I have personally seen create bugs because the negation operator is easily missed between the ( and the i