[dart2js] Some DartTypeVisitor cleanup.

This CL deletes BaseDartTypeVisitor and makes all of the visit* methods
in DartTypeVisitor abstract. This forces subclasses of DartTypeVisitor
to provide definitions for all of these methods rather than relying on
some default.

The old code led to a series of bugs with the same root cause: when a
new kind of DartType was added (and a corresponding visit* method added
to DartTypeVisitor), not all concrete implementations of the visitor
were updated to handle the new DartType. This didn't produce static
errors because DartTypeVisitor provided default no-op implementations
for visit* methods. In some cases, this was the desired behavior anyway,
but in practice, any time a new DartType is added, we want our tools to
yell at us until we've validated that it's properly handled everywhere
(even if the proper handling turns out to be "do nothing").

This CL also updates Namer.getTypeRepresentationForConstant to use a
visitor pattern.

Fixes: https://github.com/dart-lang/sdk/issues/46589
Change-Id: I451b592ae1ce4afff40de913535798a62e17b8b6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206943
Reviewed-by: Joshua Litt <joshualitt@google.com>
Commit-Queue: Mayank Patke <fishythefish@google.com>

pkg/compiler/lib/src/elements/types.dart

@@ -911,86 +911,29 @@ abstract class DartTypeVisitor<R, A> {
 
   R visit(covariant DartType type, A argument) => type.accept(this, argument);
 
-  R visitLegacyType(covariant LegacyType type, A argument) => null;
+  R visitLegacyType(covariant LegacyType type, A argument);
 
-  R visitNullableType(covariant NullableType type, A argument) => null;
+  R visitNullableType(covariant NullableType type, A argument);
 
-  R visitNeverType(covariant NeverType type, A argument) => null;
+  R visitNeverType(covariant NeverType type, A argument);
 
-  R visitVoidType(covariant VoidType type, A argument) => null;
+  R visitVoidType(covariant VoidType type, A argument);
 
-  R visitTypeVariableType(covariant TypeVariableType type, A argument) => null;
+  R visitTypeVariableType(covariant TypeVariableType type, A argument);
 
-  R visitFunctionTypeVariable(
-          covariant FunctionTypeVariable type, A argument) =>
-      null;
+  R visitFunctionTypeVariable(covariant FunctionTypeVariable type, A argument);
 
-  R visitFunctionType(covariant FunctionType type, A argument) => null;
+  R visitFunctionType(covariant FunctionType type, A argument);
 
-  R visitInterfaceType(covariant InterfaceType type, A argument) => null;
+  R visitInterfaceType(covariant InterfaceType type, A argument);
 
-  R visitDynamicType(covariant DynamicType type, A argument) => null;
+  R visitDynamicType(covariant DynamicType type, A argument);
 
-  R visitErasedType(covariant ErasedType type, A argument) => null;
+  R visitErasedType(covariant ErasedType type, A argument);
 
-  R visitAnyType(covariant AnyType type, A argument) => null;
+  R visitAnyType(covariant AnyType type, A argument);
 
-  R visitFutureOrType(covariant FutureOrType type, A argument) => null;
-}
-
-abstract class BaseDartTypeVisitor<R, A> extends DartTypeVisitor<R, A> {
-  const BaseDartTypeVisitor();
-
-  R visitType(covariant DartType type, A argument);
-
-  @override
-  R visitLegacyType(covariant LegacyType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitNullableType(covariant NullableType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitNeverType(covariant NeverType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitVoidType(covariant VoidType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitTypeVariableType(covariant TypeVariableType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitFunctionTypeVariable(
-          covariant FunctionTypeVariable type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitFunctionType(covariant FunctionType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitInterfaceType(covariant InterfaceType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitDynamicType(covariant DynamicType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitErasedType(covariant ErasedType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitAnyType(covariant AnyType type, A argument) =>
-      visitType(type, argument);
-
-  @override
-  R visitFutureOrType(covariant FutureOrType type, A argument) =>
-      visitType(type, argument);
+  R visitFutureOrType(covariant FutureOrType type, A argument);
 }
 
 class _LegacyErasureVisitor extends DartTypeVisitor<DartType, Null> {

pkg/compiler/lib/src/js_backend/namer.dart

@@ -557,9 +557,12 @@ class Namer extends ModularNamer {
   /// key into maps.
   final Map<LibraryEntity, String> _libraryKeys = {};
 
+  _TypeConstantRepresentationVisitor _typeConstantRepresenter;
+
   Namer(this._closedWorld, this.fixedNames) {
     _literalGetterPrefix = new StringBackedName(fixedNames.getterPrefix);
     _literalSetterPrefix = new StringBackedName(fixedNames.setterPrefix);
+    _typeConstantRepresenter = _TypeConstantRepresentationVisitor(this);
   }
 
   JElementEnvironment get _elementEnvironment =>
@@ -1460,32 +1463,79 @@ class Namer extends ModularNamer {
     }
   }
 
-  String getTypeRepresentationForTypeConstant(DartType type) {
-    type = type.withoutNullability;
-    if (type is DynamicType) return "dynamic";
-    if (type is NeverType) return "Never";
-    if (type is FutureOrType) {
-      return "FutureOr<dynamic>";
-    }
-    if (type is FunctionType) {
-      // TODO(johnniwinther): Add naming scheme for function type literals.
-      // These currently only occur from kernel.
-      return '()->';
-    }
-    InterfaceType interface = type;
-    String name = uniqueNameForTypeConstantElement(
-        interface.element.library, interface.element);
+  String getTypeRepresentationForTypeConstant(DartType type) =>
+      _typeConstantRepresenter.visit(type, null);
+}
+
+class _TypeConstantRepresentationVisitor extends DartTypeVisitor<String, Null> {
+  final Namer _namer;
+
+  _TypeConstantRepresentationVisitor(this._namer);
+
+  String _represent(DartType type) => visit(type, null);
+
+  @override
+  String visitLegacyType(LegacyType type, _) => '${_represent(type.baseType)}*';
+
+  @override
+  String visitNullableType(NullableType type, _) =>
+      '${_represent(type.baseType)}?';
+
+  @override
+  String visitNeverType(NeverType type, _) => 'Never';
+
+  @override
+  String visitVoidType(VoidType type, _) => 'void';
+
+  @override
+  String visitTypeVariableType(TypeVariableType type, _) {
+    throw StateError('Unexpected TypeVariableType $type');
+  }
+
+  @override
+  String visitFunctionTypeVariable(FunctionTypeVariable type, _) {
+    throw StateError('Unexpected FunctionTypeVariable $type');
+  }
+
+  @override
+  String visitFunctionType(FunctionType type, _) {
+    // TODO(johnniwinther): Add naming scheme for function type literals.
+    // These currently only occur from kernel.
+    return '()->';
+  }
+
+  @override
+  String visitInterfaceType(InterfaceType type, _) {
+    String name = _namer.uniqueNameForTypeConstantElement(
+        type.element.library, type.element);
 
     // Type constants can currently only be raw types, so there is no point
     // adding ground-term type parameters, as they would just be 'dynamic'.
     // TODO(sra): Since the result string is used only in constructing constant
     // names, it would result in more readable names if the final string was a
     // legal JavaScript identifier.
-    if (interface.typeArguments.isEmpty) return name;
+    if (type.typeArguments.isEmpty) return name;
     String arguments =
-        new List.filled(interface.typeArguments.length, 'dynamic').join(', ');
+        new List.filled(type.typeArguments.length, 'dynamic').join(', ');
     return '$name<$arguments>';
   }
+
+  @override
+  String visitDynamicType(DynamicType type, _) => 'dynamic';
+
+  @override
+  String visitErasedType(ErasedType type, _) {
+    throw StateError('Unexpected ErasedType $type');
+  }
+
+  @override
+  String visitAnyType(AnyType type, _) {
+    throw StateError('Unexpected AnyType $type');
+  }
+
+  @override
+  String visitFutureOrType(FutureOrType type, _) =>
+      'FutureOr<${_represent(type.typeArgument)}>';
 }
 
 /// Returns a unique suffix for an intercepted accesses to [classes]. This is

pkg/compiler/lib/src/js_backend/runtime_types.dart

@@ -829,8 +829,23 @@ class ArgumentCollector extends DartTypeVisitor<void, void> {
   }
 
   @override
-  void visitFutureOrType(FutureOrType type, _) {
-    collect(type.typeArgument);
+  void visitNeverType(NeverType type, _) {}
+
+  @override
+  void visitVoidType(VoidType type, _) {}
+
+  @override
+  void visitTypeVariableType(TypeVariableType type, _) {}
+
+  @override
+  void visitFunctionTypeVariable(FunctionTypeVariable type, _) {}
+
+  @override
+  void visitFunctionType(FunctionType type, _) {
+    collect(type.returnType);
+    collectAll(type.parameterTypes);
+    collectAll(type.optionalParameterTypes);
+    collectAll(type.namedParameterTypes);
   }
 
   @override
@@ -840,11 +855,17 @@ class ArgumentCollector extends DartTypeVisitor<void, void> {
   }
 
   @override
-  void visitFunctionType(FunctionType type, _) {
-    collect(type.returnType);
-    collectAll(type.parameterTypes);
-    collectAll(type.optionalParameterTypes);
-    collectAll(type.namedParameterTypes);
+  void visitDynamicType(DynamicType type, _) {}
+
+  @override
+  void visitErasedType(ErasedType type, _) {}
+
+  @override
+  void visitAnyType(AnyType type, _) {}
+
+  @override
+  void visitFutureOrType(FutureOrType type, _) {
+    collect(type.typeArgument);
   }
 }
 
@@ -913,8 +934,10 @@ class TypeVisitor extends DartTypeVisitor<void, TypeVisitorState> {
       visitType(type.baseType, state);
 
   @override
-  void visitFutureOrType(FutureOrType type, TypeVisitorState state) =>
-      visitType(type.typeArgument, state);
+  void visitNeverType(NeverType type, TypeVisitorState state) {}
+
+  @override
+  void visitVoidType(VoidType type, TypeVisitorState state) {}
 
   @override
   void visitTypeVariableType(TypeVariableType type, TypeVisitorState state) {
@@ -924,15 +947,14 @@ class TypeVisitor extends DartTypeVisitor<void, TypeVisitorState> {
   }
 
   @override
-  visitInterfaceType(InterfaceType type, TypeVisitorState state) {
-    if (onClass != null) {
-      onClass(type.element, state: state);
+  visitFunctionTypeVariable(FunctionTypeVariable type, TypeVisitorState state) {
+    if (_visitedFunctionTypeVariables.add(type)) {
+      visitType(type.bound, state);
     }
-    visitTypes(type.typeArguments, covariantArgument(state));
   }
 
   @override
-  visitFunctionType(FunctionType type, TypeVisitorState state) {
+  void visitFunctionType(FunctionType type, TypeVisitorState state) {
     if (onFunctionType != null) {
       onFunctionType(type, state: state);
     }
@@ -946,11 +968,25 @@ class TypeVisitor extends DartTypeVisitor<void, TypeVisitorState> {
   }
 
   @override
-  visitFunctionTypeVariable(FunctionTypeVariable type, TypeVisitorState state) {
-    if (_visitedFunctionTypeVariables.add(type)) {
-      visitType(type.bound, state);
+  void visitInterfaceType(InterfaceType type, TypeVisitorState state) {
+    if (onClass != null) {
+      onClass(type.element, state: state);
     }
+    visitTypes(type.typeArguments, covariantArgument(state));
   }
+
+  @override
+  void visitDynamicType(DynamicType type, TypeVisitorState state) {}
+
+  @override
+  void visitErasedType(ErasedType type, TypeVisitorState state) {}
+
+  @override
+  void visitAnyType(AnyType type, TypeVisitorState state) {}
+
+  @override
+  void visitFutureOrType(FutureOrType type, TypeVisitorState state) =>
+      visitType(type.typeArgument, state);
 }
 
 /// Runtime type usage for a class.

pkg/compiler/test/rti/rti_need_test_helper.dart

@@ -187,23 +187,22 @@ abstract class ComputeValueMixin {
 }
 
 /// Visitor that determines whether a type refers to [entity].
-class FindTypeVisitor extends BaseDartTypeVisitor<bool, Null> {
+class FindTypeVisitor extends DartTypeVisitor<bool, Null> {
   final Entity entity;
 
   FindTypeVisitor(this.entity);
 
-  bool visitTypes(List<DartType> types) {
+  bool check(DartType type) => visit(type, null);
+
+  bool checkList(List<DartType> types) {
     for (DartType type in types) {
-      if (type.accept(this, null)) {
+      if (check(type)) {
         return true;
       }
     }
     return false;
   }
 
-  @override
-  bool visitType(DartType type, _) => false;
-
   @override
   bool visitLegacyType(LegacyType type, _) => visit(type.baseType, _);
 
@@ -211,30 +210,41 @@ class FindTypeVisitor extends BaseDartTypeVisitor<bool, Null> {
   bool visitNullableType(NullableType type, _) => visit(type.baseType, _);
 
   @override
-  bool visitInterfaceType(InterfaceType type, _) {
-    if (type.element == entity) return true;
-    return visitTypes(type.typeArguments);
-  }
+  bool visitNeverType(NeverType type, _) => false;
 
   @override
-  bool visitFunctionType(FunctionType type, _) {
-    if (type.returnType.accept(this, null)) return true;
-    if (visitTypes(type.typeVariables)) return true;
-    if (visitTypes(type.parameterTypes)) return true;
-    if (visitTypes(type.optionalParameterTypes)) return true;
-    if (visitTypes(type.namedParameterTypes)) return true;
-    return false;
-  }
+  bool visitVoidType(VoidType type, _) => false;
 
   @override
-  bool visitTypeVariableType(TypeVariableType type, _) {
-    return type.element.typeDeclaration == entity;
-  }
+  bool visitTypeVariableType(TypeVariableType type, _) =>
+      type.element.typeDeclaration == entity;
 
   @override
-  bool visitFutureOrType(FutureOrType type, _) {
-    return type.typeArgument.accept(this, null);
-  }
+  bool visitFunctionTypeVariable(FunctionTypeVariable type, _) => false;
+
+  @override
+  bool visitFunctionType(FunctionType type, _) =>
+      type.returnType.accept(this, null) ||
+      checkList(type.typeVariables) ||
+      checkList(type.parameterTypes) ||
+      checkList(type.optionalParameterTypes) ||
+      checkList(type.namedParameterTypes);
+
+  @override
+  bool visitInterfaceType(InterfaceType type, _) =>
+      type.element == entity || checkList(type.typeArguments);
+
+  @override
+  bool visitDynamicType(DynamicType type, _) => false;
+
+  @override
+  bool visitErasedType(ErasedType type, _) => false;
+
+  @override
+  bool visitAnyType(AnyType type, _) => false;
+
+  @override
+  bool visitFutureOrType(FutureOrType type, _) => check(type.typeArgument);
 }
 
 class RtiNeedDataComputer extends DataComputer<String> {

tests/web/regress/46589_test.dart

@@ -0,0 +1,16 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+
+typedef MyVoid = void;
+
+bool isVoid<X>() {
+  return X == MyVoid;
+}
+
+void main() {
+  Expect.isFalse(isVoid<int>());
+  Expect.isTrue(isVoid<void>());
+}

tests/web_2/regress/46589_test.dart

@@ -0,0 +1,16 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+
+typedef MyVoid = void;
+
+bool isVoid<X>() {
+  return X == MyVoid;
+}
+
+void main() {
+  Expect.isFalse(isVoid<int>());
+  Expect.isTrue(isVoid<void>());
+}