Make type variable constraint generation conventions uniform.

This CL changes the CFE's
TypeConstraintGatherer._isNullabilityAwareSubtypeMatch method so that
it is responsible for restoring the constraint state if there is no
match, making it consistent with the contraint gathering methods in
the analyzer and _fe_analyzer_shared.

This made it possible to remove much of the calls to state restoring
logic that _isNullabilityAwareSubtypeMatch previously had to do after
making recursive calls to itself, as well as a lot of state restoring
logic in _fe_analyzer_shared. It also made it possible to eliminate
_tryNullabilityAwareSubtypeMatch from the CFE (since
_isNullabilityAwareSubtypeMatch now has the same behavior).

Making this change now should hopefully simplify the remaining steps
in sharing type variable constraint generation logic, since it will no
longer be necessary to adjust state restoring logic when moving code
between the CFE and _fe_analyzer_shared.

I also took the liberty of rewriting some of the documentation
comments to try to clarify the new conventions.

In the process I also discovered several instances of unnecessary
state restoring logic in the analyzer; I'll make a separate CL to
clean those up (and adjust the analyzer documentation too).

Change-Id: If74c8be06f1d53f61d109e5ea2a8526d5cbcd347
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/388265
Commit-Queue: Chloe Stefantsova <[email protected]>
Reviewed-by: Chloe Stefantsova <[email protected]>
Auto-Submit: Paul Berry <[email protected]>

Author: stereotype441

pkg/_fe_analyzer_shared/lib/src/type_inference/type_analyzer_operations.dart

@@ -898,23 +898,18 @@ abstract class TypeConstraintGenerator<
   List<TypeStructure>? getTypeArgumentsAsInstanceOf(
       TypeDeclarationType type, TypeDeclaration typeDeclaration);
 
-  /// Matches [p] against [q] as a subtype against supertype and returns true if
-  /// [p] and [q] are both function types, and [p] is a subtype of [q] under
-  /// some constraints imposed on type parameters occurring in [q]; false if
-  /// both [p] and [q] are function types, but [p] can't possibly be a subtype
-  /// of [q] under any constraints; and null otherwise.
+  /// Matches [p] against [q].
+  ///
+  /// If [p] and [q] are both non-generic function types, and [p] is a subtype
+  /// of [q] under some constraints, the constraints making the relation
+  /// possible are recorded, and `true` is returned. Otherwise, the constraint
+  /// state is unchanged (or rolled back using [restoreState]), and `null` is
+  /// returned.
   ///
   /// An invariant of the type inference is that only [p] or [q] may be a
   /// schema (in other words, may contain the unknown type `_`); the other must
   /// be simply a type. If [leftSchema] is `true`, [p] may contain `_`; if it is
   /// `false`, [q] may contain `_`.
-  ///
-  /// As the generator computes the constraints making the relation possible, it
-  /// changes its internal state. The current state of the generator can be
-  /// obtained by [currentState], and the generator can be restored to a state
-  /// via [restoreState]. All of the shared constraint generation methods are
-  /// supposed to restore the generator to the prior state in case of a
-  /// mismatch, taking that responsibility away from the caller.
   bool? performSubtypeConstraintGenerationForFunctionTypes(
       TypeStructure p, TypeStructure q,
       {required bool leftSchema, required AstNode? astNodeForTesting}) {
@@ -943,7 +938,6 @@ abstract class TypeConstraintGenerator<
         if (!performSubtypeConstraintGenerationInternal(
             p.returnType, q.returnType,
             leftSchema: leftSchema, astNodeForTesting: astNodeForTesting)) {
-          restoreState(state);
           return null;
         }
         for (int i = 0; i < q.positionalParameterTypes.length; ++i) {
@@ -973,7 +967,6 @@ abstract class TypeConstraintGenerator<
         if (!performSubtypeConstraintGenerationInternal(
             p.returnType, q.returnType,
             leftSchema: leftSchema, astNodeForTesting: astNodeForTesting)) {
-          restoreState(state);
           return null;
         }
         for (int i = 0; i < p.positionalParameterTypes.length; ++i) {
@@ -1048,23 +1041,18 @@ abstract class TypeConstraintGenerator<
     return null;
   }
 
-  /// Matches [p] against [q] as a subtype against supertype and returns true if
-  /// [p] and [q] are both FutureOr, with or without nullability suffixes as
-  /// defined by [enableDiscrepantObliviousnessOfNullabilitySuffixOfFutureOr],
-  /// and [p] is a subtype of [q] under some constraints imposed on type
-  /// parameters occurring in [q], and false otherwise.
+  /// Matches [p] against [q].
+  ///
+  /// If [q] is of the form `FutureOr<q0>` for some `q0`, and [p] is a subtype
+  /// of [q] under some constraints, the constraints making the relation
+  /// possible are recorded, and `true` is returned. Otherwise, the constraint
+  /// state is unchanged (or rolled back using [restoreState]), and `false` is
+  /// returned.
   ///
   /// An invariant of the type inference is that only [p] or [q] may be a
   /// schema (in other words, may contain the unknown type `_`); the other must
   /// be simply a type. If [leftSchema] is `true`, [p] may contain `_`; if it is
   /// `false`, [q] may contain `_`.
-  ///
-  /// As the generator computes the constraints making the relation possible,
-  /// it changes its internal state. The current state of the generator can be
-  /// obtained by [currentState], and the generator can be restored to a state
-  /// via [restoreState]. All of the shared constraint generation methods are
-  /// supposed to restore the generator to the prior state in case of a
-  /// mismatch, taking that responsibility away from the caller.
   bool performSubtypeConstraintGenerationForFutureOr(
       TypeStructure p, TypeStructure q,
       {required bool leftSchema, required AstNode? astNodeForTesting}) {
@@ -1083,7 +1071,6 @@ abstract class TypeConstraintGenerator<
             leftSchema: leftSchema, astNodeForTesting: astNodeForTesting)) {
           return true;
         }
-        restoreState(state);
       }
 
       // Or if `P` is a subtype match for `Future<Q0>` under non-empty
@@ -1095,14 +1082,12 @@ abstract class TypeConstraintGenerator<
       if (isMatchWithFuture && matchWithFutureAddsConstraints) {
         return true;
       }
-      restoreState(state);
 
       // Or if `P` is a subtype match for `Q0` under constraint set `C`.
       if (performSubtypeConstraintGenerationInternal(p, q0,
           leftSchema: leftSchema, astNodeForTesting: astNodeForTesting)) {
         return true;
       }
-      restoreState(state);
 
       // Or if `P` is a subtype match for `Future<Q0>` under empty
       // constraint set `C`.
@@ -1114,24 +1099,22 @@ abstract class TypeConstraintGenerator<
     return false;
   }
 
-  /// Matches [p] against [q] as a subtype against supertype and returns true if
-  /// [p] and [q] are both type declaration types as defined by the enum
-  /// [TypeDeclarationKind], and [p] is a subtype of [q] under some constraints
-  /// imposed on type parameters occurring in [q]; false if both [p] and [q] are
-  /// type declaration types, but [p] can't possibly be a subtype of [q] under
-  /// any constraints; and null otherwise.
+  /// Matches [p] against [q] as a subtype against supertype.
+  ///
+  /// If [p] and [q] are both type declaration types, then:
+  ///
+  /// - If [p] is a subtype of [q] under some constraints, the constraints
+  ///   making the relation possible are recorded, and `true` is returned.
+  /// - Otherwise, the constraint state is unchanged (or rolled back using
+  ///   [restoreState]), and `false` is returned.
+  ///
+  /// Otherwise (either [p] or [q] is not a type declaration type), the
+  /// constraint state is unchanged, and `null` is returned.
   ///
   /// An invariant of the type inference is that only [p] or [q] may be a
   /// schema (in other words, may contain the unknown type `_`); the other must
   /// be simply a type. If [leftSchema] is `true`, [p] may contain `_`; if it is
   /// `false`, [q] may contain `_`.
-  ///
-  /// As the generator computes the constraints making the relation possible, it
-  /// changes its internal state. The current state of the generator can be
-  /// obtained by [currentState], and the generator can be restored to a state
-  /// via [restoreState]. All of the shared constraint generation methods are
-  /// supposed to restore the generator to the prior state in case of a
-  /// mismatch, taking that responsibility away from the caller.
   bool? performSubtypeConstraintGenerationForTypeDeclarationTypes(
       TypeStructure p, TypeStructure q,
       {required bool leftSchema, required AstNode? astNodeForTesting}) {
@@ -1174,6 +1157,14 @@ abstract class TypeConstraintGenerator<
     }
   }
 
+  /// Implementation backing [performSubtypeConstraintGenerationLeftSchema] and
+  /// [performSubtypeConstraintGenerationRightSchema].
+  ///
+  /// If [p] is a subtype of [q] under some constraints, the constraints making
+  /// the relation possible are recorded, and `true` is returned. Otherwise,
+  /// the constraint state is unchanged (or rolled back using [restoreState]),
+  /// and `false` is returned.
+  ///
   /// [performSubtypeConstraintGenerationInternal] should be implemented by
   /// concrete classes implementing [TypeConstraintGenerator]. The
   /// implementations of [performSubtypeConstraintGenerationLeftSchema] and
@@ -1201,9 +1192,8 @@ abstract class TypeConstraintGenerator<
   /// As the generator computes the constraints making the relation possible,
   /// it changes its internal state. The current state of the generator can be
   /// obtained by [currentState], and the generator can be restored to a state
-  /// via [restoreState]. All of the shared constraint generation methods are
-  /// supposed to restore the generator to the prior state in case of a
-  /// mismatch, taking that responsibility away from the caller.
+  /// via [restoreState]. If this method returns `false`, it restores the state,
+  /// so it is not necessary for the caller to do so.
   ///
   /// The algorithm for subtype constraint generation is described in
   /// https://github.com/dart-lang/language/blob/main/resources/type-system/inference.md#subtype-constraint-generation
@@ -1219,9 +1209,8 @@ abstract class TypeConstraintGenerator<
   /// As the generator computes the constraints making the relation possible,
   /// it changes its internal state. The current state of the generator can be
   /// obtained by [currentState], and the generator can be restored to a state
-  /// via [restoreState]. All of the shared constraint generation methods are
-  /// supposed to restore the generator to the prior state in case of a
-  /// mismatch, taking that responsibility away from the caller.
+  /// via [restoreState]. If this method returns `false`, it restores the state,
+  /// so it is not necessary for the caller to do so.
   ///
   /// The algorithm for subtype constraint generation is described in
   /// https://github.com/dart-lang/language/blob/main/resources/type-system/inference.md#subtype-constraint-generation
@@ -1271,6 +1260,13 @@ abstract class TypeConstraintGenerator<
     return true;
   }
 
+  /// Matches [p] against [q], assuming both [p] and [q] are both type
+  /// declaration types that refer to different type declarations.
+  ///
+  /// If [p] is a subtype of [q] under some constraints, the constraints making
+  /// the relation possible are recorded, and `true` is returned. Otherwise,
+  /// the constraint state is unchanged (or rolled back using [restoreState]),
+  /// and `false` is returned.
   bool _interfaceTypes(TypeStructure p, TypeStructure q, bool leftSchema,
       {required AstNode? astNodeForTesting}) {
     if (p.nullabilitySuffix != NullabilitySuffix.none) {

pkg/front_end/lib/src/type_inference/type_constraint_gatherer.dart

@@ -123,7 +123,7 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
   /// a subtype of [type] under any set of constraints.
   bool tryConstrainLower(DartType type, DartType bound,
       {required TreeNode? treeNodeForTesting}) {
-    return _tryNullabilityAwareSubtypeMatch(bound, type,
+    return _isNullabilityAwareSubtypeMatch(bound, type,
         constrainSupertype: true, treeNodeForTesting: treeNodeForTesting);
   }
 
@@ -133,7 +133,7 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
   /// a subtype of [bound] under any set of constraints.
   bool tryConstrainUpper(DartType type, DartType bound,
       {required TreeNode? treeNodeForTesting}) {
-    return _tryNullabilityAwareSubtypeMatch(type, bound,
+    return _isNullabilityAwareSubtypeMatch(type, bound,
         constrainSupertype: false, treeNodeForTesting: treeNodeForTesting);
   }
 
@@ -156,34 +156,6 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
     return isMatch;
   }
 
-  /// Tries to match [subtype] against [supertype].
-  ///
-  /// If the match succeeds, the member returns true, and the resulting type
-  /// constraints are recorded for later use by [computeConstraints].  If the
-  /// match fails, the member returns false, and the set of type constraints is
-  /// unchanged.
-  ///
-  /// In contrast with [_tryNullabilityObliviousSubtypeMatch], this method
-  /// distinguishes between cases when the type parameters to constraint occur
-  /// in [subtype] and in [supertype].  If [constrainSupertype] is true, the
-  /// type parameters to constrain occur in [supertype]; otherwise, they occur
-  /// in [subtype].  If one type contains the type parameters to constrain, the
-  /// other one isn't allowed to contain them.  The type that contains the type
-  /// parameters isn't allowed to also contain [UnknownType], that is, to be a
-  /// type schema.
-  bool _tryNullabilityAwareSubtypeMatch(DartType subtype, DartType supertype,
-      {required bool constrainSupertype,
-      required TreeNode? treeNodeForTesting}) {
-    int baseConstraintCount = _protoConstraints.length;
-    bool isMatch = _isNullabilityAwareSubtypeMatch(subtype, supertype,
-        constrainSupertype: constrainSupertype,
-        treeNodeForTesting: treeNodeForTesting);
-    if (!isMatch) {
-      _protoConstraints.length = baseConstraintCount;
-    }
-    return isMatch;
-  }
-
   /// Add constraint: [lower] <: [parameter] <: TOP.
   void _constrainParameterLower(StructuralParameter parameter, DartType lower,
       {required TreeNode? treeNodeForTesting}) {
@@ -367,10 +339,10 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
 
   /// Matches [p] against [q] as a subtype against supertype.
   ///
-  /// Returns true if [p] is a subtype of [q] under some constraints, and false
-  /// otherwise.  The constraints making the relation possible are recorded to
-  /// [_protoConstraints].  It is the responsibility of the caller to cleanup
-  /// [_protoConstraints] in case [p] can't be a subtype of [q].
+  /// If [p] is a subtype of [q] under some constraints, the constraints making
+  /// the relation possible are recorded to [_protoConstraints], and `true` is
+  /// returned. Otherwise, [_protoConstraints] is left unchanged (or rolled
+  /// back), and `false` is returned.
   ///
   /// If [constrainSupertype] is true, the type parameters to constrain occur in
   /// [supertype]; otherwise, they occur in [subtype].  If one type contains the
@@ -505,7 +477,6 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
               treeNodeForTesting: treeNodeForTesting)) {
         return true;
       }
-      _protoConstraints.length = baseConstraintCount;
 
       if ((p is SharedDynamicTypeStructure || p is SharedVoidTypeStructure) &&
           _isNullabilityAwareSubtypeMatch(
@@ -514,7 +485,6 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
               treeNodeForTesting: treeNodeForTesting)) {
         return true;
       }
-      _protoConstraints.length = baseConstraintCount;
 
       bool isMatchWithRawQ = _isNullabilityAwareSubtypeMatch(p, rawQ,
           constrainSupertype: constrainSupertype,
@@ -524,20 +494,17 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
       if (isMatchWithRawQ && matchWithRawQAddsConstraints) {
         return true;
       }
-      _protoConstraints.length = baseConstraintCount;
 
       if (_isNullabilityAwareSubtypeMatch(
           p, typeOperations.nullType.unwrapTypeView(),
           constrainSupertype: constrainSupertype,
           treeNodeForTesting: treeNodeForTesting)) {
         return true;
       }
-      _protoConstraints.length = baseConstraintCount;
 
       if (isMatchWithRawQ && !matchWithRawQAddsConstraints) {
         return true;
       }
-      _protoConstraints.length = baseConstraintCount;
     }
 
     // If P is FutureOr<P0> the match holds under constraint set C1 + C2:
@@ -616,22 +583,18 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
     // already eliminated the case that X is a variable in L.
     if (p is TypeParameterType) {
       // Coverage-ignore-block(suite): Not run.
-      final int baseConstraintCount = _protoConstraints.length;
       if (_isNullabilityAwareSubtypeMatch(p.bound, q,
           constrainSupertype: constrainSupertype,
           treeNodeForTesting: treeNodeForTesting)) {
         return true;
       }
-      _protoConstraints.length = baseConstraintCount;
     } else if (p is StructuralParameterType) {
       // Coverage-ignore-block(suite): Not run.
-      final int baseConstraintCount = _protoConstraints.length;
       if (_isNullabilityAwareSubtypeMatch(p.bound, q,
           constrainSupertype: constrainSupertype,
           treeNodeForTesting: treeNodeForTesting)) {
         return true;
       }
-      _protoConstraints.length = baseConstraintCount;
     }
 
     bool? constraintGenerationResult =
@@ -774,6 +737,7 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
         }
       }
       if (sameNames) {
+        final int baseConstraintCount = _protoConstraints.length;
         bool isMatch = true;
         for (int i = 0; isMatch && i < p.positional.length; i++) {
           isMatch = isMatch &&
@@ -788,6 +752,8 @@ class TypeConstraintGatherer extends shared.TypeConstraintGenerator<
                   treeNodeForTesting: treeNodeForTesting);
         }
         if (isMatch) return true;
+        // Coverage-ignore(suite): Not run.
+        _protoConstraints.length = baseConstraintCount;
       }
     }
 

pkg/front_end/test/spell_checking_list_code.txt

@@ -121,6 +121,7 @@ awaited
 awaiting
 awaits
 b
+backing
 backlog
 backping
 backstop