Skip to content

Commit

Permalink
Fix StinkyBooleanExpression analyzer (#190)
Browse files Browse the repository at this point in the history
- Add test cases for StinkyBooleanExpression analyzer
- Fix StinkyBooleanExpression analyzer to allow conditional expressions where
  the second or third operand is a throw expression
- Update documentation
  • Loading branch information
maroontress-tomohisa authored Jan 4, 2025
1 parent 5519b78 commit 2aced8f
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 29 deletions.
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

0 comments on commit 2aced8f

Please sign in to comment.