changes for 0.3.9

changes for 0.3.9

- add experimental PbMapMixin class
- fix bug where ExtensionRegistry would not be used for nested messages.

BUG=https://github.com/dart-lang/dart-protobuf/issues/27
R=sgjesse@google.com

Committed: https://github.com/dart-lang/protobuf/commit/08bb09f58349707af89ebc51a7552e59d9a3eca5

[skybrian, 2015-06-20 00:50:38]

skybrian@google.com changed reviewers:
+ sgjesse@google.com, sigmund@google.com

[Søren Gjesse, 2015-06-22 07:25:01]

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/cod...
File lib/src/protobuf/coded_buffer_reader.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/cod...
lib/src/protobuf/coded_buffer_reader.dart:70:
message.mergeFromCodedBufferReader(this, extensionRegistry);
Should there be a regression test for this bug?

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
File lib/src/protobuf/map_mixin.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
lib/src/protobuf/map_mixin.dart:20: class PbMapMixin implements Map<String,
dynamic> {
Just for context here. Is this supposed to replace/simplify the code generated
for the map interface by the Dart protoc plugin?

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
lib/src/protobuf/map_mixin.dart:63: // methods copied from MapMixin (since
mixins cannot mix in other mixins).
Could another option be that whenever PbMapMixin is used as a mixin MapMixin has
to be used as well?

https://chromiumcodereview.appspot.com/1196773004/diff/1/test/map_mixin_test....
File test/map_mixin_test.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/1/test/map_mixin_test....
test/map_mixin_test.dart:108: });
Test iterators as well, and test that concurrent modification errors are thrown
as expected.

[skybrian, 2015-06-22 17:19:02]

> Just for context here. Is this supposed to replace/simplify the code generated
> for the map interface by the Dart protoc plugin?

Yes, there is another CL that goes along with this. (Somehow I missed adding
reviewers until now.)
https://chromiumcodereview.appspot.com/1192943003/

[Siggi Cherem (dart-lang), 2015-06-22 19:45:37]

I like this general change of decoupling things from protoc.

I added one general suggestion below (which would affect both this CL and the
protoc one)

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
File lib/src/protobuf/map_mixin.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
lib/src/protobuf/map_mixin.dart:15: /// A PbMapMixin provides an experimental
implementation of the
since your new protoc is more pluggable, I wonder if it's possible to take this
out of this package, and provide it to protoc externally.

The main challenge I see is how to provide the list of reserved names to protoc.
For that one option could be to provide the list of names on a flag. Another
idea is to use a second isolate to load the definition of the mixin, and use
mirrors to extract the required names.

Thoughts?

[skybrian, 2015-06-22 21:53:50]

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
File lib/src/protobuf/map_mixin.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
lib/src/protobuf/map_mixin.dart:15: /// A PbMapMixin provides an experimental
implementation of the
On 2015/06/22 19:45:37, Siggi Cherem (dart-lang) wrote:
> since your new protoc is more pluggable, I wonder if it's possible to take
this
> out of this package, and provide it to protoc externally.

I'd like to keep it in the open source code as an example.

> The main challenge I see is how to provide the list of reserved names to
protoc.
> For that one option could be to provide the list of names on a flag. Another
> idea is to use a second isolate to load the definition of the mixin, and use
> mirrors to extract the required names.

Yes, I'd like to relax the restrictions on mixins, but that's trickier and I
need to get something out soon, so I'm going to leave it as a followup.

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
lib/src/protobuf/map_mixin.dart:20: class PbMapMixin implements Map<String,
dynamic> {
On 2015/06/22 07:25:01, Søren Gjesse wrote:
> Just for context here. Is this supposed to replace/simplify the code generated
> for the map interface by the Dart protoc plugin?

Yes.

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
lib/src/protobuf/map_mixin.dart:63: // methods copied from MapMixin (since
mixins cannot mix in other mixins).
On 2015/06/22 07:25:01, Søren Gjesse wrote:
> Could another option be that whenever PbMapMixin is used as a mixin MapMixin
has
> to be used as well?

Good idea. I can implement a simple form of inheritance without much trouble.

[Siggi Cherem (dart-lang), 2015-06-22 22:22:25]

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
File lib/src/protobuf/map_mixin.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/map...
lib/src/protobuf/map_mixin.dart:15: /// A PbMapMixin provides an experimental
implementation of the
On 2015/06/22 21:53:50, skybrian wrote:
> On 2015/06/22 19:45:37, Siggi Cherem (dart-lang) wrote:
> > since your new protoc is more pluggable, I wonder if it's possible to take
> this
> > out of this package, and provide it to protoc externally.
> 
> I'd like to keep it in the open source code as an example.

Totally, we could still include an example in the protoc repo (e.g. an example
folder).

Long term, I would prefer if this mixin is not baked into this package. Since
the idea is for others to be able to provide their own mixin, it would be nice
to show how to do so without having to modify this library (which is hard for
people to do).

> 
> > The main challenge I see is how to provide the list of reserved names to
> protoc.
> > For that one option could be to provide the list of names on a flag. Another
> > idea is to use a second isolate to load the definition of the mixin, and use
> > mirrors to extract the required names.
> 
> Yes, I'd like to relax the restrictions on mixins, but that's trickier and I
> need to get something out soon, so I'm going to leave it as a followup.

[skybrian, 2015-06-23 02:35:34]

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/cod...
File lib/src/protobuf/coded_buffer_reader.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/1/lib/src/protobuf/cod...
lib/src/protobuf/coded_buffer_reader.dart:70:
message.mergeFromCodedBufferReader(this, extensionRegistry);
On 2015/06/22 07:25:01, Søren Gjesse wrote:
> Should there be a regression test for this bug?

Probably, but for now, a regression will be caught by another test because
dart_options handling in the protoc compiler won't work without it.

https://chromiumcodereview.appspot.com/1196773004/diff/1/test/map_mixin_test....
File test/map_mixin_test.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/1/test/map_mixin_test....
test/map_mixin_test.dart:108: });
On 2015/06/22 07:25:01, Søren Gjesse wrote:
> Test iterators as well, and test that concurrent modification errors are
thrown
> as expected.

Obsolete because this mixin is "inheriting" MapMixin now.
(Via configuration in mixins_meta.dart)

[skybrian, 2015-06-23 02:38:08]

On 2015/06/22 22:22:25, Siggi Cherem (dart-lang) wrote:

> Totally, we could still include an example in the protoc repo (e.g. an example
> folder).
> 
> Long term, I would prefer if this mixin is not baked into this package. Since
> the idea is for others to be able to provide their own mixin, it would be nice
> to show how to do so without having to modify this library (which is hard for
> people to do).

Makes sense, once we have a way to actually do it.

(I started on it by moving PbMapMixin to a separate library, though it's still
in the same package.)

[Søren Gjesse, 2015-06-23 06:49:56]

lgtm

https://chromiumcodereview.appspot.com/1196773004/diff/20001/lib/mixins_meta....
File lib/mixins_meta.dart (right):

https://chromiumcodereview.appspot.com/1196773004/diff/20001/lib/mixins_meta....
lib/mixins_meta.dart:21: /// The mixin can be applied to a message using the
options in dart_options.proto.
Long line.

https://chromiumcodereview.appspot.com/1196773004/diff/20001/lib/mixins_meta....
lib/mixins_meta.dart:26: /// The Dart symbol to be imported into the .pb.dart
file.
Maybe be more explicit in this comment that this is the class name of the actual
mixin.

https://chromiumcodereview.appspot.com/1196773004/diff/20001/lib/mixins_meta....
lib/mixins_meta.dart:70: const _mapMixin = const PbMixin._raw("MapMixin",
This mixin is in this registry because it is required by PbMapMixin. However it
cannot be a primary mixin (or at least is not intended to be as it does not work
on its own). Should there be a boolean flag here to indicate that this is not a
primary mixin so that the protoc plugin can fail if this is the only mixin used.

Of course getting into this situation require "mis-configuration" of the prats
of the protoc-plugin that are currently hard-coded.

[skybrian, 2015-06-23 17:23:42]

Committed patchset #2 (id:20001) manually as
08bb09f58349707af89ebc51a7552e59d9a3eca5 (presubmit successful).

[skybrian, 2015-06-24 18:39:04]

(Oops, didn't publish this earlier.)

https://codereview.chromium.org/1196773004/diff/20001/lib/mixins_meta.dart
File lib/mixins_meta.dart (right):

https://codereview.chromium.org/1196773004/diff/20001/lib/mixins_meta.dart#ne...
lib/mixins_meta.dart:21: /// The mixin can be applied to a message using the
options in dart_options.proto.
On 2015/06/23 06:49:56, Søren Gjesse wrote:
> Long line.

Done.

https://codereview.chromium.org/1196773004/diff/20001/lib/mixins_meta.dart#ne...
lib/mixins_meta.dart:26: /// The Dart symbol to be imported into the .pb.dart
file.
On 2015/06/23 06:49:56, Søren Gjesse wrote:
> Maybe be more explicit in this comment that this is the class name of the
actual
> mixin.

Done.

https://codereview.chromium.org/1196773004/diff/20001/lib/mixins_meta.dart#ne...
lib/mixins_meta.dart:70: const _mapMixin = const PbMixin._raw("MapMixin",
On 2015/06/23 06:49:56, Søren Gjesse wrote:
> This mixin is in this registry because it is required by PbMapMixin. However
it
> cannot be a primary mixin (or at least is not intended to be as it does not
work
> on its own). Should there be a boolean flag here to indicate that this is not
a
> primary mixin so that the protoc plugin can fail if this is the only mixin
used.
> 
> Of course getting into this situation require "mis-configuration" of the prats
> of the protoc-plugin that are currently hard-coded.

I'm relying on _exportedMixins to indicate whether a Mixin class is "public".
Since MapMixin is not listed there, findMixin will return null and the protoc
plugin will report that the mixin isn't found if someone attempts to use it in a
 .proto file.

We could make the error message better by using a flag like you suggest, but
I'm going to skip that for now.