From 8b1e241adc934f5f615727082d489b70018eab7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 22 Oct 2025 11:36:19 +0100 Subject: [PATCH 1/3] Add more field value range check tests --- protoc_plugin/test/validate_fail_test.dart | 359 +++++++++++++++++++-- 1 file changed, 339 insertions(+), 20 deletions(-) diff --git a/protoc_plugin/test/validate_fail_test.dart b/protoc_plugin/test/validate_fail_test.dart index 00c155b5..bf3116da 100644 --- a/protoc_plugin/test/validate_fail_test.dart +++ b/protoc_plugin/test/validate_fail_test.dart @@ -6,70 +6,389 @@ import 'package:test/test.dart'; import 'gen/google/protobuf/unittest.pb.dart'; +const int minI32 = -2147483648; + +const int maxI32 = 2147483647; + +const int maxU32 = 4294967295; + +const List invalidFloats = [ + -3.4028234663852886E39, + 3.4028234663852886E39, +]; + void main() { - test('testValidationFailureMessages', () { - final builder = TestAllTypes(); + test('Fields validate values', () { + // int32 + TestAllTypes().optionalInt32 = minI32; + TestAllTypes().optionalInt32 = -123; + TestAllTypes().optionalInt32 = maxI32; + TestAllTypes().optionalInt32 = 123; + + expect(() { + TestAllTypes().optionalInt32 = minI32 - 1; + }, throwsArgumentError); + + expect(() { + TestAllTypes().optionalInt32 = maxI32 + 1; + }, throwsArgumentError); + + // sint32 + TestAllTypes().optionalSint32 = minI32; + TestAllTypes().optionalSint32 = -123; + TestAllTypes().optionalSint32 = maxI32; + TestAllTypes().optionalSint32 = 123; + + expect(() { + TestAllTypes().optionalSint32 = minI32 - 1; + }, throwsArgumentError); + + expect(() { + TestAllTypes().optionalSint32 = maxI32 + 1; + }, throwsArgumentError); + + // sfixed32 + TestAllTypes().optionalSfixed32 = minI32; + TestAllTypes().optionalSfixed32 = -123; + TestAllTypes().optionalSfixed32 = maxI32; + TestAllTypes().optionalSfixed32 = 123; + + expect(() { + TestAllTypes().optionalSfixed32 = minI32 - 1; + }, throwsArgumentError); + + expect(() { + TestAllTypes().optionalSfixed32 = maxI32 + 1; + }, throwsArgumentError); + + // uint32 + TestAllTypes().optionalUint32 = maxU32; + TestAllTypes().optionalUint32 = 123; + + expect(() { + TestAllTypes().optionalUint32 = -1; + }, throwsArgumentError); + + expect(() { + TestAllTypes().optionalUint32 = maxU32 + 1; + }, throwsArgumentError); + + // fixed32 + TestAllTypes().optionalFixed32 = maxU32; + TestAllTypes().optionalFixed32 = 123; + + expect(() { + TestAllTypes().optionalFixed32 = -1; + }, throwsArgumentError); + + expect(() { + TestAllTypes().optionalFixed32 = maxU32 + 1; + }, throwsArgumentError); + + // float + TestAllTypes().optionalFloat = 1.2; + + for (double invalid in invalidFloats) { + expect(() { + TestAllTypes().optionalFloat = invalid; + }, throwsArgumentError); + } + }); + + test('Repeated fields validate values', () { + // int32 + TestAllTypes().repeatedInt32.add(minI32); + TestAllTypes().repeatedInt32.add(-123); + TestAllTypes().repeatedInt32.add(maxI32); + TestAllTypes().repeatedInt32.add(123); + + expect(() { + TestAllTypes().repeatedInt32.add(minI32 - 1); + }, throwsArgumentError); + + expect(() { + TestAllTypes().repeatedInt32.add(maxI32 + 1); + }, throwsArgumentError); + + // sint32 + TestAllTypes().repeatedSint32.add(minI32); + TestAllTypes().repeatedSint32.add(-123); + TestAllTypes().repeatedSint32.add(maxI32); + TestAllTypes().repeatedSint32.add(123); + + expect(() { + TestAllTypes().repeatedSint32.add(minI32 - 1); + }, throwsArgumentError); + + expect(() { + TestAllTypes().repeatedSint32.add(maxI32 + 1); + }, throwsArgumentError); + + // sfixed32 + TestAllTypes().repeatedSfixed32.add(minI32); + TestAllTypes().repeatedSfixed32.add(-123); + TestAllTypes().repeatedSfixed32.add(maxI32); + TestAllTypes().repeatedSfixed32.add(123); + + expect(() { + TestAllTypes().repeatedSfixed32.add(minI32 - 1); + }, throwsArgumentError); + + expect(() { + TestAllTypes().repeatedSfixed32.add(maxI32 + 1); + }, throwsArgumentError); + + // uint32 + TestAllTypes().repeatedUint32.add(maxU32); + TestAllTypes().repeatedUint32.add(123); expect(() { - builder.optionalInt32 = -2147483649; + TestAllTypes().repeatedUint32.add(-1); }, throwsArgumentError); expect(() { - builder.optionalInt32 = 2147483648; + TestAllTypes().repeatedUint32.add(maxU32 + 1); }, throwsArgumentError); + // fixed32 + TestAllTypes().repeatedFixed32.add(maxU32); + TestAllTypes().repeatedFixed32.add(123); + expect(() { - builder.optionalUint32 = -1; + TestAllTypes().repeatedFixed32.add(-1); }, throwsArgumentError); expect(() { - builder.optionalUint32 = 4294967296; + TestAllTypes().repeatedFixed32.add(maxU32 + 1); + }, throwsArgumentError); + + // float + TestAllTypes().repeatedFloat.add(1.2); + + for (double invalid in invalidFloats) { + expect(() { + TestAllTypes().repeatedFloat.add(invalid); + }, throwsArgumentError); + } + }); + + test('Extension fields validate values', () { + // int32 + TestAllExtensions().setExtension(Unittest.optionalInt32Extension, minI32); + TestAllExtensions().setExtension(Unittest.optionalInt32Extension, -123); + TestAllExtensions().setExtension(Unittest.optionalInt32Extension, maxI32); + TestAllExtensions().setExtension(Unittest.optionalInt32Extension, 123); + + expect(() { + TestAllExtensions().setExtension( + Unittest.optionalInt32Extension, + minI32 - 1, + ); }, throwsArgumentError); expect(() { - builder.optionalSint32 = -2147483649; + TestAllExtensions().setExtension( + Unittest.optionalInt32Extension, + maxI32 + 1, + ); }, throwsArgumentError); + // sint32 + TestAllExtensions().setExtension(Unittest.optionalSint32Extension, minI32); + TestAllExtensions().setExtension(Unittest.optionalSint32Extension, -123); + TestAllExtensions().setExtension(Unittest.optionalSint32Extension, maxI32); + TestAllExtensions().setExtension(Unittest.optionalSint32Extension, 123); + expect(() { - builder.optionalSint32 = 2147483648; + TestAllExtensions().setExtension( + Unittest.optionalSint32Extension, + minI32 - 1, + ); }, throwsArgumentError); expect(() { - builder.optionalFixed32 = -1; + TestAllExtensions().setExtension( + Unittest.optionalSint32Extension, + maxI32 + 1, + ); }, throwsArgumentError); + // sfixed32 + TestAllExtensions().setExtension( + Unittest.optionalSfixed32Extension, + minI32, + ); + TestAllExtensions().setExtension(Unittest.optionalSfixed32Extension, -123); + TestAllExtensions().setExtension( + Unittest.optionalSfixed32Extension, + maxI32, + ); + TestAllExtensions().setExtension(Unittest.optionalSfixed32Extension, 123); + expect(() { - builder.optionalFixed32 = 4294967296; + TestAllExtensions().setExtension( + Unittest.optionalSfixed32Extension, + minI32 - 1, + ); }, throwsArgumentError); expect(() { - builder.optionalSfixed32 = -2147483649; + TestAllExtensions().setExtension( + Unittest.optionalSfixed32Extension, + maxI32 + 1, + ); }, throwsArgumentError); + // uint32 + TestAllExtensions().setExtension(Unittest.optionalUint32Extension, maxU32); + TestAllExtensions().setExtension(Unittest.optionalUint32Extension, 123); + expect(() { - builder.optionalSfixed32 = 2147483648; + TestAllExtensions().setExtension(Unittest.optionalUint32Extension, -1); }, throwsArgumentError); expect(() { - builder.optionalFloat = -3.4028234663852886E39; + TestAllExtensions().setExtension( + Unittest.optionalUint32Extension, + maxU32 + 1, + ); }, throwsArgumentError); + // fixed32 + TestAllExtensions().setExtension(Unittest.optionalFixed32Extension, maxU32); + TestAllExtensions().setExtension(Unittest.optionalFixed32Extension, 123); + expect(() { - builder.optionalFloat = 3.4028234663852886E39; + TestAllExtensions().setExtension(Unittest.optionalFixed32Extension, -1); }, throwsArgumentError); - // Set repeating value (no setter should exist). expect(() { - (builder as dynamic).repeatedInt32 = 201; - }, throwsNoSuchMethodError); + TestAllExtensions().setExtension( + Unittest.optionalFixed32Extension, + maxU32 + 1, + ); + }, throwsArgumentError); + + // float + TestAllExtensions().setExtension(Unittest.optionalFloatExtension, 1.2); + + for (double invalid in invalidFloats) { + expect(() { + TestAllExtensions().setExtension( + Unittest.optionalFloatExtension, + invalid, + ); + }, throwsArgumentError); + } + }); + + test('Repetaed extension fields validate values', () { + // int32 + TestAllExtensions().addExtension(Unittest.repeatedInt32Extension, minI32); + TestAllExtensions().addExtension(Unittest.repeatedInt32Extension, -123); + TestAllExtensions().addExtension(Unittest.repeatedInt32Extension, maxI32); + TestAllExtensions().addExtension(Unittest.repeatedInt32Extension, 123); - // Unknown tag. expect(() { - builder.setField(999, 'field'); + TestAllExtensions().addExtension( + Unittest.repeatedInt32Extension, + minI32 - 1, + ); }, throwsArgumentError); expect(() { - TestAllExtensions().setExtension(Unittest.optionalInt32Extension, '101'); + TestAllExtensions().addExtension( + Unittest.repeatedInt32Extension, + maxI32 + 1, + ); }, throwsArgumentError); + + // sint32 + TestAllExtensions().addExtension(Unittest.repeatedSint32Extension, minI32); + TestAllExtensions().addExtension(Unittest.repeatedSint32Extension, -123); + TestAllExtensions().addExtension(Unittest.repeatedSint32Extension, maxI32); + TestAllExtensions().addExtension(Unittest.repeatedSint32Extension, 123); + + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedSint32Extension, + minI32 - 1, + ); + }, throwsArgumentError); + + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedSint32Extension, + maxI32 + 1, + ); + }, throwsArgumentError); + + // sfixed32 + TestAllExtensions().addExtension( + Unittest.repeatedSfixed32Extension, + minI32, + ); + TestAllExtensions().addExtension(Unittest.repeatedSfixed32Extension, -123); + TestAllExtensions().addExtension( + Unittest.repeatedSfixed32Extension, + maxI32, + ); + TestAllExtensions().addExtension(Unittest.repeatedSfixed32Extension, 123); + + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedSfixed32Extension, + minI32 - 1, + ); + }, throwsArgumentError); + + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedSfixed32Extension, + maxI32 + 1, + ); + }, throwsArgumentError); + + // uint32 + TestAllExtensions().addExtension(Unittest.repeatedUint32Extension, maxU32); + TestAllExtensions().addExtension(Unittest.repeatedUint32Extension, 123); + + expect(() { + TestAllExtensions().addExtension(Unittest.repeatedUint32Extension, -1); + }, throwsArgumentError); + + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedUint32Extension, + maxU32 + 1, + ); + }, throwsArgumentError); + + // fixed32 + TestAllExtensions().addExtension(Unittest.repeatedFixed32Extension, maxU32); + TestAllExtensions().addExtension(Unittest.repeatedFixed32Extension, 123); + + expect(() { + TestAllExtensions().addExtension(Unittest.repeatedFixed32Extension, -1); + }, throwsArgumentError); + + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedFixed32Extension, + maxU32 + 1, + ); + }, throwsArgumentError); + + // float + TestAllExtensions().addExtension(Unittest.repeatedFloatExtension, 1.2); + + for (double invalid in invalidFloats) { + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedFloatExtension, + invalid, + ); + }, throwsArgumentError); + } }); } From cbefc5984791ddbdd70c0dd0bd6397c56d209a89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 22 Oct 2025 11:47:13 +0100 Subject: [PATCH 2/3] More tests --- protoc_plugin/test/validate_fail_test.dart | 76 +++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/protoc_plugin/test/validate_fail_test.dart b/protoc_plugin/test/validate_fail_test.dart index bf3116da..ed90e7f6 100644 --- a/protoc_plugin/test/validate_fail_test.dart +++ b/protoc_plugin/test/validate_fail_test.dart @@ -17,8 +17,30 @@ const List invalidFloats = [ 3.4028234663852886E39, ]; +const isTypeError = TypeMatcher(); + +// ignore: deprecated_member_use +const Matcher throwsTypeError = Throws(isTypeError); + void main() { test('Fields validate values', () { + // Nullability and type checks + expect(() { + (TestAllTypes() as dynamic).optionalNestedMessage = null; + }, throwsTypeError); + + expect(() { + (TestAllTypes() as dynamic).optionalBool = null; + }, throwsTypeError); + + expect(() { + (TestAllTypes() as dynamic).optionalNestedMessage = 123; + }, throwsTypeError); + + expect(() { + (TestAllTypes() as dynamic).optionalBool = 123; + }, throwsTypeError); + // int32 TestAllTypes().optionalInt32 = minI32; TestAllTypes().optionalInt32 = -123; @@ -96,6 +118,23 @@ void main() { }); test('Repeated fields validate values', () { + // Nullability and type checks + expect(() { + (TestAllTypes() as dynamic).repeatedNestedMessage.add(null); + }, throwsTypeError); + + expect(() { + (TestAllTypes() as dynamic).repeatedBool.add(null); + }, throwsTypeError); + + expect(() { + (TestAllTypes() as dynamic).repeatedNestedMessage.add(123); + }, throwsTypeError); + + expect(() { + (TestAllTypes() as dynamic).repeatedBool.add(123); + }, throwsTypeError); + // int32 TestAllTypes().repeatedInt32.add(minI32); TestAllTypes().repeatedInt32.add(-123); @@ -173,6 +212,18 @@ void main() { }); test('Extension fields validate values', () { + // Type checks + expect(() { + TestAllExtensions().setExtension( + Unittest.optionalNestedMessageExtension, + 123, + ); + }, throwsArgumentError); + + expect(() { + TestAllExtensions().setExtension(Unittest.optionalBoolExtension, 123); + }, throwsArgumentError); + // int32 TestAllExtensions().setExtension(Unittest.optionalInt32Extension, minI32); TestAllExtensions().setExtension(Unittest.optionalInt32Extension, -123); @@ -282,7 +333,30 @@ void main() { } }); - test('Repetaed extension fields validate values', () { + test('Repeated extension fields validate values', () { + // Nullability and type checks + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedNestedMessageExtension, + null, + ); + }, throwsTypeError); + + expect(() { + TestAllExtensions().addExtension( + Unittest.repeatedNestedMessageExtension, + 123, + ); + }, throwsTypeError); + + expect(() { + TestAllExtensions().addExtension(Unittest.repeatedBoolExtension, null); + }, throwsTypeError); + + expect(() { + TestAllExtensions().addExtension(Unittest.repeatedBoolExtension, 123); + }, throwsTypeError); + // int32 TestAllExtensions().addExtension(Unittest.repeatedInt32Extension, minI32); TestAllExtensions().addExtension(Unittest.repeatedInt32Extension, -123); From be96bf35df6afaed4b131f92261731a8649cd84a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 22 Oct 2025 12:58:55 +0100 Subject: [PATCH 3/3] Avoid redundant nullability checks in repeated fields Since sound null safety we no longer need to manually check for nulls when setting fields, as we don't use nullable types for field values. Fixes #978. --- protobuf/lib/src/protobuf/builder_info.dart | 2 +- protobuf/lib/src/protobuf/extension.dart | 2 +- protobuf/lib/src/protobuf/field_error.dart | 6 +-- protobuf/lib/src/protobuf/field_info.dart | 6 +-- protobuf/lib/src/protobuf/pb_list.dart | 42 ++++++++++++++------- 5 files changed, 37 insertions(+), 21 deletions(-) diff --git a/protobuf/lib/src/protobuf/builder_info.dart b/protobuf/lib/src/protobuf/builder_info.dart index 5d1019e7..28997e4c 100644 --- a/protobuf/lib/src/protobuf/builder_info.dart +++ b/protobuf/lib/src/protobuf/builder_info.dart @@ -120,7 +120,7 @@ class BuilderInfo { int tagNumber, String name, int fieldType, - CheckFunc check, + CheckFunc? check, CreateBuilderFunc? subBuilder, ValueOfFunc? valueOf, List? enumValues, { diff --git a/protobuf/lib/src/protobuf/extension.dart b/protobuf/lib/src/protobuf/extension.dart index dc37b91f..1af1529a 100644 --- a/protobuf/lib/src/protobuf/extension.dart +++ b/protobuf/lib/src/protobuf/extension.dart @@ -35,7 +35,7 @@ class Extension extends FieldInfo { String name, int tagNumber, int fieldType, { - required CheckFunc check, + required CheckFunc? check, CreateBuilderFunc? subBuilder, ValueOfFunc? valueOf, List? enumValues, diff --git a/protobuf/lib/src/protobuf/field_error.dart b/protobuf/lib/src/protobuf/field_error.dart index c6d7e40c..a9116023 100644 --- a/protobuf/lib/src/protobuf/field_error.dart +++ b/protobuf/lib/src/protobuf/field_error.dart @@ -72,8 +72,8 @@ String? _getFieldError(int fieldType, var value) { /// unsigned 32 bit ints where there also is a range check. /// /// @nodoc -CheckFunc getCheckFunction(int fieldType) { - switch (fieldType & ~0x7) { +CheckFunc? getCheckFunction(int fieldType) { + switch (PbFieldType.baseType(fieldType)) { case PbFieldType.BOOL_BIT: case PbFieldType.BYTES_BIT: case PbFieldType.STRING_BIT: @@ -89,7 +89,7 @@ CheckFunc getCheckFunction(int fieldType) { // We always use the full range of the same Dart type. // It's up to the caller to treat the Int64 as signed or unsigned. // See: https://github.com/google/protobuf.dart/issues/44 - return checkNotNull; + return null; case PbFieldType.FLOAT_BIT: return _checkFloat; diff --git a/protobuf/lib/src/protobuf/field_info.dart b/protobuf/lib/src/protobuf/field_info.dart index cf5b28cc..775b3c0f 100644 --- a/protobuf/lib/src/protobuf/field_info.dart +++ b/protobuf/lib/src/protobuf/field_info.dart @@ -138,7 +138,7 @@ class FieldInfo { this.tagNumber, this.index, this.type, - CheckFunc this.check, + this.check, this.subBuilder, { this.valueOf, this.enumValues, @@ -232,13 +232,13 @@ class FieldInfo { /// Creates a repeated field. PbList _createRepeatedField() { assert(isRepeated); - return PbList(check: check!); + return PbList(check: check); } /// Same as above, but allow a tighter typed [PbList] to be created. PbList _createRepeatedFieldWithType() { assert(isRepeated); - return PbList(check: check!); + return PbList(check: check); } /// Convenience method to thread this FieldInfo's reified type parameter to diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index 4736f6c3..1c2c8ce4 100644 --- a/protobuf/lib/src/protobuf/pb_list.dart +++ b/protobuf/lib/src/protobuf/pb_list.dart @@ -29,15 +29,13 @@ class PbList extends ListBase { /// We can't use `const []` as it makes the `_wrappedList` field polymorphic. static final _emptyList = []; - final CheckFunc _check; + final CheckFunc? _check; bool _isReadOnly = false; bool get isFrozen => _isReadOnly; - PbList({CheckFunc check = checkNotNull}) - : _wrappedList = [], - _check = check; + PbList({CheckFunc? check}) : _wrappedList = [], _check = check; PbList.unmodifiable() : _wrappedList = _emptyList, @@ -52,7 +50,9 @@ class PbList extends ListBase { @pragma('dart2js:never-inline') void add(E element) { _checkModifiable('add'); - _check(element); + if (_check != null) { + _check(element); + } _wrappedList.add(element); } @@ -67,7 +67,9 @@ class PbList extends ListBase { @pragma('dart2js:never-inline') void addAll(Iterable iterable) { _checkModifiable('addAll'); - iterable.forEach(_check); + if (_check != null) { + iterable.forEach(_check); + } _wrappedList.addAll(iterable); } @@ -96,21 +98,27 @@ class PbList extends ListBase { @override void insert(int index, E element) { _checkModifiable('insert'); - _check(element); + if (_check != null) { + _check(element); + } _wrappedList.insert(index, element); } @override void insertAll(int index, Iterable iterable) { _checkModifiable('insertAll'); - iterable.forEach(_check); + if (_check != null) { + iterable.forEach(_check); + } _wrappedList.insertAll(index, iterable); } @override void setAll(int index, Iterable iterable) { _checkModifiable('setAll'); - iterable.forEach(_check); + if (_check != null) { + iterable.forEach(_check); + } _wrappedList.setAll(index, iterable); } @@ -149,7 +157,9 @@ class PbList extends ListBase { _checkModifiable('setRange'); // NOTE: In case `take()` returns less than `end - start` elements, the // _wrappedList will fail with a `StateError`. - iterable.skip(skipCount).take(end - start).forEach(_check); + if (_check != null) { + iterable.skip(skipCount).take(end - start).forEach(_check); + } _wrappedList.setRange(start, end, iterable, skipCount); } @@ -162,7 +172,9 @@ class PbList extends ListBase { @override void fillRange(int start, int end, [E? fill]) { _checkModifiable('fillRange'); - _check(fill); + if (_check != null) { + _check(fill); + } _wrappedList.fillRange(start, end, fill); } @@ -170,7 +182,9 @@ class PbList extends ListBase { void replaceRange(int start, int end, Iterable newContents) { _checkModifiable('replaceRange'); final values = newContents.toList(); - newContents.forEach(_check); + if (_check != null) { + newContents.forEach(_check); + } _wrappedList.replaceRange(start, end, values); } @@ -202,7 +216,9 @@ class PbList extends ListBase { @override void operator []=(int index, E value) { _checkModifiable('set element'); - _check(value); + if (_check != null) { + _check(value); + } _wrappedList[index] = value; }