SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
772 stars 226 forks source link

Improve S1125: Flexibility in opting in and out constant patterns #8178

Open antonioaversa opened 11 months ago

antonioaversa commented 11 months ago

The context

After this issue, reported in 2019, was implemented earlier this year, we have got a user request to relax the reporting in presence of constant patterns: https://github.com/SonarSource/sonar-dotnet/issues/8147.

The goal

Provide the user with more flexibility, by allowing to opt in and out:

Possible solutions

From @antonioaversa I think we have four options here:

  1. keep the rule as is, and reject the user request: b is false and b is true remain redundant

Or split the rule into two: take out is cases from 1125 and create a new rule for them:

  1. with the new rule treating b is true as redundant and b is false as redundant

  2. with the new rule treating b is true as redundant and b is false as valid

  3. Or keep b is true in S1125 and have the new rule only report on b is false.

From @Tim-Pohlmann

There could be three rules:

  1. Use x instead of x == true and x is true.
  2. Use !x instead of x == false and x is false.
  3. Use x is true/false instead of x == true/false.

Allowed patterns based on enabled rules:

In addition to that 3 could also have an effect on != or that could be a separate thing.

AronGreen commented 8 months ago

It would also be nice if the rule is relaxed when the comparison involves checking the run-time type of a boxed value.

A slimmed down example from a WPF IValueConverter implementation where the is true pattern is reported as a code smell, but returning value is not equivalent.

public object Convert(object value)
{
    return value is true;
}
WarBorg commented 1 month ago

hello, any news when this will be done ? we are also struggling to remove the ! and use is false whenever possible in our code but this rule kinda hinders us at the moment

MaxFmi commented 1 month ago

Having is false as a valid option sounds good to me. is true is redundant and should IMHO not be valid

Tim-Pohlmann commented 1 month ago

I increased the priority of this ticket. Maybe we can pick it up in a future hardening sprint.