Fix S1125 FP: using ternary operator and throw expressions

[costin-zaharia-sonarsource]

Description

Rule S1125 is raising FPs when analyzing ternary operators.

Repro steps

    public bool Method(bool condition)
    {
        return condition == false
            ? true
            : throw new Exception();
    }

    public bool Method2(bool condition)
    {
        return condition
            ? throw new Exception()
            : true;
    }

Expected behavior

Either the message needs to be improved to suggest a refactoring (remove the == false and flip the condition) or don't raise the message at all.

For the second method the issue should not be raised at all.

Actual behavior

Issues are raised for both false and true literals.

Additionally, the generated code by the code fix provider is not correct and it will not compile:

    public bool Method3(bool condition)
    {
        return !condition || throw new Exception();
    }

    public bool Method4(bool condition)
    {
        return !condition
               || throw new Exception()
            ; // Fixed
    }

[StingyJack]

As there is nothing incorrect about the first one, it should not be flagging it either.

Code isn't always automatically more readable in truthy conditional paths (2nd example). A lot of times it's going to be clearer using a falsy (1st example). Sonar shouldn't be trying to dictate one way over the other, or be forcing someone's particular code style preference when there isn't a specific problem it's trying to prevent.

While you are in there, please fix the other bug with S1125 where it is flagging a nonredundant false as redundant so we can actually activate this rule in our QP's.

[pavel-mikula-sonarsource]

The == false is independent discussion happening on other places that should not be sneaked into this specific use case of ternary operator unrelated to == false part.

[StingyJack]

@pavel-mikula-sonarsource This reported defect wont be fixed unless it doesnt raise an issue for either true or false. You (sonar) cant determine what the more correct condition to be in the if or else path, so you shouldn't be coercing it.

I dont see how this can be fixed for both true and false without fixing == false as well. There arent any logical differences between these. The pattern is the same.

return  condition == false
            ? DoSomething()
            : throw new Exception();
 //    or 
if (condition == false) 
{
     return DoSomething();
}
throw new Exception();

[costin-zaharia-sonarsource]

A possible solution is to suggest the removal of == false and flip the ternary branches. This will be easier to read and maintain.

[StingyJack]

@costin-zaharia-sonarsource , @pavel-mikula-sonarsource just told me to stop sneaking reference to that in.

But you would be wrong about easier to read and maintain anyway. Sonar does not know whether a true conditional or a false conditional is more appropriate and readable for a section of code. It should not be forcing customers to change code around to suit Sonar's rigid coding style and incorrect idea of redundancy in this rule or force them to disable the S1125 rule and lose out on its benefits.

code 
code
code
code 
lots of code

if (someCondition == false)
{
   DoSomethingExtra()
}

code
code
etc.
return something;

Tell me how to switch that code around to make the if condition true and have it work properly and be more readable. Even for a 3 line ternary Sonar isnt going to be able to judge how readable one way is over the other.

Also mentally understanding "if not true" is going to require more effort than "if false"

[costin-zaharia-sonarsource]

@StingyJack, we are in the context of ternary operator and I was writing strictly about that. I was trying to ideate and suggest possible solutions. We will decide later what the next iteration for this rule will be.

To clarify a bit what I mean:

return condition == false ? true : throw new Exception();

could be replaced by:

return condition ? throw new Exception() : true;

[StingyJack]

Neither of those two patterns are wrong as they are written because neither is unclear or obfuscates the intention for the reader.

You have a personal preference for a style that only wants and expects to see an non-redundant true being evaluated for by the if condition. I used to prefer that also but its not always appropriate for the code flow. Its wrong to shoehorn code into a pattern when that degrades our ability to read and understand it properly

If there was a third and fourth example of

var returnValue =  !IsReady() ? ReRunSetupAndThenGo() : Go();
return returnValue; //splitting the return statement from its derivation makes an easier debugging experience.

var returnValue = IsReady() == false ? ReRunSetupAndThenGo() : Go();
return returnValue;

... with regards to fixing any FP for this, example 4 should not trigger S1125. the "false" is not redundant. The pattern for #4 is a better pattern because it avoids bugs due to the ! being missed when next to I and other characters with left side bars. Using == false is harder for someone to miss. Do you naturally say "is false" or "not is true"? Both convey the idea, but the latter is more stilted.

[pavel-mikula-sonarsource]

Reproducer from another thread that's focused on the reported problem:

var x = (key is null) ? throw new ArgumentNullException(nameof(key)) : false;

[AraHaan]

let's see if the problem goes away if you used switch expressions:

    public bool Method(bool condition)
    {
        return !condition switch
        {
            true => true,
            false => throw new Exception(),
        };
    }

    public bool Method2(bool condition)
    {
        return condition switch
        {
            true => throw new Exception(),
            false => true;
        }
    }

[StingyJack]

Getting this on another kind of code that was a ternary at one point but was refactored to be simpler and does not throw exceptions...

var isAppNamePathSegmentNeeded = string.IsNullOrWhiteSpace(appName) == false;

produces

S1125	Remove the unnecessary Boolean literal(s).

Removing the false would invert the logic at all places where the variable is used and would require renaming of the variable. Doing all that is what seems unnecessary to me.

[AraHaan]

It basically wants you to change that line to:

var isAppNamePathSegmentNeeded = !string.IsNullOrWhiteSpace(appName);

[StingyJack]

It basically wants you to change...

Thats not what it says it wants. It says my boolean literal is unnecessary, which is true only on Opposite Day. The current message is more incorrect and misleading than "Remove redundant boolean literals", which I think is what it used to say.

(There is some history to go along with this complaint)

[andrei-epure-sonarsource]

I've changed the title of this issue to focus on the initial (and most annoying, recurring) FP.