[L] Change in dart/sdk[main]: Flow analysis: add support for relational patterns.

[Paul Berry (Gerrit)]

Attention is currently required from: Johnni Winther.

Paul Berry would like Johnni Winther to review this change.

View Change

Flow analysis: add support for relational patterns.

There are two new API methods: `equalityRelationalPattern_end` (for
relational patterns using `==` or `!=`, where some degree of flow
analysis is warranted) and `nonEqualityRelationalPattern_end` (for all
other relational patterns, where all we care about is making sure that
both the "matched" and "unmatched" branches are reachable).

I've moved most of the logic for constant patterns into
`equalityRelationalPattern_end`, since it's essentially the same logic
(except that `equalityRelationalPattern_end` also has support for
`!=`).  So now, `constantPattern_end` simply calls
`equalityRelationalPattern_end` in the case where pattern support is
enabled.

I also had to make a change to `RelationalOperatorResolution` to make
it possible to distinguish `==` from `!=`.

Bug: https://github.com/dart-lang/sdk/issues/50419
Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded
---
M pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
M pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart
M pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
M pkg/_fe_analyzer_shared/test/mini_ast.dart
M pkg/analyzer/lib/src/generated/resolver.dart
M pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart
M pkg/front_end/test/spell_checking_list_code.txt
7 files changed, 381 insertions(+), 68 deletions(-)

diff --git a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
index eb96696..c940de0 100644
--- a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
+++ b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
@@ -294,6 +294,13 @@
       EqualityInfo<Type>? leftOperandInfo, EqualityInfo<Type>? rightOperandInfo,
       {bool notEqual = false});
 
+  /// Call this method after processing a relational pattern that uses an
+  /// equality operator (either `==` or `!=`).  [operand] should be the operand
+  /// to the right of the operator, [operandType] should be its static type, and
+  /// [notEqual] should be `true` iff the operator was `!=`.
+  void equalityRelationalPattern_end(Expression operand, Type operandType,
+      {bool notEqual = false});
+
   /// Retrieves the [ExpressionInfo] associated with [target], if known.  Will
   /// return `null` if (a) no info is associated with [target], or (b) another
   /// expression with info has been visited more recently than [target].  For
@@ -587,6 +594,10 @@
   /// Call this method after visiting a logical-or (`||`) pattern.
   void logicalOrPattern_end();
 
+  /// Call this method after processing a relational pattern that uses a
+  /// non-equality operator (any operator other than `==` or `!=`).
+  void nonEqualityRelationalPattern_end();
+
   /// Call this method just after visiting a non-null assertion (`x!`)
   /// expression.
   void nonNullAssert_end(Expression operand);
@@ -1200,6 +1211,16 @@
   }
 
   @override
+  void equalityRelationalPattern_end(Expression operand, Type operandType,
+      {bool notEqual = false}) {
+    _wrap(
+        'equalityRelationalPattern_end($operand, $operandType, '
+        'notEqual: $notEqual)',
+        () => _wrapped.equalityRelationalPattern_end(operand, operandType,
+            notEqual: notEqual));
+  }
+
+  @override
   ExpressionInfo<Type>? expressionInfoForTesting(Expression target) {
     return _wrap('expressionInfoForTesting($target)',
         () => _wrapped.expressionInfoForTesting(target),
@@ -1451,6 +1472,12 @@
   }
 
   @override
+  void nonEqualityRelationalPattern_end() {
+    _wrap('nonEqualityRelationalPattern_end()',
+        () => _wrapped.nonEqualityRelationalPattern_end());
+  }
+
+  @override
   void nonNullAssert_end(Expression operand) {
     return _wrap('nonNullAssert_end($operand)',
         () => _wrapped.nonNullAssert_end(operand));
@@ -3775,58 +3802,12 @@
       {required bool patternsEnabled}) {
     assert(_stack.last is _PatternContext<Type>);
     if (patternsEnabled) {
-      _EqualityCheckResult equalityCheckResult = _equalityCheck(
-          equalityOperand_end(expression, type), _scrutineeInfo!);
-      if (equalityCheckResult is _NoEqualityInformation) {
-        // We have no information so we have to assume the pattern might or
-        // might not match.
-        _unmatched = _join(_unmatched!, _current);
-      } else if (equalityCheckResult is _EqualityCheckIsNullCheck<Type>) {
-        FlowModel<Type>? ifNotNull;
-        if (equalityCheckResult.isReferenceOnRight) {
-          // The `null` literal is on the left hand side of the implicit
-          // equality check, meaning it is the constant value.  So the user is
-          // doing something like this:
-          //
-          //     if (v case null) { ... }
-          //
-          // So we want to promote the type of `v` in the case where the
-          // constant pattern *didn't* match.
-          ifNotNull = _nullCheckPattern();
-          if (ifNotNull == null) {
-            // `_nullCheckPattern` returns `null` in the case where the matched
-            // value type is non-nullable.  In fully sound programs, this would
-            // mean that the pattern cannot possibly match.  However, in mixed
-            // mode programs it might match due to unsoundness.  Since we don't
-            // want type inference results to change when a program becomes
-            // fully sound, we have to assume that we're in mixed mode, and thus
-            // the pattern might match.
-            ifNotNull = _current;
-          }
-        } else {
-          // The `null` literal is on the right hand side of the implicit
-          // equality check, meaning it is the scrutinee.  So the user is doing
-          // something silly like this:
-          //
-          //     if (null case c) { ... }
-          //
-          // (where `c` is some constant).  There's no variable to promote.
-          //
-          // Since flow analysis can't make use of the results of constant
-          // evaluation, we can't really assume anything; as far as we know, the
-          // pattern might or might not match.
-          ifNotNull = _current;
-        }
-        _unmatched = _join(_unmatched!, ifNotNull);
-      } else {
-        assert(equalityCheckResult is _GuaranteedEqual);
-        // Both operands are known by flow analysis to compare equal, so the
-        // constant pattern is guaranteed to match.  Since our approach to
-        // handling patterns in flow analysis uses "implicit and" semantics
-        // (initially assuming that the pattern always matches, and then
-        // updating the `_current` and `_unmatched` states to reflect what
-        // values the pattern rejects), we don't have to do any updates.
-      }
+      // The behavior of a constant pattern is the same as that of a relational
+      // pattern using `==`, except for the order of the operands to the `==`
+      // operator.  However, the order of operands to `==` doesn't matter for
+      // flow analysis, so we can just re-use the logic for a relational pattern
+      // using `==`.
+      equalityRelationalPattern_end(expression, type);
     } else {
       // Before pattern support was added to Dart, flow analysis didn't do any
       // promotion based on the constants in individual case clauses.  Also, it
@@ -3952,6 +3933,75 @@
   }
 
   @override
+  void equalityRelationalPattern_end(Expression operand, Type operandType,
+      {bool notEqual = false}) {
+    _EqualityCheckResult equalityCheckResult = _equalityCheck(
+        _scrutineeInfo!, equalityOperand_end(operand, operandType));
+    if (equalityCheckResult is _NoEqualityInformation) {
+      // We have no information so we have to assume the pattern might or
+      // might not match.
+      _unmatched = _join(_unmatched!, _current);
+    } else if (equalityCheckResult is _EqualityCheckIsNullCheck<Type>) {
+      FlowModel<Type>? ifNotNull;
+      if (!equalityCheckResult.isReferenceOnRight) {
+        // The `null` literal is on the right hand side of the implicit
+        // equality check, meaning it is the constant value.  So the user is
+        // doing something like this:
+        //
+        //     if (v case == null) { ... }
+        //
+        // So we want to promote the type of `v` in the case where the
+        // constant pattern *didn't* match.
+        ifNotNull = _nullCheckPattern();
+        if (ifNotNull == null) {
+          // `_nullCheckPattern` returns `null` in the case where the matched
+          // value type is non-nullable.  In fully sound programs, this would
+          // mean that the pattern cannot possibly match.  However, in mixed
+          // mode programs it might match due to unsoundness.  Since we don't
+          // want type inference results to change when a program becomes
+          // fully sound, we have to assume that we're in mixed mode, and thus
+          // the pattern might match.
+          ifNotNull = _current;
+        }
+      } else {
+        // The `null` literal is on the left hand side of the implicit
+        // equality check, meaning it is the scrutinee.  So the user is doing
+        // something silly like this:
+        //
+        //     if (null case == c) { ... }
+        //
+        // (where `c` is some constant).  There's no variable to promote.
+        //
+        // Since flow analysis can't make use of the results of constant
+        // evaluation, we can't really assume anything; as far as we know, the
+        // pattern might or might not match.
+        ifNotNull = _current;
+      }
+      if (notEqual) {
+        _unmatched = _join(_unmatched!, _current);
+        _current = ifNotNull;
+      } else {
+        _unmatched = _join(_unmatched!, ifNotNull);
+      }
+    } else {
+      assert(equalityCheckResult is _GuaranteedEqual);
+      if (notEqual) {
+        // Both operands are known by flow analysis to compare equal, so the
+        // constant pattern is guaranteed *not* to match.
+        _unmatched = _join(_unmatched!, _current);
+        _current = _current.setUnreachable();
+      } else {
+        // Both operands are known by flow analysis to compare equal, so the
+        // constant pattern is guaranteed to match.  Since our approach to
+        // handling patterns in flow analysis uses "implicit and" semantics
+        // (initially assuming that the pattern always matches, and then
+        // updating the `_current` and `_unmatched` states to reflect what
+        // values the pattern rejects), we don't have to do any updates.
+      }
+    }
+  }
+
+  @override
   ExpressionInfo<Type>? expressionInfoForTesting(Expression target) =>
       identical(target, _expressionWithInfo) ? _expressionInfo : null;
 
@@ -4353,6 +4403,14 @@
   }
 
   @override
+  void nonEqualityRelationalPattern_end() {
+    // Flow analysis has no way of knowing whether the operator will return
+    // `true` or `false`, so just assume the worst case--both cases are
+    // reachable and no promotions can be done in either case.
+    _unmatched = _join(_unmatched!, _current);
+  }
+
+  @override
   void nonNullAssert_end(Expression operand) {
     ReferenceWithType<Type>? operandReference =
         _getExpressionReference(operand);
@@ -5440,6 +5498,10 @@
       {bool notEqual = false}) {}
 
   @override
+  void equalityRelationalPattern_end(Expression operand, Type operandType,
+      {bool notEqual = false}) {}
+
+  @override
   ExpressionInfo<Type>? expressionInfoForTesting(Expression target) {
     throw new StateError(
         'expressionInfoForTesting requires null-aware flow analysis');
@@ -5692,6 +5754,9 @@
   void logicalOrPattern_end() {}
 
   @override
+  void nonEqualityRelationalPattern_end() {}
+
+  @override
   void nonNullAssert_end(Expression operand) {}
 
   @override
diff --git a/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart b/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart
index 49eb814..6979442 100644
--- a/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart
+++ b/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart
@@ -91,15 +91,27 @@
   });
 }
 
+/// Kinds of relational pattern operators that shared analysis needs to
+/// distinguish.
+enum RelationalOperatorKind {
+  /// The operator `==`
+  equals,
+
+  /// The operator `!=`
+  notEquals,
+
+  /// Any relational pattern operator other than `==` or `!=`
+  other,
+}
+
 /// Information about a relational operator.
 class RelationalOperatorResolution<Type extends Object> {
-  /// Is `true` when the operator is `==` or `!=`.
-  final bool isEquality;
+  final RelationalOperatorKind kind;
   final Type parameterType;
   final Type returnType;
 
   RelationalOperatorResolution({
-    required this.isEquality,
+    required this.kind,
     required this.parameterType,
     required this.returnType,
   });
@@ -1376,11 +1388,27 @@
         resolveRelationalPatternOperator(node, matchedValueType);
     Type operandContext = operator?.parameterType ?? unknownType;
     Type operandType = analyzeExpression(operand, operandContext);
+    bool isEquality;
+    switch (operator?.kind) {
+      case RelationalOperatorKind.equals:
+        isEquality = true;
+        flow.equalityRelationalPattern_end(operand, operandType,
+            notEqual: false);
+        break;
+      case RelationalOperatorKind.notEquals:
+        isEquality = true;
+        flow.equalityRelationalPattern_end(operand, operandType,
+            notEqual: true);
+        break;
+      default:
+        isEquality = false;
+        flow.nonEqualityRelationalPattern_end();
+        break;
+    }
     // Stack: (Expression)
     if (errors != null && operator != null) {
-      Type argumentType = operator.isEquality
-          ? operations.promoteToNonNull(operandType)
-          : operandType;
+      Type argumentType =
+          isEquality ? operations.promoteToNonNull(operandType) : operandType;
       if (!operations.isAssignableTo(argumentType, operator.parameterType)) {
         errors.argumentTypeNotAssignable(
           argument: operand,
diff --git a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
index 653dd65..fb97e6d 100644
--- a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
+++ b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
@@ -7197,6 +7197,186 @@
       });
     });
 
+    group('Relational pattern:', () {
+      group('==:', () {
+        test('Guaranteed match due to Null type', () {
+          h.run([
+            ifCase(expr('Null'), relationalPattern('==', nullLiteral), [
+              checkReachable(true),
+            ], [
+              checkReachable(false),
+            ])
+          ]);
+        });
+
+        test('In the general case, may or may not match', () {
+          h.run([
+            ifCase(expr('Object?'), relationalPattern('==', intLiteral(0)), [
+              checkReachable(true),
+            ], [
+              checkReachable(true),
+            ]),
+          ]);
+        });
+
+        test('Null pattern promotes unchanged scrutinee', () {
+          var x = Var('x');
+          h.run([
+            declare(x, initializer: expr('int?')),
+            ifCase(x.expr, relationalPattern('==', nullLiteral), [
+              checkReachable(true),
+              checkNotPromoted(x),
+            ], [
+              checkReachable(true),
+              checkPromoted(x, 'int'),
+            ])
+          ]);
+        });
+
+        test("Null pattern doesn't promote changed scrutinee", () {
+          var x = Var('x');
+          h.run([
+            declare(x, initializer: expr('int?')),
+            switch_(x.expr, [
+              wildcard()
+                  .when(x.write(expr('int?')).stmt.thenExpr(expr('bool')))
+                  .then([
+                break_(),
+              ]),
+              relationalPattern('==', nullLiteral).then([
+                checkReachable(true),
+                checkNotPromoted(x),
+              ]),
+              wildcard(expectInferredType: 'int').then([
+                checkReachable(true),
+                checkNotPromoted(x),
+              ]),
+            ]),
+          ]);
+        });
+
+        test('Null pattern promotes matched pattern var', () {
+          h.run([
+            ifCase(
+                expr('int?'),
+                relationalPattern('==', nullLiteral)
+                    .or(wildcard(expectInferredType: 'int')),
+                []),
+          ]);
+        });
+
+        test('Null pattern can even match non-nullable types', () {
+          // Due to mixed mode unsoundness, attempting to match `null` to a
+          // non-nullable type can still succeed, so in order to avoid
+          // unsoundness escalation, it's important that the matching case is
+          // considered reachable.
+          h.run([
+            ifCase(expr('int'), relationalPattern('==', nullLiteral), [
+              checkReachable(true),
+            ], [
+              checkReachable(true),
+            ]),
+          ]);
+        });
+      });
+
+      group('!=:', () {
+        test('Guaranteed mismatch due to Null type', () {
+          h.run([
+            ifCase(expr('Null'), relationalPattern('!=', nullLiteral), [
+              checkReachable(false),
+            ], [
+              checkReachable(true),
+            ])
+          ]);
+        });
+
+        test('In the general case, may or may not match', () {
+          h.run([
+            ifCase(expr('Object?'), relationalPattern('!=', intLiteral(0)), [
+              checkReachable(true),
+            ], [
+              checkReachable(true),
+            ]),
+          ]);
+        });
+
+        test('Null pattern promotes unchanged scrutinee', () {
+          var x = Var('x');
+          h.run([
+            declare(x, initializer: expr('int?')),
+            ifCase(x.expr, relationalPattern('!=', nullLiteral), [
+              checkReachable(true),
+              checkPromoted(x, 'int'),
+            ], [
+              checkReachable(true),
+              checkNotPromoted(x),
+            ])
+          ]);
+        });
+
+        test("Null pattern doesn't promote changed scrutinee", () {
+          var x = Var('x');
+          h.run([
+            declare(x, initializer: expr('int?')),
+            switch_(x.expr, [
+              wildcard()
+                  .when(x.write(expr('int?')).stmt.thenExpr(expr('bool')))
+                  .then([
+                break_(),
+              ]),
+              relationalPattern('!=', nullLiteral)
+                  .and(wildcard(expectInferredType: 'int'))
+                  .then([
+                checkReachable(true),
+                checkNotPromoted(x),
+              ]),
+            ]),
+          ]);
+        });
+
+        test('Null pattern promotes matched pattern var', () {
+          h.run([
+            ifCase(
+                expr('int?'),
+                relationalPattern('!=', nullLiteral)
+                    .and(wildcard(expectInferredType: 'int')),
+                []),
+          ]);
+        });
+
+        test('Null pattern can even match non-nullable types', () {
+          // Due to mixed mode unsoundness, attempting to match `null` to a
+          // non-nullable type can still succeed, so in order to avoid
+          // unsoundness escalation, it's important that the matching case is
+          // considered reachable.
+          h.run([
+            ifCase(expr('int'), relationalPattern('!=', nullLiteral), [
+              checkReachable(true),
+            ], [
+              checkReachable(true),
+            ]),
+          ]);
+        });
+      });
+
+      group('other:', () {
+        test('Does not assume anything, even though == or != would', () {
+          // This is a bit of a contrived test case, since it exercises
+          // `null < null`.  But such a thing is possible with extension
+          // methods.
+          h.addMember('Null', '<', 'bool Function(Object?)');
+          h.run([
+            ifCase(expr('Null'), relationalPattern('<', nullLiteral), [
+              checkReachable(true),
+            ], [
+              checkReachable(true),
+            ])
+          ]);
+        });
+      });
+    });
+
     group('Switch expression:', () {
       test('guarded', () {
         var x = Var('x');
diff --git a/pkg/_fe_analyzer_shared/test/mini_ast.dart b/pkg/_fe_analyzer_shared/test/mini_ast.dart
index 2bb4e14..a88064d 100644
--- a/pkg/_fe_analyzer_shared/test/mini_ast.dart
+++ b/pkg/_fe_analyzer_shared/test/mini_ast.dart
@@ -741,7 +741,9 @@
       Type matchedValueType, String operator) {
     if (operator == '==' || operator == '!=') {
       return RelationalOperatorResolution(
-          isEquality: true,
+          kind: operator == '=='
+              ? RelationalOperatorKind.equals
+              : RelationalOperatorKind.notEquals,
           parameterType: Type('Object'),
           returnType: Type('bool'));
     }
@@ -757,7 +759,7 @@
           'must accept a parameter');
     }
     return RelationalOperatorResolution(
-        isEquality: operator == '==' || operator == '!=',
+        kind: RelationalOperatorKind.other,
         parameterType: memberType.positionalParameters[0],
         returnType: memberType.returnType);
   }
diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
index bb93956..8eb019b 100644
--- a/pkg/analyzer/lib/src/generated/resolver.dart
+++ b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -1571,8 +1571,18 @@
     DartType matchedType,
   ) {
     var operatorLexeme = node.operator.lexeme;
-    var isEquality = const {'==', '!='}.contains(operatorLexeme);
-    var methodName = isEquality ? '==' : operatorLexeme;
+    RelationalOperatorKind kind;
+    String methodName;
+    if (operatorLexeme == '==') {
+      kind = RelationalOperatorKind.equals;
+      methodName = '==';
+    } else if (operatorLexeme == '!=') {
+      kind = RelationalOperatorKind.notEquals;
+      methodName = '==';
+    } else {
+      kind = RelationalOperatorKind.other;
+      methodName = operatorLexeme;
+    }
 
     var result = typePropertyResolver.resolve(
       receiver: null,
@@ -1603,7 +1613,7 @@
     }
 
     return RelationalOperatorResolution(
-      isEquality: isEquality,
+      kind: kind,
       parameterType: parameterType,
       returnType: element.returnType,
     );
diff --git a/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart b/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart
index eb1c954..9178ace 100644
--- a/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart
+++ b/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart
@@ -10131,12 +10131,15 @@
   RelationalOperatorResolution<DartType>? resolveRelationalPatternOperator(
       covariant RelationalPattern node, DartType matchedValueType) {
     Name operatorName;
-    bool isEquality = false;
+    RelationalOperatorKind kind = RelationalOperatorKind.other;
     switch (node.kind) {
       case RelationalPatternKind.equals:
+        operatorName = equalsName;
+        kind = RelationalOperatorKind.equals;
+        break;
       case RelationalPatternKind.notEquals:
         operatorName = equalsName;
-        isEquality = true;
+        kind = RelationalOperatorKind.notEquals;
         break;
       case RelationalPatternKind.lessThan:
         operatorName = lessThanName;
@@ -10161,9 +10164,7 @@
     assert(!binaryTarget.isSpecialCasedBinaryOperator(this));
 
     return new RelationalOperatorResolution(
-        isEquality: isEquality,
-        parameterType: parameterType,
-        returnType: returnType);
+        kind: kind, parameterType: parameterType, returnType: returnType);
   }
 }
 
diff --git a/pkg/front_end/test/spell_checking_list_code.txt b/pkg/front_end/test/spell_checking_list_code.txt
index b663501b..fbd28fd 100644
--- a/pkg/front_end/test/spell_checking_list_code.txt
+++ b/pkg/front_end/test/spell_checking_list_code.txt
@@ -1711,6 +1711,7 @@
 wiser
 wishes
 wn
+worst
 worthwhile
 worthy
 woven

To view, visit change 280900. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Gerrit-Change-Number: 280900

Gerrit-PatchSet: 2

Gerrit-Owner: Paul Berry <paul...@google.com>

Gerrit-Reviewer: Johnni Winther <johnni...@google.com>

Gerrit-Reviewer: Paul Berry <paul...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Johnni Winther <johnni...@google.com>

Gerrit-MessageType: newchange

[Paul Berry (Gerrit)]

Attention is currently required from: Johnni Winther.

View Change

To view, visit change 280900. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Gerrit-Change-Number: 280900

Gerrit-PatchSet: 2

Gerrit-Owner: Paul Berry <paul...@google.com>

Gerrit-Reviewer: Johnni Winther <johnni...@google.com>

Gerrit-Reviewer: Paul Berry <paul...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Johnni Winther <johnni...@google.com>

Gerrit-Comment-Date: Mon, 06 Feb 2023 17:04:47 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

Gerrit-MessageType: comment

[Johnni Winther (Gerrit)]

Attention is currently required from: Paul Berry.

Patch set 2:Code-Review +1

View Change

1 comment:

• File pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart:

  • Patch Set #2, Line 3936: void equalityRelationalPattern_end(

    Instead of calling this from `constantPattern_end`, create helper method and call it from both `constantPattern_end` and `equalityRelationalPattern_end`. The former is not a case of the latter - they just happen to have a sharable implementation.

To view, visit change 280900. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Gerrit-Change-Number: 280900

Gerrit-PatchSet: 2

Gerrit-Owner: Paul Berry <paul...@google.com>

Gerrit-Reviewer: Johnni Winther <johnni...@google.com>

Gerrit-Reviewer: Paul Berry <paul...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Paul Berry <paul...@google.com>

Gerrit-Comment-Date: Mon, 06 Feb 2023 18:58:09 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Gerrit-MessageType: comment

[Paul Berry (Gerrit)]

Patch set 4:Commit-Queue +1

View Change

1 comment:

• File pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart:

• • Patch Set #2, Line 3936: void equalityRelationalPattern_end(

• • Instead of calling this from `constantPattern_end`, create helper method and call it from both `cons […]

    Done

To view, visit change 280900. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Gerrit-Change-Number: 280900

Gerrit-PatchSet: 4

Gerrit-Owner: Paul Berry <paul...@google.com>

Gerrit-Reviewer: Johnni Winther <johnni...@google.com>

Gerrit-Reviewer: Paul Berry <paul...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Mon, 06 Feb 2023 23:09:05 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Johnni Winther <johnni...@google.com>

Gerrit-MessageType: comment

[CBuild (Gerrit)]

go/dart-cbuild result: SUCCESS

Details: https://goto.google.com/dart-cbuild/find/1b6e76320b7e8091e4aa348e8c8418c502b8c4f9

View Change

To view, visit change 280900. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Gerrit-Change-Number: 280900

Gerrit-PatchSet: 4

Gerrit-Owner: Paul Berry <paul...@google.com>

Gerrit-Reviewer: Johnni Winther <johnni...@google.com>

Gerrit-Reviewer: Paul Berry <paul...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Mon, 06 Feb 2023 23:44:40 +0000

[Paul Berry (Gerrit)]

Attention is currently required from: Paul Berry.

Patch set 6:Commit-Queue +2

View Change

To view, visit change 280900. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Gerrit-Change-Number: 280900

Gerrit-PatchSet: 6

Gerrit-Owner: Paul Berry <paul...@google.com>

Gerrit-Reviewer: Johnni Winther <johnni...@google.com>

Gerrit-Reviewer: Paul Berry <paul...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Paul Berry <paul...@google.com>

Gerrit-Comment-Date: Tue, 07 Feb 2023 20:25:23 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

Gerrit-MessageType: comment

[Commit Queue (Gerrit)]

Commit Queue submitted this change.

View Change

2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
Insertions: 77, Deletions: 70.

The diff is too large to show. Please review the diff.
```

Approvals: Johnni Winther: Looks good to me, approved Paul Berry: Commit

Flow analysis: add support for relational patterns.

There are two new API methods: `equalityRelationalPattern_end` (for
relational patterns using `==` or `!=`, where some degree of flow
analysis is warranted) and `nonEqualityRelationalPattern_end` (for all
other relational patterns, where all we care about is making sure that
both the "matched" and "unmatched" branches are reachable).

I've moved most of the logic for constant patterns into
`equalityRelationalPattern_end`, since it's essentially the same logic
(except that `equalityRelationalPattern_end` also has support for
`!=`).  So now, `constantPattern_end` simply calls
`equalityRelationalPattern_end` in the case where pattern support is
enabled.

I also had to make a change to `RelationalOperatorResolution` to make
it possible to distinguish `==` from `!=`.

Bug: https://github.com/dart-lang/sdk/issues/50419
Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280900
Reviewed-by: Johnni Winther <johnni...@google.com>
Commit-Queue: Paul Berry <paul...@google.com>

---
M pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
M pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart
M pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
M pkg/_fe_analyzer_shared/test/mini_ast.dart
M pkg/analyzer/lib/src/generated/resolver.dart
M pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart
M pkg/front_end/test/spell_checking_list_code.txt

7 files changed, 391 insertions(+), 68 deletions(-)

diff --git a/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart b/pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
index 29db18c..71b66ac 100644

@@ -1202,6 +1213,16 @@

   }
 
   @override
+  void equalityRelationalPattern_end(Expression operand, Type operandType,
+      {bool notEqual = false}) {
+    _wrap(
+        'equalityRelationalPattern_end($operand, $operandType, '
+        'notEqual: $notEqual)',
+        () => _wrapped.equalityRelationalPattern_end(operand, operandType,
+            notEqual: notEqual));
+  }
+
+  @override
   ExpressionInfo<Type>? expressionInfoForTesting(Expression target) {
     return _wrap('expressionInfoForTesting($target)',
         () => _wrapped.expressionInfoForTesting(target),

@@ -1453,6 +1474,12 @@

   }
 
   @override
+  void nonEqualityRelationalPattern_end() {
+    _wrap('nonEqualityRelationalPattern_end()',
+        () => _wrapped.nonEqualityRelationalPattern_end());
+  }
+
+  @override
   void nonNullAssert_end(Expression operand) {
     return _wrap('nonNullAssert_end($operand)',
         () => _wrapped.nonNullAssert_end(operand));

@@ -3779,58 +3806,7 @@

+      _handleEqualityCheckPattern(expression, type, notEqual: false);

     } else {
       // Before pattern support was added to Dart, flow analysis didn't do any
       // promotion based on the constants in individual case clauses.  Also, it

@@ -3955,6 +3931,12 @@

   }
 
   @override
+  void equalityRelationalPattern_end(Expression operand, Type operandType,
+      {bool notEqual = false}) {

+    _handleEqualityCheckPattern(operand, operandType, notEqual: notEqual);

+  }
+
+  @override
   ExpressionInfo<Type>? expressionInfoForTesting(Expression target) =>
       identical(target, _expressionWithInfo) ? _expressionInfo : null;
 

@@ -4356,6 +4338,14 @@

   }
 
   @override
+  void nonEqualityRelationalPattern_end() {
+    // Flow analysis has no way of knowing whether the operator will return
+    // `true` or `false`, so just assume the worst case--both cases are
+    // reachable and no promotions can be done in either case.
+    _unmatched = _join(_unmatched!, _current);
+  }
+
+  @override
   void nonNullAssert_end(Expression operand) {
     ReferenceWithType<Type>? operandReference =
         _getExpressionReference(operand);

@@ -4952,6 +4942,81 @@
     return () => {};
   }
 
+  /// Common code for handling patterns that perform an equality check.
+  /// [operand] is the expression that the matched value is being compared to,
+  /// and [operandType] is its type.
+  ///
+  /// If [notEqual] is `true`, the pattern matches if the matched value is *not*
+  /// equal to the operand; otherwise, it matches if the matched value is
+  /// *equal* to the operand.
+  void _handleEqualityCheckPattern(Expression operand, Type operandType,
+      {required bool notEqual}) {

   Type? _handleProperty(Expression? wholeExpression, Expression? target,
       String propertyName, Object? propertyMember, Type staticType) {
     int targetKey;
@@ -5450,6 +5515,10 @@

       {bool notEqual = false}) {}
 
   @override
+  void equalityRelationalPattern_end(Expression operand, Type operandType,
+      {bool notEqual = false}) {}
+
+  @override
   ExpressionInfo<Type>? expressionInfoForTesting(Expression target) {
     throw new StateError(
         'expressionInfoForTesting requires null-aware flow analysis');

@@ -5702,6 +5771,9 @@

   void logicalOrPattern_end() {}
 
   @override
+  void nonEqualityRelationalPattern_end() {}
+
+  @override
   void nonNullAssert_end(Expression operand) {}
 
   @override
diff --git a/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart b/pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer.dart

index 32b6e0a..182f919 100644

@@ -1393,11 +1405,27 @@

index c347523..0c4f9c7 100644
--- a/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
+++ b/pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart
@@ -7204,6 +7204,186 @@

index 86f0f98..03184e0 100644

--- a/pkg/_fe_analyzer_shared/test/mini_ast.dart
+++ b/pkg/_fe_analyzer_shared/test/mini_ast.dart
@@ -741,7 +741,9 @@
       Type matchedValueType, String operator) {
     if (operator == '==' || operator == '!=') {
       return RelationalOperatorResolution(
-          isEquality: true,
+          kind: operator == '=='
+              ? RelationalOperatorKind.equals
+              : RelationalOperatorKind.notEquals,
           parameterType: Type('Object'),
           returnType: Type('bool'));
     }
@@ -757,7 +759,7 @@
           'must accept a parameter');
     }
     return RelationalOperatorResolution(
-        isEquality: operator == '==' || operator == '!=',
+        kind: RelationalOperatorKind.other,
         parameterType: memberType.positionalParameters[0],
         returnType: memberType.returnType);
   }
diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart

index a1578d1..d3ca15c 100644

index 7670c71..ffd5785 100644
--- a/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart
+++ b/pkg/front_end/lib/src/fasta/type_inference/inference_visitor.dart
@@ -10167,12 +10167,15 @@

   RelationalOperatorResolution<DartType>? resolveRelationalPatternOperator(
       covariant RelationalPattern node, DartType matchedValueType) {
     Name operatorName;
-    bool isEquality = false;
+    RelationalOperatorKind kind = RelationalOperatorKind.other;
     switch (node.kind) {
       case RelationalPatternKind.equals:
+        operatorName = equalsName;
+        kind = RelationalOperatorKind.equals;
+        break;
       case RelationalPatternKind.notEquals:
         operatorName = equalsName;
-        isEquality = true;
+        kind = RelationalOperatorKind.notEquals;
         break;
       case RelationalPatternKind.lessThan:
         operatorName = lessThanName;

@@ -10197,9 +10200,7 @@

     assert(!binaryTarget.isSpecialCasedBinaryOperator(this));
 
     return new RelationalOperatorResolution(
-        isEquality: isEquality,
-        parameterType: parameterType,
-        returnType: returnType);
+        kind: kind, parameterType: parameterType, returnType: returnType);
   }
 }
 
diff --git a/pkg/front_end/test/spell_checking_list_code.txt b/pkg/front_end/test/spell_checking_list_code.txt
index b663501b..fbd28fd 100644
--- a/pkg/front_end/test/spell_checking_list_code.txt
+++ b/pkg/front_end/test/spell_checking_list_code.txt
@@ -1711,6 +1711,7 @@
 wiser
 wishes
 wn
+worst
 worthwhile
 worthy
 woven

To view, visit change 280900. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Gerrit-Change-Number: 280900

Gerrit-PatchSet: 7

Gerrit-Owner: Paul Berry <paul...@google.com>

Gerrit-Reviewer: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Johnni Winther <johnni...@google.com>

Gerrit-Reviewer: Paul Berry <paul...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-MessageType: merged

[CBuild (Gerrit)]

go/dart-cbuild result: SUCCESS

Details: https://goto.google.com/dart-cbuild/find/832ae6722d5b61e8d4335686f0d18089f8f8bbba

View Change

To view, visit change 280900. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Idc5dbcfb02e3f8b57cfcccb3f46364fe34268ded

Gerrit-Change-Number: 280900

Gerrit-PatchSet: 7

Gerrit-Owner: Paul Berry <paul...@google.com>

Gerrit-Reviewer: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Johnni Winther <johnni...@google.com>

Gerrit-Reviewer: Paul Berry <paul...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-Comment-Date: Tue, 07 Feb 2023 21:43:55 +0000