From 2be548666d99b5dbfcd558cb6d6a8076a25274e9 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Mon, 23 Jun 2025 11:29:41 -0700 Subject: [PATCH] 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`. * 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 Reviewed-by: Ben Konyi --- .../tool/dart/generate_dart_client.dart | 6 +- .../tool/dart/generate_dart_common.dart | 111 ++++++++++-------- .../tool/dart/generate_dart_interface.dart | 2 +- 3 files changed, 63 insertions(+), 56 deletions(-) diff --git a/pkg/vm_service/tool/dart/generate_dart_client.dart b/pkg/vm_service/tool/dart/generate_dart_client.dart index 0e928fef635..86386aaca01 100644 --- a/pkg/vm_service/tool/dart/generate_dart_client.dart +++ b/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 = {'); 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 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; diff --git a/pkg/vm_service/tool/dart/generate_dart_common.dart b/pkg/vm_service/tool/dart/generate_dart_common.dart index 2a873192b6b..be79d4c6656 100644 --- a/pkg/vm_service/tool/dart/generate_dart_common.dart +++ b/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,7 +14,7 @@ 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'; @@ -21,7 +22,7 @@ String? _coerceRefType(String? typeName) { 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 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 methods = []; List enums = []; - List types = []; + List types = []; List streamCategories = []; void parse(List 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? _events; + final String name; + final List 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 events = line.split(',').map((w) => w.trim()).toList(); + + return StreamCategory._(name: name, events: events); } - String? get name => _name; - - List? 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 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 types = []; + final List 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? 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 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,7 +1152,7 @@ class Enum extends Member { List enums = []; - Enum(this.name, String definition, [this.docs]) { + Enum(this.name, String definition, this.docs) { _parse(Tokenizer(definition).tokenize()); } @@ -1153,7 +1160,7 @@ class Enum extends Member { 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 map = {}; 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 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)); } } } diff --git a/pkg/vm_service/tool/dart/generate_dart_interface.dart b/pkg/vm_service/tool/dart/generate_dart_interface.dart index 75b3d837fa6..1defc343247 100644 --- a/pkg/vm_service/tool/dart/generate_dart_interface.dart +++ b/pkg/vm_service/tool/dart/generate_dart_interface.dart @@ -278,7 +278,7 @@ class VmServerConnection { 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;