From 8335677db9144b07710a9b32ed6176bdb5d6564d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Tue, 14 Oct 2025 11:30:55 +0100 Subject: [PATCH 1/7] Fix a few extension field bugs --- .../lib/src/protobuf/extension_field_set.dart | 1 - protobuf/lib/src/protobuf/field_set.dart | 38 +++++++++++++- .../lib/src/protobuf/generated_message.dart | 34 +++---------- protoc_plugin/test/extension_test.dart | 50 +++++++++++++++++++ .../test/protos/extend_unittest.proto | 4 ++ 5 files changed, 98 insertions(+), 29 deletions(-) diff --git a/protobuf/lib/src/protobuf/extension_field_set.dart b/protobuf/lib/src/protobuf/extension_field_set.dart index 442f7709e..053e67b3c 100644 --- a/protobuf/lib/src/protobuf/extension_field_set.dart +++ b/protobuf/lib/src/protobuf/extension_field_set.dart @@ -121,7 +121,6 @@ class ExtensionFieldSet { ), ); } - _ensureWritable(); _validateInfo(fi); _parent._validateField(fi, value); _addInfoUnchecked(fi); diff --git a/protobuf/lib/src/protobuf/field_set.dart b/protobuf/lib/src/protobuf/field_set.dart index 0b1aed8fa..7f21b39d7 100644 --- a/protobuf/lib/src/protobuf/field_set.dart +++ b/protobuf/lib/src/protobuf/field_set.dart @@ -106,8 +106,18 @@ class FieldSet { /// The [FieldInfo] for each non-extension field in tag order. Iterable get _infosSortedByTag => _meta.sortedByTag; - ExtensionFieldSet _ensureExtensions() => - _extensions ??= ExtensionFieldSet(this); + ExtensionFieldSet _ensureExtensions() { + var extensions = _extensions; + if (extensions != null) { + return extensions; + } + extensions = ExtensionFieldSet(this); + _extensions = extensions; + if (_isReadOnly) { + extensions._markReadOnly(); + } + return extensions; + } UnknownFieldSet _ensureUnknownFields() { if (_unknownFields == null) { @@ -990,6 +1000,30 @@ class FieldSet { _oneofCases!.addAll(originalOneofCases); } } + + bool hasExtension(Extension extension) => + _extensions?._getFieldOrNull(extension) != null; + + dynamic getExtension(Extension extension) => + _ensureExtensions()._getFieldOrDefault(extension); + + void setExtension(Extension extension, Object value) => + _ensureExtensions()._setFieldAndInfo(extension, value); + + void addExtension(Extension extension, Object? value) { + _ensureWritable(); + if (!extension.isRepeated) { + throw ArgumentError( + 'Cannot add to a non-repeated field (use setExtension())', + ); + } + _ensureExtensions()._ensureRepeatedField(extension).add(value); + } + + void clearExtension(Extension extension) { + _ensureWritable(); + _extensions?._clearFieldAndInfo(extension); + } } extension FieldSetInternalExtension on FieldSet { diff --git a/protobuf/lib/src/protobuf/generated_message.dart b/protobuf/lib/src/protobuf/generated_message.dart index 5c9da7702..62028d9ac 100644 --- a/protobuf/lib/src/protobuf/generated_message.dart +++ b/protobuf/lib/src/protobuf/generated_message.dart @@ -334,19 +334,12 @@ abstract class GeneratedMessage { /// /// The backing [List] will be created if necessary. /// If the list already exists, the old extension won't be overwritten. - void addExtension(Extension extension, Object? value) { - if (!extension.isRepeated) { - throw ArgumentError( - 'Cannot add to a non-repeated field (use setExtension())', - ); - } - _fieldSet._ensureExtensions()._ensureRepeatedField(extension).add(value); - } + void addExtension(Extension extension, Object? value) => + _fieldSet.addExtension(extension, value); /// Clears an extension field and also removes the extension. - void clearExtension(Extension extension) { - _fieldSet._extensions?._clearFieldAndInfo(extension); - } + void clearExtension(Extension extension) => + _fieldSet.clearExtension(extension); /// Clears the contents of a given field. /// @@ -364,7 +357,7 @@ abstract class GeneratedMessage { /// /// If not set, returns the extension's default value. dynamic getExtension(Extension extension) => - _fieldSet._ensureExtensions()._getFieldOrDefault(extension); + _fieldSet.getExtension(extension); /// Returns the value of the field associated with [tagNumber], or the /// default value if it is not set. @@ -386,8 +379,7 @@ abstract class GeneratedMessage { _fieldSet._ensureInfo(tagNumber).readonlyDefault; /// Returns `true` if a value of [extension] is present. - bool hasExtension(Extension extension) => - _fieldSet._extensions?._getFieldOrNull(extension) != null; + bool hasExtension(Extension extension) => _fieldSet.hasExtension(extension); /// Whether this message has a field associated with [tagNumber]. bool hasField(int tagNumber) => _fieldSet._hasField(tagNumber); @@ -406,18 +398,8 @@ abstract class GeneratedMessage { .mergeFromUnknownFieldSet(unknownFieldSet); /// Sets the value of a non-repeated extension field to [value]. - void setExtension(Extension extension, Object value) { - if (PbFieldType.isRepeated(extension.type)) { - throw ArgumentError( - _fieldSet._setFieldFailedMessage( - extension, - value, - 'repeating field (use get + .add())', - ), - ); - } - _fieldSet._ensureExtensions()._setFieldAndInfo(extension, value); - } + void setExtension(Extension extension, Object value) => + _fieldSet.setExtension(extension, value); /// Sets the value of a field by its [tagNumber]. /// diff --git a/protoc_plugin/test/extension_test.dart b/protoc_plugin/test/extension_test.dart index 0e4864c17..10a7a7af5 100644 --- a/protoc_plugin/test/extension_test.dart +++ b/protoc_plugin/test/extension_test.dart @@ -696,4 +696,54 @@ void main() { final d = r.reparseMessage(c); expect(m.hashCode, d.hashCode); }); + + test('setExtension throws when the extension field is repeated', () { + final m = Outer(); + m.addExtension(Extend_unittest.extensionRepeated, 'hi'); + expect(() { + m.setExtension(Extend_unittest.extensionRepeated, "bye"); + }, throwsArgumentError); + }); + + test('addExtension throws when the extension field is not repeated', () { + final m = Outer(); + m.setExtension(Extend_unittest.extensionInner, Inner()); + expect(() { + m.addExtension(Extend_unittest.extensionInner, Inner()); + }, throwsArgumentError); + }); + + test('addExtension throws when the message is frozen', () { + final m = Outer()..freeze(); + expect( + () => m.addExtension(Extend_unittest.extensionRepeated, 'hi'), + throwsUnsupportedError, + ); + }); + + test('setExtension throws when the message is frozen', () { + final m = Outer()..freeze(); + expect( + () => m.setExtension(Extend_unittest.extensionInner, Inner()), + throwsUnsupportedError, + ); + }); + + test( + 'getExtension returns frozen non-repeated value when the parent message is frozen', + () { + final m = Outer()..freeze(); + final Inner ext = m.getExtension(Extend_unittest.extensionInner); + expect(() => ext.value = 'hi', throwsUnsupportedError); + }, + ); + + test( + 'getExtension returns frozen repeated value when the parent message is frozen', + () { + final m = Outer()..freeze(); + final List ext = m.getExtension(Extend_unittest.extensionRepeated); + expect(() => ext.add('hi'), throwsUnsupportedError); + }, + ); } diff --git a/protoc_plugin/test/protos/extend_unittest.proto b/protoc_plugin/test/protos/extend_unittest.proto index 2532ba213..6a6e12044 100644 --- a/protoc_plugin/test/protos/extend_unittest.proto +++ b/protoc_plugin/test/protos/extend_unittest.proto @@ -42,3 +42,7 @@ extend InnerMost { extend protobuf_unittest.TestAllExtensions { optional Outer outer = 104; } + +extend Outer { + repeated string extension_repeated = 6; +} \ No newline at end of file From dbb776444da77ac5dbbb4bd65e6e348f92a28742 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Tue, 14 Oct 2025 11:50:10 +0100 Subject: [PATCH 2/7] More tests --- protoc_plugin/test/extension_test.dart | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/protoc_plugin/test/extension_test.dart b/protoc_plugin/test/extension_test.dart index 10a7a7af5..b379c6f1f 100644 --- a/protoc_plugin/test/extension_test.dart +++ b/protoc_plugin/test/extension_test.dart @@ -746,4 +746,15 @@ void main() { expect(() => ext.add('hi'), throwsUnsupportedError); }, ); + + test( + 'getExtension repeated value turns frozen when the parent message is frozen', + () { + final m = Outer(); + final List ext = m.getExtension(Extend_unittest.extensionRepeated); + ext.add('hi'); + m.freeze(); + expect(() => ext.add('bye'), throwsUnsupportedError); + }, + ); } From 1d7cfab19e45a758bc60a08a814cb26b31423ec3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Tue, 14 Oct 2025 11:52:09 +0100 Subject: [PATCH 3/7] Update changelog --- protobuf/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/protobuf/CHANGELOG.md b/protobuf/CHANGELOG.md index 5b911adb1..4c1633b0f 100644 --- a/protobuf/CHANGELOG.md +++ b/protobuf/CHANGELOG.md @@ -5,7 +5,12 @@ The new limits are consistent with the Java and C++ implementations. ([#1060]) +* Fix `GeneratedMessage.addExtension` returning non-frozen and + `GeneratedMessage.getExtension` allowing modifying an extension when the + message is frozen before initializing the extension field set. ([#1062]) + [#1060]: https://github.com/google/protobuf.dart/pull/1060 +[#1062]: https://github.com/google/protobuf.dart/pull/1062 ## 5.0.0 From 5ca6f74d37eae649a60c9940a1fc0b563c226b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Tue, 14 Oct 2025 12:47:31 +0100 Subject: [PATCH 4/7] lint --- protoc_plugin/test/extension_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protoc_plugin/test/extension_test.dart b/protoc_plugin/test/extension_test.dart index b379c6f1f..abbc28e43 100644 --- a/protoc_plugin/test/extension_test.dart +++ b/protoc_plugin/test/extension_test.dart @@ -701,7 +701,7 @@ void main() { final m = Outer(); m.addExtension(Extend_unittest.extensionRepeated, 'hi'); expect(() { - m.setExtension(Extend_unittest.extensionRepeated, "bye"); + m.setExtension(Extend_unittest.extensionRepeated, 'bye'); }, throwsArgumentError); }); From 2a3b10483d475eba1041362ab32fbb4a7b07036a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 15 Oct 2025 09:58:12 +0100 Subject: [PATCH 5/7] Fix yet another bug --- protobuf/CHANGELOG.md | 4 ++++ .../lib/src/protobuf/extension_field_set.dart | 2 +- protoc_plugin/test/extension_test.dart | 17 +++++++++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/protobuf/CHANGELOG.md b/protobuf/CHANGELOG.md index 4c1633b0f..a376c83be 100644 --- a/protobuf/CHANGELOG.md +++ b/protobuf/CHANGELOG.md @@ -9,6 +9,10 @@ `GeneratedMessage.getExtension` allowing modifying an extension when the message is frozen before initializing the extension field set. ([#1062]) +* Fix `GeneratedMessage.getExtension` returning differently typed lists when the + message extension field set is initialized and frozen and initialized but not + frozen. ([#1062]) + [#1060]: https://github.com/google/protobuf.dart/pull/1060 [#1062]: https://github.com/google/protobuf.dart/pull/1062 diff --git a/protobuf/lib/src/protobuf/extension_field_set.dart b/protobuf/lib/src/protobuf/extension_field_set.dart index 053e67b3c..3ea5eb2de 100644 --- a/protobuf/lib/src/protobuf/extension_field_set.dart +++ b/protobuf/lib/src/protobuf/extension_field_set.dart @@ -58,7 +58,7 @@ class ExtensionFieldSet { final value = _values[fi.tagNumber]; if (value != null) return value; _checkNotInUnknown(fi); - if (_isReadOnly) return PbList.unmodifiable(); + if (_isReadOnly) return fi._createRepeatedField()..freeze(); return _addInfoAndCreateList(fi); } diff --git a/protoc_plugin/test/extension_test.dart b/protoc_plugin/test/extension_test.dart index abbc28e43..c4fb7242a 100644 --- a/protoc_plugin/test/extension_test.dart +++ b/protoc_plugin/test/extension_test.dart @@ -757,4 +757,21 @@ void main() { expect(() => ext.add('bye'), throwsUnsupportedError); }, ); + + test( + 'getExtension repeated value type is right when the field set is frozen', + () { + { + final m = Outer(); + final ext = m.getExtension(Extend_unittest.extensionRepeated); + expect(ext, isA>()); + } + + { + final m = Outer().freeze(); + final ext = m.getExtension(Extend_unittest.extensionRepeated); + expect(ext, isA>()); + } + }, + ); } From d1d86723eeebd2403bf617dbeeb0af77defe3a3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Wed, 15 Oct 2025 10:07:36 +0100 Subject: [PATCH 6/7] Newline --- protoc_plugin/test/protos/extend_unittest.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protoc_plugin/test/protos/extend_unittest.proto b/protoc_plugin/test/protos/extend_unittest.proto index 6a6e12044..f1d7d50c0 100644 --- a/protoc_plugin/test/protos/extend_unittest.proto +++ b/protoc_plugin/test/protos/extend_unittest.proto @@ -45,4 +45,4 @@ extend protobuf_unittest.TestAllExtensions { extend Outer { repeated string extension_repeated = 6; -} \ No newline at end of file +} From 246d28ca0c16ee2dbc3b8116ac22f890b0bee4ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Mon, 20 Oct 2025 10:40:33 +0100 Subject: [PATCH 7/7] Simplify --- protobuf/lib/src/protobuf/extension_field_set.dart | 10 ++++++---- protobuf/lib/src/protobuf/field_set.dart | 14 ++------------ 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/protobuf/lib/src/protobuf/extension_field_set.dart b/protobuf/lib/src/protobuf/extension_field_set.dart index 3ea5eb2de..c61a0ea0a 100644 --- a/protobuf/lib/src/protobuf/extension_field_set.dart +++ b/protobuf/lib/src/protobuf/extension_field_set.dart @@ -8,13 +8,15 @@ class ExtensionFieldSet { final FieldSet _parent; final Map _info; final Map _values; - bool _isReadOnly = false; + bool _isReadOnly; - ExtensionFieldSet(this._parent) + ExtensionFieldSet(this._parent, {required bool readOnly}) : _info = {}, - _values = {}; + _values = {}, + _isReadOnly = readOnly; - ExtensionFieldSet._(this._parent, this._info, this._values); + ExtensionFieldSet._(this._parent, this._info, this._values) + : _isReadOnly = false; Extension? _getInfoOrNull(int tagNumber) => _info[tagNumber]; diff --git a/protobuf/lib/src/protobuf/field_set.dart b/protobuf/lib/src/protobuf/field_set.dart index 7f21b39d7..66cd91ac3 100644 --- a/protobuf/lib/src/protobuf/field_set.dart +++ b/protobuf/lib/src/protobuf/field_set.dart @@ -106,18 +106,8 @@ class FieldSet { /// The [FieldInfo] for each non-extension field in tag order. Iterable get _infosSortedByTag => _meta.sortedByTag; - ExtensionFieldSet _ensureExtensions() { - var extensions = _extensions; - if (extensions != null) { - return extensions; - } - extensions = ExtensionFieldSet(this); - _extensions = extensions; - if (_isReadOnly) { - extensions._markReadOnly(); - } - return extensions; - } + ExtensionFieldSet _ensureExtensions() => + _extensions ??= ExtensionFieldSet(this, readOnly: _isReadOnly); UnknownFieldSet _ensureUnknownFields() { if (_unknownFields == null) {