Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dynamic calls #579

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions protobuf/analysis_options.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ include: package:lints/recommended.yaml

linter:
rules:
- avoid_dynamic_calls
- comment_references
- directives_ordering
- prefer_relative_imports
Expand Down
4 changes: 4 additions & 0 deletions protobuf/benchmarks/common.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
/// both on the VM and when compiled to JavaScript.
library common;

// TODO(https://github.com/google/protobuf.dart/issues/578): Remove remaining
// dynamic calls.
// ignore_for_file: avoid_dynamic_calls

import 'dart:typed_data';

import 'package:benchmark_harness/benchmark_harness.dart';
Expand Down
11 changes: 8 additions & 3 deletions protobuf/lib/src/protobuf/coded_buffer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ void _mergeFromCodedBufferReader(BuilderInfo meta, _FieldSet fs,
}

void _readPackable(BuilderInfo meta, _FieldSet fs, CodedBufferReader input,
int wireType, FieldInfo fi, Function readFunc) {
int wireType, FieldInfo fi, Object Function() readFunc) {
void readToList(List list) => list.add(readFunc());
_readPackableToList(meta, fs, input, wireType, fi, readToList);
}
Expand All @@ -222,8 +222,13 @@ void _readPackableToListEnum(
_readPackableToList(meta, fs, input, wireType, fi, readToList);
}

void _readPackableToList(BuilderInfo meta, _FieldSet fs,
CodedBufferReader input, int wireType, FieldInfo fi, Function readToList) {
void _readPackableToList(
BuilderInfo meta,
_FieldSet fs,
CodedBufferReader input,
int wireType,
FieldInfo fi,
void Function(List<Object?>) readToList) {
var list = fs._ensureRepeatedField(meta, fi);

if (wireType == WIRETYPE_LENGTH_DELIMITED) {
Expand Down
2 changes: 1 addition & 1 deletion protobuf/lib/src/protobuf/coded_buffer_reader.dart
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class CodedBufferReader {

bool isAtEnd() => _bufferPos >= _currentLimit;

void _withLimit(int byteLimit, callback) {
void _withLimit(int byteLimit, void Function() callback) {
if (byteLimit < 0) {
throw ArgumentError(
'CodedBufferReader encountered an embedded string or message'
Expand Down
26 changes: 16 additions & 10 deletions protobuf/lib/src/protobuf/coded_buffer_writer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,12 @@ class CodedBufferWriter {
_commitChunk(true);
}

void writeField(int fieldNumber, int fieldType, fieldValue) {
void writeField(int fieldNumber, int fieldType, Object? fieldValue) {
final valueType = fieldType & ~0x07;

if ((fieldType & PbFieldType._PACKED_BIT) != 0) {
if (!fieldValue.isEmpty) {
fieldValue as List<Int64>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write this code as

final listValue = fieldValue as List<Object?>;

and then use listValue? I find such code easier to read. This applies to all other cases.

(also I doubt it's always List<Int64>, though I am not sure).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and switched to List<Object?>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the new fieldValue as List<Object?>; pattern enabled by flow analysis / type promotion :) it feels strange to convert fieldValue as List<Object?>; to final listValue = fieldValue as List<Object?>;, because fieldValue is still promoted in such an assignment, so no errors pop up. The subsequent uses of fieldValue as a List<Object?> continue to work, and I must hunt down each reference to fieldValue, in the scope of the cast, and change it to refer to listValue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW using a variable also allows to satisfy dart2js constraints, you could write

final List<Object?> listValue = fieldValue;

to promote at 0 cost for dart2js.

if (fieldValue.isNotEmpty) {
_writeTag(fieldNumber, WIRETYPE_LENGTH_DELIMITED);
final mark = _startLengthDelimited();
for (var value in fieldValue) {
Expand All @@ -82,24 +83,26 @@ class CodedBufferWriter {
final wireFormat = _wireTypes[_valueTypeIndex(valueType)];

if ((fieldType & PbFieldType._MAP_BIT) != 0) {
fieldValue as PbMap<Object?, Object?>;
final keyWireFormat =
_wireTypes[_valueTypeIndex(fieldValue.keyFieldType)];
_wireTypes[_valueTypeIndex(fieldValue.keyFieldType!)];
final valueWireFormat =
_wireTypes[_valueTypeIndex(fieldValue.valueFieldType)];
_wireTypes[_valueTypeIndex(fieldValue.valueFieldType!)];

fieldValue.forEach((key, value) {
_writeTag(fieldNumber, WIRETYPE_LENGTH_DELIMITED);
final mark = _startLengthDelimited();
_writeValue(
PbMap._keyFieldNumber, fieldValue.keyFieldType, key, keyWireFormat);
_writeValue(PbMap._valueFieldNumber, fieldValue.valueFieldType, value,
_writeValue(PbMap._keyFieldNumber, fieldValue.keyFieldType!, key,
keyWireFormat);
_writeValue(PbMap._valueFieldNumber, fieldValue.valueFieldType!, value,
valueWireFormat);
_endLengthDelimited(mark);
});
return;
}

if ((fieldType & PbFieldType._REPEATED_BIT) != 0) {
fieldValue as List<Object?>;
for (var i = 0; i < fieldValue.length; i++) {
_writeValue(fieldNumber, valueType, fieldValue[i], wireFormat);
}
Expand Down Expand Up @@ -349,9 +352,11 @@ class CodedBufferWriter {
_writeFloat(value);
break;
case PbFieldType._ENUM_BIT:
value as ProtobufEnum;
_writeVarint32(value.value & 0xffffffff);
break;
case PbFieldType._GROUP_BIT:
value as GeneratedMessage;
value.writeToCodedBufferWriter(this);
break;
case PbFieldType._INT32_BIT:
Expand Down Expand Up @@ -386,15 +391,16 @@ class CodedBufferWriter {
break;
case PbFieldType._MESSAGE_BIT:
final mark = _startLengthDelimited();
value as GeneratedMessage;
value.writeToCodedBufferWriter(this);
_endLengthDelimited(mark);
break;
}
}

void _writeBytesNoTag(dynamic value) {
writeInt32NoTag(value.length);
writeRawBytes(value);
void _writeBytesNoTag(Object value) {
mraleph marked this conversation as resolved.
Show resolved Hide resolved
writeInt32NoTag((value as List).length);
writeRawBytes(value as TypedData);
}

void _writeTag(int fieldNumber, int wireFormat) {
Expand Down
4 changes: 2 additions & 2 deletions protobuf/lib/src/protobuf/extension_field_set.dart
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,10 @@ class _ExtensionFieldSet {
_isReadOnly = true;
for (var field in _info.values) {
if (field.isRepeated) {
final entries = _values[field.tagNumber];
final entries = _values[field.tagNumber] as PbList<GeneratedMessage>?;
mraleph marked this conversation as resolved.
Show resolved Hide resolved
if (entries == null) continue;
if (field.isGroupOrMessage) {
for (var subMessage in entries as List<GeneratedMessage>) {
for (var subMessage in entries) {
subMessage.freeze();
}
}
Expand Down
4 changes: 2 additions & 2 deletions protobuf/lib/src/protobuf/extension_registry.dart
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ T _reparseMessage<T extends GeneratedMessage>(
resultMap ??= ensureResult()._fieldSet._values[field.index!];

if (field.isRepeated) {
final messageEntries = message._fieldSet._values[field.index!];
final messageEntries = message._fieldSet._values[field.index!] as List?;
if (messageEntries == null) continue;
if (field.isGroupOrMessage) {
for (var i = 0; i < messageEntries.length; i++) {
Expand All @@ -146,7 +146,7 @@ T _reparseMessage<T extends GeneratedMessage>(
}
}
} else if (field is MapFieldInfo) {
final messageMap = message._fieldSet._values[field.index!];
final messageMap = message._fieldSet._values[field.index!] as PbMap?;
if (messageMap == null) continue;
if (_isGroupOrMessage(field.valueFieldType!)) {
for (var key in messageMap.keys) {
Expand Down
9 changes: 6 additions & 3 deletions protobuf/lib/src/protobuf/field_set.dart
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ class _FieldSet {
_frozenState = true;
for (var field in _meta.sortedByTag) {
if (field.isRepeated) {
final entries = _values[field.index!];
final entries = _values[field.index!] as PbList?;
if (entries == null) continue;
if (field.isGroupOrMessage) {
for (var subMessage in entries as List<GeneratedMessage>) {
Expand Down Expand Up @@ -668,7 +668,9 @@ class _FieldSet {
} else if (!_isEnum(fi.type)) {
hash = _HashUtils._combine(hash, value.hashCode);
} else if (fi.isRepeated) {
hash = _HashUtils._hashObjects(value.map((enm) => enm.value));
value as List;
hash = _HashUtils._hashObjects(
value.map((enm) => (enm as ProtobufEnum).value));
} else {
ProtobufEnum enm = value;
hash = _HashUtils._combine(hash, enm.value);
Expand Down Expand Up @@ -802,6 +804,7 @@ class _FieldSet {
var mustClone = _isGroupOrMessage(otherFi.type);

if (fi!.isMapField) {
fieldValue as Map;
var f = fi as MapFieldInfo<dynamic, dynamic>;
mustClone = _isGroupOrMessage(f.valueFieldType!);
var map = f._ensureMapField(meta, this) as PbMap<dynamic, dynamic>;
Expand Down Expand Up @@ -832,7 +835,7 @@ class _FieldSet {
}

if (otherFi.isGroupOrMessage) {
final currentFi = isExtension!
GeneratedMessage? currentFi = isExtension!
? _ensureExtensions()._getFieldOrNull(fi as Extension<dynamic>)
: _values[fi.index!];

Expand Down
8 changes: 6 additions & 2 deletions protobuf/lib/src/protobuf/json.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
part of protobuf;

Map<String, dynamic> _writeToJsonMap(_FieldSet fs) {
dynamic convertToMap(dynamic fieldValue, int fieldType) {
dynamic convertToMap(Object? fieldValue, int fieldType) {
var baseType = PbFieldType._baseType(fieldType);

if (_isRepeated(fieldType)) {
fieldValue as List;
return List.from(fieldValue.map((e) => convertToMap(e, baseType)));
}

Expand All @@ -27,23 +28,26 @@ Map<String, dynamic> _writeToJsonMap(_FieldSet fs) {
// Encode 'bytes' as a base64-encoded string.
return base64Encode(fieldValue as List<int>);
case PbFieldType._ENUM_BIT:
fieldValue as ProtobufEnum;
return fieldValue.value; // assume |value| < 2^52
case PbFieldType._INT64_BIT:
case PbFieldType._SINT64_BIT:
case PbFieldType._SFIXED64_BIT:
return fieldValue.toString();
case PbFieldType._UINT64_BIT:
case PbFieldType._FIXED64_BIT:
fieldValue as Int64;
return fieldValue.toStringUnsigned();
case PbFieldType._GROUP_BIT:
case PbFieldType._MESSAGE_BIT:
fieldValue as GeneratedMessage;
return fieldValue.writeToJsonMap();
default:
throw 'Unknown type $fieldType';
}
}

List _writeMap(dynamic fieldValue, MapFieldInfo fi) =>
List _writeMap(Map<Object?, Object?> fieldValue, MapFieldInfo fi) =>
List.from(fieldValue.entries.map((MapEntry e) => {
'${PbMap._keyFieldNumber}': convertToMap(e.key, fi.keyFieldType!),
'${PbMap._valueFieldNumber}':
Expand Down
2 changes: 1 addition & 1 deletion protobuf/lib/src/protobuf/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ bool _areMapsEqual(Map lhs, Map rhs) {
}

bool _areByteDataEqual(ByteData lhs, ByteData rhs) {
Uint8List asBytes(d) =>
Uint8List asBytes(ByteData d) =>
Uint8List.view(d.buffer, d.offsetInBytes, d.lengthInBytes);
return _areListsEqual(asBytes(lhs), asBytes(rhs));
}
Expand Down
4 changes: 4 additions & 0 deletions protobuf/test/json_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
// There are more JSON tests in the dart-protoc-plugin package.
library json_test;

// TODO(https://github.com/google/protobuf.dart/issues/578): Remove remaining
// dynamic calls.
// ignore_for_file: avoid_dynamic_calls

import 'dart:convert';

import 'package:fixnum/fixnum.dart' show Int64;
Expand Down
3 changes: 3 additions & 0 deletions protobuf/test/list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ void main() {

test('PbList validates items', () {
expect(() {
// TODO(https://github.com/google/protobuf.dart/issues/578): Remove
// remaining dynamic calls.
// ignore: avoid_dynamic_calls
(PbList<int>() as dynamic).add('hello');
}, throwsA(TypeMatcher<TypeError>()));
});
Expand Down
4 changes: 4 additions & 0 deletions protobuf/test/map_mixin_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
// There are more tests in the dart-protoc-plugin package.
library map_mixin_test;

// TODO(https://github.com/google/protobuf.dart/issues/578): Remove remaining
// dynamic calls.
// ignore_for_file: avoid_dynamic_calls

import 'dart:collection' show MapMixin;

import 'package:protobuf/protobuf.dart';
Expand Down
3 changes: 3 additions & 0 deletions protobuf/test/message_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ class Rec extends MockMessage {
Matcher throwsError(Type expectedType, String expectedMessage) =>
throwsA(predicate((dynamic x) {
expect(x.runtimeType, expectedType);
// TODO(https://github.com/google/protobuf.dart/issues/578): Remove
// remaining dynamic calls.
// ignore: avoid_dynamic_calls
expect(x!.message, expectedMessage);
return true;
}));
Expand Down
3 changes: 3 additions & 0 deletions protobuf/test/readonly_message_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ import 'package:test/test.dart';
Matcher throwsError(Type expectedType, Matcher expectedMessage) =>
throwsA(predicate((dynamic x) {
expect(x.runtimeType, expectedType);
// TODO(https://github.com/google/protobuf.dart/issues/578): Remove
// remaining dynamic calls.
// ignore: avoid_dynamic_calls
expect(x!.message, expectedMessage);
return true;
}));
Expand Down