vm_service: Tidy nullability and finality in generator

While exploring this code, I saw some easy wins:

* A number of local variables of the form `String? x = a.b.c;` can be
  made non-nullable as `.c` now returns  `String`; no tooling will warn
  the developer of this.
* A number of methods of the form `String? m() {...}` can be made non-
  nullable, as they only return non-null values; no tooling will warn
  the developer of this.
* A number of methods of the form `m(String? p) {...}` can instead
  accept non-nullable Strings, as all of the call sites pass non-null
  Strings.
* `_coerceRefType` accepted a `String?` parameter but immediately null-
  checked it's value. We can use the type system to enforce that this
  function requires non-null values.
* StreamCategory's `_name` and `_events` fields can be made non-nullable
  by using a little factory constructor; this enhancement reveals that
  the fields can be made final (analyzer reports this). Then we have
  unnecessary public getters that expose these final fields; the fields
  can instead be made public, and the getters removed.
* `Api.types` can be made a `List<Type>`.
* Some fields can be made final: `MemberType.types`, `TypeRef.name`,
  `TypeRef.nullable`, `MethodArg.type`, and `MethodArg.name`.
* Some fields can be made non-nullable: `TypeRef.name`,
  `MethodArg.name`, and `EnumValue.name`.

Change-Id: Ia36c1edc4686c6a14d76fd053d70da60b7b4aac6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/436420
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Ben Konyi <[email protected]>

Author: srawlins

pkg/vm_service/tool/dart/generate_dart_client.dart

@@ -533,7 +533,7 @@ void addTypeFactory(String name, Function factory) {
     gen.writeln();
     gen.writeln('final _typeFactories = <String, Function>{');
     for (var type in types) {
-      gen.writeln("'${type!.rawName}': ${type.name}.parse,");
+      gen.writeln("'${type.rawName}': ${type.name}.parse,");
     }
     gen.writeln('};');
     gen.writeln();
@@ -672,12 +672,12 @@ typedef VmServiceFactory<T extends VmService> = T Function({
     }
     gen.writeln();
     gen.writeln('// types');
-    types.where((t) => !t!.skip).forEach((t) => t!.generate(gen));
+    types.where((t) => !t.skip).forEach((t) => t.generate(gen));
   }
 
   void setDefaultValue(String typeName, String fieldName, String defaultValue) {
     types
-        .firstWhere((t) => t!.name == typeName)!
+        .firstWhere((t) => t.name == typeName)
         .fields
         .firstWhere((f) => f.name == fieldName)
         .defaultValue = defaultValue;

pkg/vm_service/tool/dart/generate_dart_common.dart

@@ -4,6 +4,7 @@
 
 library generate_vm_service_dart;
 
+import 'package:collection/collection.dart';
 import 'package:markdown/markdown.dart';
 
 import '../common/generate_common.dart';
@@ -13,15 +14,15 @@ import 'src_gen_dart.dart';
 
 export 'src_gen_dart.dart' show DartGenerator;
 
-String? _coerceRefType(String? typeName) {
+String _coerceRefType(String typeName) {
   if (typeName == 'Object') typeName = 'Obj';
   if (typeName == '@Object') typeName = 'ObjRef';
   if (typeName == 'Null') typeName = 'NullVal';
   if (typeName == '@Null') typeName = 'NullValRef';
   if (typeName == 'Function') typeName = 'Func';
   if (typeName == '@Function') typeName = 'FuncRef';
 
-  if (typeName!.startsWith('@')) typeName = '${typeName.substring(1)}Ref';
+  if (typeName.startsWith('@')) typeName = '${typeName.substring(1)}Ref';
 
   if (typeName == 'string') typeName = 'String';
   if (typeName == 'map') typeName = 'Map';
@@ -30,7 +31,7 @@ String? _coerceRefType(String? typeName) {
 }
 
 String typeRefListToString(List<TypeRef> types) =>
-    'const [${types.map((e) => "'${e.name!}'").join(',')}]';
+    'const [${types.map((e) => "'${e.name}'").join(',')}]';
 
 final registerServiceImpl = '''
 _serviceExtensionRegistry.registerExtension(params!['service'], this);
@@ -89,7 +90,7 @@ abstract class Api with ApiParseUtil {
   String? serviceVersion;
   List<Method> methods = [];
   List<Enum> enums = [];
-  List<Type?> types = [];
+  List<Type> types = [];
   List<StreamCategory> streamCategories = [];
 
   void parse(List<Node> nodes) {
@@ -132,8 +133,8 @@ abstract class Api with ApiParseUtil {
       }
     }
 
-    for (Type? type in types) {
-      type!.calculateFieldOverrides();
+    for (Type type in types) {
+      type.calculateFieldOverrides();
     }
 
     Method streamListenMethod =
@@ -190,32 +191,30 @@ abstract class Api with ApiParseUtil {
 
   void generate(DartGenerator gen);
 
-  bool isEnumName(String? typeName) =>
-      enums.any((Enum e) => e.name == typeName);
+  bool isEnumName(String typeName) => enums.any((Enum e) => e.name == typeName);
 
-  Type? getType(String? name) =>
-      types.firstWhere((t) => t!.name == name, orElse: () => null);
+  Type? getType(String name) => types.firstWhereOrNull((t) => t.name == name);
 }
 
 class StreamCategory {
-  String? _name;
-  List<String>? _events;
+  final String name;
+  final List<String> events;
 
-  StreamCategory(String line) {
+  factory StreamCategory(String line) {
     // Debug | PauseStart, PauseExit, ...
-    _name = line.split('|')[0].trim();
+    String name = line.split('|')[0].trim();
 
     line = line.split('|')[1];
-    _events = line.split(',').map((w) => w.trim()).toList();
-  }
+    List<String> events = line.split(',').map((w) => w.trim()).toList();
 
-  String? get name => _name;
+    return StreamCategory._(name: name, events: events);
+  }
 
-  List<String>? get events => _events;
+  StreamCategory._({required this.name, required this.events});
 
   void generate(DartGenerator gen) {
     gen.writeln();
-    gen.writeln('// ${events!.join(', ')}');
+    gen.writeln('// ${events.join(', ')}');
     gen.writeln(
         "Stream<Event> get on${name}Event => _getEventController('$name').stream;");
   }
@@ -258,7 +257,7 @@ class Method extends Member {
           .join());
 
       args.where((MethodArg a) => a.optional).forEach((MethodArg arg) {
-        String? valueRef = arg.name;
+        String valueRef = arg.name;
         // Special case for `getAllocationProfile`. We do not want to add these
         // params if they are false.
         if (name == 'getAllocationProfile') {
@@ -332,7 +331,7 @@ class Method extends Member {
 
 class MemberType extends Member {
   final Api api;
-  List<TypeRef> types = [];
+  final List<TypeRef> types = [];
 
   MemberType(this.api);
 
@@ -353,14 +352,14 @@ class MemberType extends Member {
             // @Instance | Sentinel
             final token = parser.advance()!;
             if (token.isName) {
-              unionTypes.add(_coerceRefType(token.text)!);
+              unionTypes.add(_coerceRefType(token.text!));
             }
           }
         }
         parser.consume(')');
         TypeRef ref;
         if (unionTypes.length == 1) {
-          ref = TypeRef(unionTypes.first)..nullable = nullable;
+          ref = TypeRef(unionTypes.first, nullable: nullable);
         } else {
           ref = TypeRef('dynamic');
         }
@@ -371,7 +370,7 @@ class MemberType extends Member {
         types.add(ref);
       } else {
         Token t = parser.expectName();
-        TypeRef ref = TypeRef(_coerceRefType(t.text));
+        TypeRef ref = TypeRef(_coerceRefType(t.text!));
         while (parser.consume('[')) {
           parser.expect(']');
           ref.arrayDepth++;
@@ -421,12 +420,12 @@ class MemberType extends Member {
 }
 
 class TypeRef {
-  String? name;
+  final String name;
   int arrayDepth = 0;
-  bool nullable = false;
+  final bool nullable;
   List<TypeRef>? genericTypes;
 
-  TypeRef(this.name);
+  TypeRef(this.name, {this.nullable = false});
 
   String get ref {
     if (arrayDepth == 2) {
@@ -436,7 +435,7 @@ class TypeRef {
     } else if (genericTypes != null) {
       return '$name<${genericTypes!.join(', ')}>';
     } else {
-      return name!.startsWith('_') ? name!.substring(1) : name!;
+      return name.startsWith('_') ? name.substring(1) : name;
     }
   }
 
@@ -450,7 +449,7 @@ class TypeRef {
     }
   }
 
-  String? get listTypeArg => arrayDepth == 2
+  String get listTypeArg => arrayDepth == 2
       ? 'List<$name${nullable ? "?" : ""}>'
       : '$name${nullable ? "?" : ""}';
 
@@ -480,9 +479,9 @@ class TypeRef {
 
 class MethodArg extends Member {
   final Method parent;
-  TypeRef type;
+  final TypeRef type;
   @override
-  String? name;
+  final String name;
   bool optional = false;
 
   MethodArg(this.parent, this.type, this.name);
@@ -539,7 +538,7 @@ class Type extends Member {
   bool get isResponse {
     if (superName == null) return false;
     if (name == 'Response' || superName == 'Response') return true;
-    return parent.getType(superName)!.isResponse;
+    return parent.getType(superName!)!.isResponse;
   }
 
   bool get isRef => name!.endsWith('Ref');
@@ -549,7 +548,7 @@ class Type extends Member {
     return superName == null ? false : getSuper()!.supportsIdentity;
   }
 
-  Type? getSuper() => superName == null ? null : api.getType(superName);
+  Type? getSuper() => superName == null ? null : api.getType(superName!);
 
   List<TypeField> getAllFields() {
     if (superName == null) return fields;
@@ -575,7 +574,7 @@ class Type extends Member {
     gen.write('class $name ');
     Type? superType;
     if (superName != null) {
-      superType = parent.getType(superName);
+      superType = parent.getType(superName!);
       gen.write('extends $superName ');
     }
     if (parent.getType('${name}Ref') != null) {
@@ -963,7 +962,7 @@ void _parseTokenPosTable() {
           TypeRef arrayType = type.types.first;
           if (arrayType.arrayDepth == 1) {
             String assertMethodName =
-                'assertListOf${arrayType.name!.substring(0, 1).toUpperCase()}${arrayType.name!.substring(1)}';
+                'assertListOf${arrayType.name.substring(0, 1).toUpperCase()}${arrayType.name.substring(1)}';
             gen.writeln('$assertMethodName(obj.${field.generatableName}!);');
           } else {
             gen.writeln(
@@ -977,7 +976,7 @@ void _parseTokenPosTable() {
             gen.writeln(
                 'if (obj.${field.generatableName} is vms.${typeRef.name}) {');
             String assertMethodName =
-                'assert${typeRef.name!.substring(0, 1).toUpperCase()}${typeRef.name!.substring(1)}';
+                'assert${typeRef.name.substring(0, 1).toUpperCase()}${typeRef.name.substring(1)}';
             gen.writeln('$assertMethodName(obj.${field.generatableName}!);');
           }
           gen.writeln('} else {');
@@ -1051,7 +1050,7 @@ class TypeField extends Member {
   void setOverrides() => overrides = true;
 
   @override
-  String? get docs {
+  String get docs {
     String str = _docs ?? '';
     if (type.isMultipleReturns) {
       str += '\n\n[$generatableName] can be one of '
@@ -1102,9 +1101,9 @@ class TypeField extends Member {
   @override
   void generate(DartGenerator gen) {
     Type? refType = parent.parent.getType('${parent.name}Ref');
-    var interfaceOverride = refType?.hasField(name) ?? false;
+    bool interfaceOverride = refType?.hasField(name) ?? false;
 
-    if (docs!.isNotEmpty) gen.writeDocs(docs!);
+    if (docs.isNotEmpty) gen.writeDocs(docs);
     if (optional) gen.write('@optional ');
     if (overrides || interfaceOverride) gen.write('@override ');
     // Special case where Instance extends Obj, but 'classRef' is not optional
@@ -1116,9 +1115,17 @@ class TypeField extends Member {
       gen.writeStatement('covariant late final String valueAsString;');
     } else */
     {
-      String? typeName =
+      String typeName =
           api.isEnumName(type.name) ? '/*${type.name}*/ String' : type.name;
       if (typeName != 'dynamic') {
+        // Since this package is used to interact with DWDS as well as the
+        // native VM service, we need to be extremely careful when making types
+        // non-nullable. DWDS isn't completely compliant with the service
+        // protocol specification, meaning that some fields aren't provided in
+        // responses even though they're not marked as optional. This has been a
+        // headache for years, but until we're able to make DWDS consistent with
+        // the native VM service responses and test package:vm_service against
+        // it, we need to be extremely defensive in our parsing logic.
         typeName = '$typeName?';
       }
       gen.writeStatement('$typeName $generatableName;');
@@ -1128,7 +1135,7 @@ class TypeField extends Member {
 
   void generateNamedParameter(DartGenerator gen, {bool fromParent = false}) {
     if (fromParent) {
-      String? typeName =
+      String typeName =
           api.isEnumName(type.name) ? '/*${type.name}*/ String' : type.name;
       gen.writeStatement('required $typeName $generatableName,');
     } else {
@@ -1145,15 +1152,15 @@ class Enum extends Member {
 
   List<EnumValue> enums = [];
 
-  Enum(this.name, String definition, [this.docs]) {
+  Enum(this.name, String definition, this.docs) {
     _parse(Tokenizer(definition).tokenize());
   }
 
   Enum._(this.name, this.docs);
 
   factory Enum.merge(Enum e1, Enum e2) {
     final String name = e1.name;
-    final String docs = [e1.docs, e2.docs].where((e) => e != null).join('\n');
+    final String docs = [e1.docs, e2.docs].nonNulls.join('\n');
     final Map<String?, EnumValue> map = <String?, EnumValue>{};
     for (EnumValue e in e2.enums.reversed) {
       map[e.name] = e;
@@ -1186,7 +1193,7 @@ class Enum extends Member {
   void generateAssert(DartGenerator gen) {
     gen.writeln('String assert$name(String obj) {');
     List<EnumValue> sorted = enums.toList()
-      ..sort((EnumValue e1, EnumValue e2) => e1.name!.compareTo(e2.name!));
+      ..sort((EnumValue e1, EnumValue e2) => e1.name.compareTo(e2.name));
     for (EnumValue value in sorted) {
       gen.writeln('  if (obj == "${value.name}") return obj;');
     }
@@ -1201,7 +1208,7 @@ class Enum extends Member {
 class EnumValue extends Member {
   final Enum parent;
   @override
-  final String? name;
+  final String name;
   @override
   final String? docs;
 
@@ -1257,7 +1264,7 @@ class TextOutputVisitor implements NodeVisitor {
 
   @override
   void visitText(Text text) {
-    String? t = text.text;
+    String t = text.text;
     if (_em) {
       t = _coerceRefType(t);
     } else if (_href) {
@@ -1323,7 +1330,7 @@ class MethodParser extends Parser {
 
     while (peek()!.text != ')') {
       Token type = expectName();
-      TypeRef ref = TypeRef(_coerceRefType(type.text));
+      TypeRef ref = TypeRef(_coerceRefType(type.text!));
       if (peek()!.text == '[') {
         while (consume('[')) {
           expect(']');
@@ -1335,14 +1342,14 @@ class MethodParser extends Parser {
         ref.genericTypes = [];
         while (peek()!.text != '>') {
           Token genericTypeName = expectName();
-          ref.genericTypes!.add(TypeRef(_coerceRefType(genericTypeName.text)));
+          ref.genericTypes!.add(TypeRef(_coerceRefType(genericTypeName.text!)));
           consume(',');
         }
         expect('>');
       }
 
       Token name = expectName();
-      MethodArg arg = MethodArg(method, ref, name.text);
+      MethodArg arg = MethodArg(method, ref, name.text!);
       if (consume('[')) {
         expect('optional');
         expect(']');
@@ -1374,10 +1381,10 @@ class TypeParser extends Parser {
 
     Token t = expectName();
     type.rawName = t.text;
-    type.name = _coerceRefType(type.rawName);
+    type.name = _coerceRefType(type.rawName!);
     if (consume('extends')) {
       t = expectName();
-      type.superName = _coerceRefType(t.text);
+      type.superName = _coerceRefType(t.text!);
     }
 
     expect('{');
@@ -1431,7 +1438,7 @@ class EnumParser extends Parser {
       t = expectName();
       consume(',');
 
-      e.enums.add(EnumValue(e, t.text, docs));
+      e.enums.add(EnumValue(e, t.text!, docs));
     }
   }
 }