-
Notifications
You must be signed in to change notification settings - Fork 180
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
Generate toBuilder
overrides with covariate return type
#177
Comments
I see the problem. The real solution as I see it is to make class GeneratedMessage<M extends GeneratedMessage<M>> {
...
M createEmptyInstance();
M toBuilder() {
final result = createEmptyInstance();
result._fieldSet._shallowCopyValues(_fieldSet);
return result;
}
...
} The message classes would then be like: class Foo extends GeneratedMessage<Foo> {
...
Foo createEmptyInstance() => new Foo();
...
} I don't really know how this would affect code size and performance, and how hard the migration would be. I guess the generic type would take up a field in each message instance. @rakudrama, @nichite any insights? Possibly it is not too bad. built_value does this already. (https://github.com/google/built_value.dart/blob/master/built_value/lib/built_value.dart#L11) |
Just as an aside from a language evolution point of view, this would be a good example of a situation where a proper self-type and the ability to require re-implementation would work well: class GeneratedMessage {
...
This createEmptyInstance();
}
class Foo extends GeneratedMessage {
...
This createEmptyInstance() => new Foo();
} This would make it possible to know at call sites that the returned result has the same type as the receiver, and given that the static type of the receiver is a subtype of the dynamic type this would always allow us to get an optimal and safe approximation of the actual type of object returned. The implementation It wouldn't be hard to make it safe, however. We could have support for a kind of marker (for instance, we could annotate (With the code generation approach there would have to be an overriding declaration in every subtype of If this is handled using metadata like |
@sigurdm We should try to get to zero methods in the generated class that are overrides of the base class. This opens up the possibility of optimizations where the derived class is implemented as the base class. I am worried about the code size impact of making GeneratedMessage generic, but it does seem like the right thing to do. If we go in that direction, I would make BuilderInfo generic too, and store create() in the BuilderInfo so that we could have one definition of createNewInstance: class BuilderInfo<M extends GeneratedMessage<M>> {
final M Function() _createFunction;
BuilderInfo(this._createFunction, ...) {...}
...
}
class GeneratedMessage<M extends GeneratedMessage<M>> {
final BuilderInfo<M> info_;
final FieldSet _fieldSet;
GeneratedMessage(this.info_) : _fieldSet = ... info_ ...;
M createNewInstance() => (info_._createFunction)();
M clone() => createNewInstance.mergeFrom(this);
}
class FooMessage extends GeneratedMessage<FooMessage> {
static BuilderInfo<FooMessage> _i = BuilderInfo<FooMessage>(create, ....);
static FooMessage create() => FooMessage._();
FooMessage._() : super(_i);
} My concern about code size is around the fact that FooMessage implements GeneratedMessage where M=FooMessage. Currently the binding of M in this case is done as an instance method of FooMessage. I hope to work on making the types less connected to the JavaScript constructors so this might be fixed in the (not near) future. @eernstg class GeneratedMessage {
final BuilderInfo<This> info_;
This createNewInstance() => (info_._createFunction)();
} |
Actually there is a slight difference between
You could not do that with I will look into a prototype of a Generic |
I created a pull request to peruse the option - but didn't do experiments with it. |
@rakudrama wrote:
It's not quite a special case. If I understand you correctly then we could say that the approach based on But we don't get the same properties—in particular, the compiler cannot trust all subtypes to follow this programming idiom, which means that abstract class C1<X extends C1<X>> { X me(); } // Introduce `X` as a This type.
abstract class C2<X extends C2<X>> extends C1<X> {} // Maintain the This type variable.
class D extends C2<D> { D me() => this; } // As intended: `this` has type `D`.
class E extends C2<D> { E me() => this; } // Oops! So it's a special case in the same sense as declaring a variable About the class GeneratedMessage {
final BuilderInfo<This> info_;
GeneratedMessage(this.info_);
This createNewInstance() => (info_._createFunction)();
} When a |
Any update on this please? I would imagine that requiring casts whenever someone uses I ended up here because I commented on what I thought were unnecessary type annotations in a code review--turns out they are needed after all, as they are casts. |
Any updates? |
|
Some things can be done with extensions like we do with Here is how we do with rebuild:
Can we write:
Then we "push the cast one more step back". Furthermore we could make "createEmptyInstance()" protected, and make an extension with the right type that does the cast.
|
Fun stuff. I looked briefly at moving One thing to note is that it's disruptive to code if extension methods are used as they need a new 'import' to get the extension--most code that uses protos does not import it. This can be mitigated by adding an 'export' to all generated proto source, and that mostly works, but there can still be cases of protos being used without importing any proto where a new import will need adding. |
Good point! Also there are semantic differences between a static method and an instance method - so we might prefer new names for these while deprecating the old. |
Any updates on this? |
At the moment
toBuilder
methods are only defined on theGeneratedMessage
, which means they are always of typeGeneratedMessage
. This works poorly whenno-implicit-casts
is used as it requires explicit casting to the actual type.The text was updated successfully, but these errors were encountered: