Allow application of external mixins to generated dart protos.

Allow application of external mixins to generated dart protos.

These are defined using the new dart_option "mixins".

For example usage, see test/protos/mixins.proto.

This change requires issue 2090033002 to be submitted first, which introduces protobuf version 0.5.2.

R=skybrian@google.com

Committed: https://github.com/dart-lang/dart-protoc-plugin/commit/fc1de6cf0dae146bf9f7d1053639bcdd71bc6b34

[frederikmutzel, 2016-06-22 10:59:08]

Description was changed from

==========
Allow application of external mixins to generated dart protos.

These are defined using the new dart_option "mixins".
==========

to

==========
Allow application of external mixins to generated dart protos.

These are defined using the new dart_option "mixins".

For example usage, see test/protos/mixins.proto.

This change requires issue 2090033002 to be submitted first, which introduces
protobuf version 0.5.2.
==========

[frederikmutzel, 2016-06-22 10:59:08]

frederikmutzel@google.com changed reviewers:
+ skybrian@google.com

[skybrian, 2016-06-22 19:27:18]

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart
File lib/file_generator.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart...
lib/file_generator.dart:21: throw ('Duplicate mixin name: "${mixin.name}"');
style nit: parentheses aren't needed.

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart...
lib/file_generator.dart:32: }
Also check for cycles?

[skybrian, 2016-06-22 20:29:53]

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart
File lib/file_generator.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart...
lib/file_generator.dart:21: throw ('Duplicate mixin name: "${mixin.name}"');
When we throw we should give some indication of the location in the file where
the error is. We can't give a line number unfortunately, but we could say
something like:

somefile.proto in 'mixins' file option: duplicate mixin name: SomeMixin

Similarly for other errors.

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart...
lib/file_generator.dart:59: var defaultMixin =
should check that we can find defaultMixin.
It will be checked later, but if we do it here, we can give the proper location
of the error. (It's on the defaultMixin file option, not on the messages.)

https://codereview.chromium.org/2086253002/diff/20001/lib/message_generator.dart
File lib/message_generator.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/lib/message_generator.d...
lib/message_generator.dart:23: if (children.contains(parentName)) {
Seems better to check for cycles sooner? The error is not in the Message that
references the mixin; it is in the "mixins" file option.

(The cycle is still there even if no message references the mixin, so it should
probably still be an error.)

https://codereview.chromium.org/2086253002/diff/20001/lib/testing/mixins.dart
File lib/testing/mixins.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/lib/testing/mixins.dart...
lib/testing/mixins.dart:1: abstract class Mixin1 {
Can we put this file under "test" instead of in "lib/testing"?

I think we can get away with a relative import in the .proto file:
"../../test/mixins.dart"

https://codereview.chromium.org/2086253002/diff/20001/test/mixin_test.dart
File test/mixin_test.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/test/mixin_test.dart#ne...
test/mixin_test.dart:25: test('overrides field with annotation', () {
It's unclear what we're testing here. It seems like this would work whether or
not the field has 'override' set.

https://codereview.chromium.org/2086253002/diff/20001/test/protos/dart_option...
File test/protos/dart_options.proto (right):

https://codereview.chromium.org/2086253002/diff/20001/test/protos/dart_option...
test/protos/dart_options.proto:32: // and may override them.
Need a warning here: there is no error-checking for inadvertent name conflicts
between mixin fields and protobuf fields. (But dartanalyzer might find some of
them.)

I think we need the warning because you are writing a tool for this but it won't
be open source.

https://codereview.chromium.org/2086253002/diff/20001/test/protos/dart_option...
test/protos/dart_options.proto:51: // Specifies a field to override
getters/setters from the inherited mixin.
I don't see any implementation of "override" - that is, in this patch it doesn't
do anything.

One thing it could do is add the @override annotation to the getter, setter, and
clear method, allowing some errors to be detected with dartanalyzer. But we
probably need separate options to add @override to the getter, setter, and clear
methods.

Maybe leave it out of this patch?

[frederikmutzel, 2016-06-23 12:30:16]

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart
File lib/file_generator.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart...
lib/file_generator.dart:21: throw ('Duplicate mixin name: "${mixin.name}"');
On 2016/06/22 19:27:18, skybrian wrote:
> style nit: parentheses aren't needed.

Done.

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart...
lib/file_generator.dart:21: throw ('Duplicate mixin name: "${mixin.name}"');
On 2016/06/22 20:29:52, skybrian wrote:
> When we throw we should give some indication of the location in the file where
> the error is. We can't give a line number unfortunately, but we could say
> something like:
> 
> somefile.proto in 'mixins' file option: duplicate mixin name: SomeMixin
> 
> Similarly for other errors.

Done.

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart...
lib/file_generator.dart:32: }
On 2016/06/22 19:27:18, skybrian wrote:
> Also check for cycles?
> 

Done.

https://codereview.chromium.org/2086253002/diff/20001/lib/file_generator.dart...
lib/file_generator.dart:59: var defaultMixin =
On 2016/06/22 20:29:52, skybrian wrote:
> should check that we can find defaultMixin.
> It will be checked later, but if we do it here, we can give the proper
location
> of the error. (It's on the defaultMixin file option, not on the messages.)

Done.

https://codereview.chromium.org/2086253002/diff/20001/lib/message_generator.dart
File lib/message_generator.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/lib/message_generator.d...
lib/message_generator.dart:23: if (children.contains(parentName)) {
On 2016/06/22 20:29:52, skybrian wrote:
> Seems better to check for cycles sooner? The error is not in the Message that
> references the mixin; it is in the "mixins" file option.
> 
> (The cycle is still there even if no message references the mixin, so it
should
> probably still be an error.)
> 
> 
> 

Moved to file_generator.

I also moved the conversion to PbMixin and made sure that the generated PbMixins
are unique (otherwise you could get double imports).

https://codereview.chromium.org/2086253002/diff/20001/lib/testing/mixins.dart
File lib/testing/mixins.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/lib/testing/mixins.dart...
lib/testing/mixins.dart:1: abstract class Mixin1 {
On 2016/06/22 20:29:52, skybrian wrote:
> Can we put this file under "test" instead of in "lib/testing"?
> 
> I think we can get away with a relative import in the .proto file:
> "../../test/mixins.dart"
> 

I'd really prefer to use a package import here. Reasons: 
- It makes it easier to move the generated dart file
- Clients are more likely to use package imports (and they probably should), so
it would be a better example
- Internally, it seems like people prefer putting only tests in test/ and
testing support libraries in testing/ (although protobuf doesn't seem to do
that)

I'm generally not a fan of relative imports, tooling seems to handle package
imports much better as well when refactoring.

https://codereview.chromium.org/2086253002/diff/20001/test/mixin_test.dart
File test/mixin_test.dart (right):

https://codereview.chromium.org/2086253002/diff/20001/test/mixin_test.dart#ne...
test/mixin_test.dart:25: test('overrides field with annotation', () {
On 2016/06/22 20:29:53, skybrian wrote:
> It's unclear what we're testing here. It seems like this would work whether or
> not the field has 'override' set.

Removed.

https://codereview.chromium.org/2086253002/diff/20001/test/protos/dart_option...
File test/protos/dart_options.proto (right):

https://codereview.chromium.org/2086253002/diff/20001/test/protos/dart_option...
test/protos/dart_options.proto:32: // and may override them.
On 2016/06/22 20:29:53, skybrian wrote:
> Need a warning here: there is no error-checking for inadvertent name conflicts
> between mixin fields and protobuf fields. (But dartanalyzer might find some of
> them.)
> 
> I think we need the warning because you are writing a tool for this but it
won't
> be open source.

Done.

https://codereview.chromium.org/2086253002/diff/20001/test/protos/dart_option...
test/protos/dart_options.proto:51: // Specifies a field to override
getters/setters from the inherited mixin.
On 2016/06/22 20:29:53, skybrian wrote:
> I don't see any implementation of "override" - that is, in this patch it
doesn't
> do anything.
> 
> One thing it could do is add the @override annotation to the getter, setter,
and
> clear method, allowing some errors to be detected with dartanalyzer. But we
> probably need separate options to add @override to the getter, setter, and
clear
> methods.
> 
> Maybe leave it out of this patch?
> 
> 

Right, removed for now.

[skybrian, 2016-06-23 19:00:00]

On 2016/06/23 12:30:16, frederikmutzel wrote:

> I'd really prefer to use a package import here.

Okay. (It also has the advantage that we don't have to change the import in the
google version.)

[skybrian, 2016-06-23 19:01:42]

LGTM

In the interest of making progress, I'm going to land this as-is. Here are some
lower priority fixes for a followup patch.

https://codereview.chromium.org/2086253002/diff/40001/lib/file_generator.dart
File lib/file_generator.dart (right):

https://codereview.chromium.org/2086253002/diff/40001/lib/file_generator.dart...
lib/file_generator.dart:23: final dartMixins = <String, DartMixin>{};
can move down

https://codereview.chromium.org/2086253002/diff/40001/lib/file_generator.dart...
lib/file_generator.dart:51: bool internalMixin = !declaredMixin &&
findMixin(parentName) != null;
Should we allow inheritance between declared and internal mixins? If you declare
one, maybe you should declare all of them?

https://codereview.chromium.org/2086253002/diff/40001/lib/message_generator.dart
File lib/message_generator.dart (right):

https://codereview.chromium.org/2086253002/diff/40001/lib/message_generator.d...
lib/message_generator.dart:41: Map<String, PbMixin> declaredMixins, String
defaultMixin)
Maybe change defaultMixin back to having type PbMixin so we don't need to look
it up again.

https://codereview.chromium.org/2086253002/diff/40001/test/protos/dart_option...
File test/protos/dart_options.proto (right):

https://codereview.chromium.org/2086253002/diff/40001/test/protos/dart_option...
test/protos/dart_options.proto:18: // File from which the mixin class is
imported.
"File" => "URI"

[skybrian, 2016-06-23 19:58:01]

When I tried testing this I got:

mixins.proto:5:8: Option field "(dart_options.mixins)" is repeated. Repeated
options are not supported.

Based on [1], it appears we will need to require protoc 2.6.1 for open source
users. This is the current version from [2] but it looks like users might need
to compile it from source?  (On my workstation, using apt-get, I have 2.5.0.)

[1] https://github.com/google/protobuf/issues/40
[2] https://developers.google.com/protocol-buffers/docs/downloads

[frederikmutzel, 2016-06-23 21:20:18]

On 2016/06/23 19:58:01, skybrian wrote:
> When I tried testing this I got:
> 
> mixins.proto:5:8: Option field "(dart_options.mixins)" is repeated. Repeated
> options are not supported.
> 
> Based on [1], it appears we will need to require protoc 2.6.1 for open source
> users. This is the current version from [2] but it looks like users might need
> to compile it from source?  (On my workstation, using apt-get, I have 2.5.0.)
> 
> [1] https://github.com/google/protobuf/issues/40
> [2] https://developers.google.com/protocol-buffers/docs/downloads

Hmm, I'm not sure which version of protoc I have on my machine, it might even be
one of the 3.0.0 betas.

It looks like the last 4 Ubuntu versions ship with 2.6.1:
https://launchpad.net/ubuntu/+source/protobuf

Should I look into making this a single option with a repeated field?

[skybrian, 2016-06-23 21:25:17]

Good idea. Let's try that.

ImportedMixins maybe?

[skybrian, 2016-06-24 00:13:19]

On 2016/06/23 21:25:17, skybrian wrote:
> Good idea. Let's try that.
> 
> ImportedMixins maybe?

Oh and to be clear, I decided not to submit this today, so please make your
changes and I will try again tomorrow.

[frederikmutzel, 2016-06-24 08:00:29]

On 2016/06/24 00:13:19, skybrian wrote:
> On 2016/06/23 21:25:17, skybrian wrote:
> > Good idea. Let's try that.
> > 
> > ImportedMixins maybe?
> 
> Oh and to be clear, I decided not to submit this today, so please make your
> changes and I will try again tomorrow.

Yep, sure.
Unfortunately I won't get to it today (most of ZRH is out and the office is
being moved around), I'll update it on Monday.

[frederikmutzel, 2016-06-27 08:44:18]

On 2016/06/24 08:00:29, frederikmutzel wrote:
> On 2016/06/24 00:13:19, skybrian wrote:
> > On 2016/06/23 21:25:17, skybrian wrote:
> > > Good idea. Let's try that.
> > > 
> > > ImportedMixins maybe?
> > 
> > Oh and to be clear, I decided not to submit this today, so please make your
> > changes and I will try again tomorrow.
> 
> Yep, sure.
> Unfortunately I won't get to it today (most of ZRH is out and the office is
> being moved around), I'll update it on Monday.

Updated - PTAL

[frederikmutzel, 2016-06-27 08:44:31]

https://chromiumcodereview.appspot.com/2086253002/diff/40001/lib/file_generat...
File lib/file_generator.dart (right):

https://chromiumcodereview.appspot.com/2086253002/diff/40001/lib/file_generat...
lib/file_generator.dart:23: final dartMixins = <String, DartMixin>{};
On 2016/06/23 19:01:42, skybrian wrote:
> can move down

Done.

https://chromiumcodereview.appspot.com/2086253002/diff/40001/lib/file_generat...
lib/file_generator.dart:51: bool internalMixin = !declaredMixin &&
findMixin(parentName) != null;
On 2016/06/23 19:01:42, skybrian wrote:
> Should we allow inheritance between declared and internal mixins? If you
declare
> one, maybe you should declare all of them?

I think it could make migration easier if you don't have to migrate all of them
at once. Don't feel strongly about it though, since it's just going to be
temporary.

https://chromiumcodereview.appspot.com/2086253002/diff/40001/lib/message_gene...
File lib/message_generator.dart (right):

https://chromiumcodereview.appspot.com/2086253002/diff/40001/lib/message_gene...
lib/message_generator.dart:41: Map<String, PbMixin> declaredMixins, String
defaultMixin)
On 2016/06/23 19:01:42, skybrian wrote:
> Maybe change defaultMixin back to having type PbMixin so we don't need to look
> it up again.

Done.

[skybrian, 2016-06-27 23:33:38]

Thanks, I will land this (with changes below).

(I will be on vacation on Wednesday and would like to get this in.)

https://codereview.chromium.org/2086253002/diff/80001/lib/message_generator.dart
File lib/message_generator.dart (right):

https://codereview.chromium.org/2086253002/diff/80001/lib/message_generator.d...
lib/message_generator.dart:14: PbMixin getMixinByName(String name) {
Could inline this. (Only called once.)

https://codereview.chromium.org/2086253002/diff/80001/lib/message_generator.d...
lib/message_generator.dart:16: return declaredMixins[name] ?? findMixin(name);
Need to report an error when the name is non-empty but isn't found.

https://codereview.chromium.org/2086253002/diff/80001/test/protos/dart_option...
File test/protos/dart_options.proto (right):

https://codereview.chromium.org/2086253002/diff/80001/test/protos/dart_option...
test/protos/dart_options.proto:29: message ImportedMixins {
On second thought, maybe just "Imports". This will allow us to add non-mixin
imports to the same message if we have some reason to do that.

https://codereview.chromium.org/2086253002/diff/80001/test/protos/dart_option...
test/protos/dart_options.proto:43: optional ImportedMixins imported_mixins =
28125061;
rename to "imports". Most of the comment can be moved to the "mixins" field.

[skybrian, 2016-06-28 00:13:08]

https://codereview.chromium.org/2086253002/diff/80001/test/protos/mixins.proto
File test/protos/mixins.proto (right):

https://codereview.chromium.org/2086253002/diff/80001/test/protos/mixins.prot...
test/protos/mixins.proto:6: mixins: [{
The bad news is that this syntax for initializing a repeated field doesn't work
with protoc 2.5.0 either.

mixins.proto:5:41: Error while parsing option value for "imported_mixins":
Expected "{", found "[".

However, I can still populate this mixin list with a single mixin if I comment
out the other two, so I guess it's not a total loss.

I think we should require a newer release of protoc for this feature.

[skybrian, 2016-06-28 00:46:38]

Description was changed from

==========
Allow application of external mixins to generated dart protos.

These are defined using the new dart_option "mixins".

For example usage, see test/protos/mixins.proto.

This change requires issue 2090033002 to be submitted first, which introduces
protobuf version 0.5.2.
==========

to

==========
Allow application of external mixins to generated dart protos.

These are defined using the new dart_option "mixins".

For example usage, see test/protos/mixins.proto.

This change requires issue 2090033002 to be submitted first, which introduces
protobuf version 0.5.2.

R=skybrian@google.com

Committed:
https://github.com/dart-lang/dart-protoc-plugin/commit/fc1de6cf0dae146bf9f7d1...
==========

[skybrian, 2016-06-28 00:46:40]

Committed patchset #5 (id:80001) manually as
fc1de6cf0dae146bf9f7d1053639bcdd71bc6b34 (presubmit successful).

[skybrian, 2016-06-28 05:29:53]

On 2016/06/28 00:46:40, skybrian wrote:
> Committed patchset #5 (id:80001) manually as
> fc1de6cf0dae146bf9f7d1053639bcdd71bc6b34 (presubmit successful).

Landed as-is and created another patch:
https://codereview.chromium.org/2103743002/