Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix StinkyBooleanExpression analyzer #190

Merged
merged 3 commits into from
Jan 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using System;

namespace StyleChecker.Test.Refactoring.StinkyBooleanExpression
{
public sealed class Okay
Expand All @@ -11,5 +13,11 @@ public void BothNonBoolLiteral(bool b, bool whenTrue, bool whenFalse)
{
_ = b ? whenFalse : whenTrue;
}

public void ThrowStatement(bool b1, bool b2)
{
_ = b1 ? throw new Exception() : false;
_ = b2 ? true : throw new Exception();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ static bool AreBothBoolLiterals(ConditionalExpressionSyntax s)
=> BoolLiteralExpressionSet.SetEquals(
[s.WhenTrue.Kind(), s.WhenFalse.Kind()]);

static bool IsAnyThrowExpr(ConditionalExpressionSyntax s)
=> s.AnyHasKindOf(SyntaxKind.ThrowExpression);

var cancellationToken = context.CancellationToken;
var model = context.SemanticModel;
var root = model.SyntaxTree
Expand All @@ -85,13 +88,14 @@ static bool AreBothBoolLiterals(ConditionalExpressionSyntax s)
.OfType<ConditionalExpressionSyntax>()
.SelectMany(ToConditionalPods)
.Where(p => IsOperationTypeBool(p.Operation)
&& !AreBothBoolLiterals(p.Node))
&& !AreBothBoolLiterals(p.Node)
&& !IsAnyThrowExpr(p.Node))
.ToList();
var allToUseConditionalLogicalAnd = targets.Where(
p => p.Node.BothIsKind(SyntaxKind.TrueLiteralExpression))
p => p.Node.AnyHasKindOf(SyntaxKind.TrueLiteralExpression))
.Select(p => (p.Node, R.UseConditionalLogicalOr));
var allToUseConditionalLogicalOr = targets.Where(
p => p.Node.BothIsKind(SyntaxKind.FalseLiteralExpression))
p => p.Node.AnyHasKindOf(SyntaxKind.FalseLiteralExpression))
.Select(p => (p.Node, R.UseConditionalLogicalAnd));
var all = allToUseConditionalLogicalAnd
.Concat(allToUseConditionalLogicalOr)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ void Register(
{
if (root.FindNodeOfType<ConditionalExpressionSyntax>(span)
is not {} node
|| !node.BothIsKind(kind))
|| !node.AnyHasKindOf(kind))
{
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ namespace StyleChecker.Refactoring.StinkyBooleanExpression;
public static class Conditionals
{
/// <summary>
/// Gets whether both the true and false branches of a conditional
/// expression have the specified syntax kind.
/// Gets whether any of the true or false branches of the conditional
/// expression has the specified syntax type.
/// </summary>
/// <param name="node">
/// The conditional expression syntax node.
Expand All @@ -20,10 +20,10 @@ public static class Conditionals
/// The syntax kind to check for.
/// </param>
/// <returns>
/// <c>true</c> if both branches have the specified syntax kind; otherwise,
/// <c>false</c>.
/// <c>true</c> if at least one branch has the specified syntax kind;
/// otherwise, <c>false</c>.
/// </returns>
public static bool BothIsKind(
public static bool AnyHasKindOf(
this ConditionalExpressionSyntax node, SyntaxKind kind)
{
return node.WhenTrue.IsKind(kind)
Expand Down
44 changes: 24 additions & 20 deletions doc/rules/StinkyBooleanExpression.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,45 +11,47 @@

## Summary

Do not use the conditional operator (`?:`)
where either the second or third operand is a `bool` literal
(`true` or `false`), resulting in `bool` values.
Do not use the conditional operator (`?:`) where either the second or third
operand is a `bool` literal (`true` or `false`), resulting in `bool` values.

## Default severity

Warning

## Description

There are some stinky boolean expressions with a conditional
operator that can be replaced with the `&&` (conditional logical
AND) or `||` (conditional logical OR) operator, as follows:
There are some stinky boolean expressions with a conditional operator that can
be replaced with the `&&` (conditional logical AND) or `||` (conditional logical
OR) operator, as follows:

```csharp
(b1 ? b2 : false)
(b1 ? true : b2)
```

where the type of `b1` and `b2` is `bool`. It is possible to
replace the former conditional expression with `b1 && b2`, the
latter with `b1 || b2`.
where the type of `b1` and `b2` is `bool`. It is possible to replace the former
conditional expression with `b1 && b2`, the latter with `b1 || b2`.

### Throw expression

Conditional expressions where the second or third operand is a [_throw expression_][ThrowExpression] are not covered, as follows:

```csharp
(b ? true : throw new Exception(…))
```

### Remarks

The diagnostics IDE0057 providing
_Simplify conditional expressions refactoring_,
which is available with
[Visual Studio 2019 version 16.6 preview
2][microsoft:vs2019-v16.6-preview-2],
includes the same feature as this analyzer.
The diagnostics IDE0057 providing _Simplify conditional expressions
refactoring_, which is available with [Visual Studio 2019 version 16.6 preview
2][microsoft:vs2019-v16.6-preview-2], includes the same feature as this
analyzer.

## Code fix

The code fix provides an option replacing the conditional
operator with the `&&` or `||` operator.
However, if the diagnostics IDE0057 provides an option
"Simplify conditional expression" with Visual Studio,
you should use it.
The code fix provides an option replacing the conditional operator with the `&&`
or `||` operator. However, if the diagnostics IDE0057 provides an option
"Simplify conditional expression" with Visual Studio, you should use it.

## Example

Expand Down Expand Up @@ -77,3 +79,5 @@ _ = (!(b1) || (b2));
https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes-preview#16.6.0-pre.2.1
[fig-StinkyBooleanExpression]:
https://maroontress.github.io/StyleChecker/images/StinkyBooleanExpression.png
[ThrowExpression]:
https://github.com/dotnet/csharplang/blob/main/proposals/csharp-7.0/throw-expression.md
Loading