dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

vm_service: Tidy nullability and finality in generator

Browse Source 
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 <srawlins@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
This commit is contained in:
Sam Rawlins
2025-06-23 11:29:41 -07:00
committed by Commit Queue
parent 22a74f5580
commit 2be548666d
3 changed files with 63 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/vm_service/tool/dart/generate_dart_client.dart
+3 -3
View File
@@ -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
+59 -52
View File
@@ -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<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();
return StreamCategory._(name: name, events: events);
}
String? get name => _name;
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,7 +1152,7 @@ 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());
}
@@ -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<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));
}
}
}
pkg/vm_service/tool/dart/generate_dart_interface.dart
+1 -1
View File
@@ -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;