[DAS] Broadens the scope of no_literal_bool_comparisons

Fixes: #60614
Change-Id: I949cc9790468ccd14a1354c92945138310b5a659
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/430901
Auto-Submit: Felipe Morschel <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>

Author: FMorschel

pkg/analysis_server/lib/src/services/correction/dart/convert_to_boolean_expression.dart

@@ -6,10 +6,24 @@ import 'package:analysis_server/src/services/correction/fix.dart';
 import 'package:analysis_server_plugin/edit/dart/correction_producer.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/precedence.dart';
+import 'package:analyzer/dart/ast/token.dart';
+import 'package:analyzer/source/source_range.dart';
 import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
 import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
 import 'package:analyzer_plugin/utilities/range_factory.dart';
 
+/// This correction producer can act on a variety of code, and various edits
+/// might be applied.
+typedef _ParametersFixData =
+    ({
+      SourceRange parensRange,
+      List<SourceRange> deleteRanges,
+      bool negated,
+      bool needsParens,
+      (SourceRange, String)? replace,
+      bool bangBeforeParens,
+    });
+
 class ConvertToBooleanExpression extends ResolvedCorrectionProducer {
   ConvertToBooleanExpression({required super.context});
 
@@ -27,49 +41,202 @@ class ConvertToBooleanExpression extends ResolvedCorrectionProducer {
   Future<void> compute(ChangeBuilder builder) async {
     AstNode? node = this.node;
     if (node is BooleanLiteral) node = node.parent;
-    if (node is! BinaryExpression) return;
-
-    var rightOperand = node.rightOperand;
-    var leftOperand = node.leftOperand;
-
-    Expression expression;
-    BooleanLiteral literal;
-
-    var deleteRange = range.endEnd(leftOperand, rightOperand);
+    _ParametersFixData parameters;
 
-    if (rightOperand is BooleanLiteral) {
-      literal = rightOperand;
-      expression = node.leftOperand;
-    } else if (leftOperand is BooleanLiteral) {
-      literal = leftOperand;
-      expression = node.rightOperand;
-      deleteRange = range.startStart(leftOperand, rightOperand);
+    if (node case BinaryExpression(
+      :var rightOperand,
+      :var leftOperand,
+      operator: Token(type: var operator),
+    )) {
+      if (rightOperand is BooleanLiteral) {
+        parameters = _processBinaryExp(
+          node,
+          operator,
+          rightOperand,
+          leftOperand,
+          currentIsLeft: false,
+        );
+      } else if (leftOperand is BooleanLiteral) {
+        parameters = _processBinaryExp(
+          node,
+          operator,
+          leftOperand,
+          rightOperand,
+          currentIsLeft: true,
+        );
+      } else {
+        return;
+      }
+    } else if (node
+        case ConditionalExpression(
+              :var condition,
+              :var thenExpression,
+              :var elseExpression,
+            ) &&
+            var conditionalExp) {
+      _ParametersFixData? result = (switch ((thenExpression, elseExpression)) {
+        (BooleanLiteral then, BooleanLiteral elseExp) => () {
+          var equalValues = then.value == elseExp.value;
+          var rangeStart =
+              equalValues
+                  // keep `then`
+                  ? range.startStart(condition, then)
+                  // keep `condition`
+                  : range.endEnd(condition, then);
+          // remove ` : elseExp`
+          var rangeEnd = range.endEnd(then, elseExp);
+          return (
+            parensRange: range.node(equalValues ? then : condition),
+            deleteRanges: [rangeStart, rangeEnd],
+            negated: !then.value && elseExp.value,
+            replace: null,
+            needsParens: !equalValues && condition.needsParens,
+            bangBeforeParens: false,
+          );
+        }(),
+        (BooleanLiteral then, Expression elseExp) => () {
+          var replaceRange = range.endStart(condition, elseExp);
+          var operator = then.ifBarElseAmpersand;
+          return (
+            parensRange: range.node(conditionalExp),
+            deleteRanges: const <SourceRange>[],
+            negated: !then.value,
+            replace: (replaceRange, ' ${operator.lexeme} '),
+            // conditional expressions always need parens so there will
+            // be no need to add them
+            needsParens: false,
+            bangBeforeParens: false,
+          );
+        }(),
+        (Expression then, BooleanLiteral elseExp) => () {
+          var rangeStart = range.endStart(condition, then);
+          var rangeEnd = range.endEnd(then, elseExp);
+          var operator = elseExp.ifBarElseAmpersand;
+          return (
+            parensRange: range.node(conditionalExp),
+            deleteRanges: [rangeEnd],
+            negated: elseExp.value,
+            replace: (rangeStart, ' ${operator.lexeme} '),
+            // conditional expressions always need parens so there will
+            // be no need to add them
+            needsParens: false,
+            bangBeforeParens: false,
+          );
+        }(),
+        (_, _) => null,
+      });
+      if (result == null) {
+        return;
+      }
+      parameters = result;
     } else {
       return;
     }
+    await _addEdit(builder, parameters);
+  }
 
-    var negated = !isPositiveCase(node, literal);
+  Future<void> _addEdit(
+    ChangeBuilder builder,
+    _ParametersFixData parameters,
+  ) async {
+    var (
+      :parensRange,
+      :deleteRanges,
+      :negated,
+      :replace,
+      :needsParens,
+      :bangBeforeParens,
+    ) = parameters;
     await builder.addDartFileEdit(file, (builder) {
-      if (negated) {
-        builder.addSimpleInsertion(expression.offset, '!');
-        if (expression.needsParens) {
-          builder.addSimpleInsertion(expression.offset, '(');
-          builder.addSimpleInsertion(expression.end, ')');
+      if (bangBeforeParens) {
+        if (negated) {
+          builder.addSimpleInsertion(parensRange.offset, TokenType.BANG.lexeme);
+          if (needsParens) {
+            builder.addSimpleInsertion(parensRange.offset, '(');
+            builder.addSimpleInsertion(parensRange.end, ')');
+          }
+        }
+      } else {
+        if (needsParens) {
+          builder.addSimpleInsertion(parensRange.offset, '(');
         }
+        if (negated) {
+          builder.addSimpleInsertion(parensRange.offset, TokenType.BANG.lexeme);
+        }
+        if (needsParens) {
+          builder.addSimpleInsertion(parensRange.end, ')');
+        }
+      }
+      if (replace != null) {
+        builder.addSimpleReplacement(replace.$1, replace.$2);
+      }
+      for (var range in deleteRanges) {
+        builder.addDeletion(range);
       }
-      builder.addDeletion(deleteRange);
     });
   }
 
+  _ParametersFixData _processBinaryExp(
+    BinaryExpression binaryExp,
+    TokenType operator,
+    BooleanLiteral current,
+    Expression other, {
+    required bool currentIsLeft,
+  }) {
+    List<SourceRange> deleteRanges;
+    SourceRange parensRange;
+    bool needsParens;
+    switch (operator) {
+      case TokenType.BAR || TokenType.BAR_BAR when current.value:
+      case TokenType.AMPERSAND || TokenType.AMPERSAND_AMPERSAND
+          when !current.value:
+        deleteRanges = [
+          currentIsLeft
+              ? range.endEnd(current, other)
+              : range.startStart(other, current),
+        ];
+        parensRange = range.node(current);
+        needsParens = current.needsParens;
+      default:
+        deleteRanges = [
+          currentIsLeft
+              ? range.startStart(current, other)
+              : range.endEnd(other, current),
+        ];
+        parensRange = range.node(other);
+        needsParens = other.needsParens;
+    }
+    return (
+      negated: !isPositiveCase(binaryExp, current),
+      parensRange: parensRange,
+      deleteRanges: deleteRanges,
+      needsParens: needsParens,
+      replace: null,
+      bangBeforeParens: true,
+    );
+  }
+
   static bool isPositiveCase(
     BinaryExpression expression,
     BooleanLiteral literal,
   ) {
-    if (expression.operator.lexeme == '==') return literal.value;
-    return !literal.value;
+    return switch (expression.operator.type) {
+      TokenType.BAR ||
+      TokenType.BAR_BAR ||
+      TokenType.AMPERSAND ||
+      TokenType.AMPERSAND_AMPERSAND => true,
+      TokenType.EQ_EQ => literal.value,
+      TokenType.BANG_EQ || TokenType.CARET => !literal.value,
+      _ => throw StateError('Unexpected operator ${expression.operator.type}'),
+    };
   }
 }
 
+extension on BooleanLiteral {
+  TokenType get ifBarElseAmpersand =>
+      value ? TokenType.BAR_BAR : TokenType.AMPERSAND_AMPERSAND;
+}
+
 extension on Expression {
   bool get needsParens => precedence <= Precedence.prefix;
 }

pkg/analysis_server/test/abstract_single_unit.dart

@@ -18,6 +18,11 @@ import 'abstract_context.dart';
 class AbstractSingleUnitTest extends AbstractContextTest {
   bool verifyNoTestUnitErrors = true;
 
+  /// Whether the test code should parse carrets as position shorthands.
+  ///
+  /// Set this to `true` when the test code is using the carret operator.
+  bool keepCaret = false;
+
   TestCode? _parsedTestCode;
   late ParsedUnitResult testParsedResult;
   late ResolvedLibraryResult? testLibraryResult;
@@ -38,7 +43,10 @@ class AbstractSingleUnitTest extends AbstractContextTest {
 
   String get testCode => parsedTestCode.code;
   set testCode(String value) {
-    parsedTestCode = TestCode.parse(normalizeSource(value));
+    parsedTestCode = TestCode.parse(
+      normalizeSource(value),
+      positionShorthand: !keepCaret,
+    );
   }
 
   void addTestSource(String code) {