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

Support `@virtual` annotation on fields to allow field overrides in strong mode #27384

Closed
leafpetersen opened this Issue Sep 17, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@leafpetersen
Copy link
Member

leafpetersen commented Sep 17, 2016

The current long term plan is to allow field overrides in strong mode with an opt out syntax for sealed fields. In the meantime, strong mode should support an opt-in for field overrides via an @virtual annotation.

@sethladd @Hixie @abarth

@bwilkerson

This comment has been minimized.

Copy link
Member

bwilkerson commented Sep 17, 2016

Is there a reason why strong mode can't allow field overrides in the short term, rather than to introduce support that will require a breaking change to remove?

@leafpetersen

This comment has been minimized.

Copy link
Member

leafpetersen commented Sep 17, 2016

We plan to have actual syntax, so no matter which annotation which choose now (@sealed, @virtual) removing it will be a breaking change. In the meantime, since DDC is the only platform for which these have semantic meaning, I'd like to keep sealed as the default since this will give our users a better experience on DDC.

@jmesserly

This comment has been minimized.

Copy link
Member

jmesserly commented Sep 17, 2016

@leafpetersen also before we change the default, we should automated refactor our internal code base to seal-all-the-things. (edit: in other words, those fields are already safe to seal, so we might as well get the DDC size+perf benefit from that, rather than regress it.)

@jmesserly

This comment has been minimized.

Copy link
Member

jmesserly commented Sep 17, 2016

note: also need to check that @checked interacts correctly with this (#27363)

@jmesserly

This comment has been minimized.

Copy link
Member

jmesserly commented Sep 17, 2016

@jmesserly jmesserly closed this in 865e808 Sep 19, 2016

@sethladd sethladd changed the title Support `@virtual` annotation on fields to allow field overrides in strong mode Support `@checked` annotation on fields to allow field overrides in strong mode Sep 22, 2016

@leafpetersen leafpetersen changed the title Support `@checked` annotation on fields to allow field overrides in strong mode Support `@virtual` annotation on fields to allow field overrides in strong mode Sep 22, 2016

@leafpetersen

This comment has been minimized.

Copy link
Member

leafpetersen commented Sep 22, 2016

@sethladd This is different from @checked - this is related to field overrides, not covariant overrides. There is a different bug (#27363) for allowing covariant overrides of fields....

@sethladd

This comment has been minimized.

Copy link
Member

sethladd commented Sep 22, 2016

Ah, thanks!

@Hixie

This comment has been minimized.

Copy link
Contributor

Hixie commented Sep 22, 2016

I tried removing strong_mode_invalid_field_override: ignore in Flutter's code base.

One of the resulting errors was just a bug on our side.

For two, arguably three, of the fields, adding @virtual makes sense, in particular RenderObject.parentData and HitTestEntry.target. These are cases where we know that the type will want to be narrowed by subclasses.

For the remaining two cases, there's no way for the superclass to know that the subclass is going to want to override the field:

  • Tween has mutable "begin" and "end" fields. A couple of subclasses (MaterialPointArcTween, MaterialRectArcTween) want to override the setters for those fields to throw because they want to make the fields read-only. This is arguably bogus and the classes should just be fixed. It's not clear how one would write the current interim code with @virtual (modifying the base class seems bad in this case).
  • SimulationGroup tries to override the setter for Simulation.tolerance. It wants to hook into the setter and then call the inherited one. I don't see how @virtual would help here. The base class has no way to know that the field will be overridden in this way.

I wasn't able to test the actual analyzer with @virtual but hopefully the above is useful feedback.

@jmesserly

This comment has been minimized.

Copy link
Member

jmesserly commented Sep 22, 2016

Would it work if you mark @virtual on any public fields that are in classes intended for subclassing? It depends a bit what you're trying to do, but that might be a reasonable rule of thumb. It will probably mark (many) more fields than necessary, but it would allow all of those public fields to be overridden/intercepted.

Imagine if you're designing a class in Java or C++ ... for which fields would you want to make a virtual get/set method? That tells you where to put the @virtual

@leafpetersen

This comment has been minimized.

Copy link
Member

leafpetersen commented Sep 22, 2016

Just a short comment to be sure that it's clear:

These are cases where we know that the type will want to be narrowed by subclasses.

Currently narrowing and overriding are two separate capabilities. @virtual says "I can override this in the subclass, but not necessarily narrow the type (unless it is final)". @checked says "I am narrowing this field, or want to allow a subclass to narrow it".

@Hixie

This comment has been minimized.

Copy link
Contributor

Hixie commented Sep 22, 2016

@jmesserly But everything's virtual in dart. It's weird that (temporarily?) we'd be saying "everything is virtual except field getters and setters except if you say @virtual on the field". I wouldn't want to put @virtual on every field in the framework just in case we happen to override it one day. Especially if it's just temporary.

I guess my question is, if this is just a temporary step along the way to making fields always virtual in strong mode, why not just make FieldElementImpl.isVirtual always return true instead of only returning true when the annotation is there.

@leafpetersen Ah, ok. How will @checked work in this case? Would that be put on the superclass field definition? (I think all the ones where we want that are in fact final, so maybe it's a non-issue?)

@leafpetersen

This comment has been minimized.

Copy link
Member

leafpetersen commented Sep 22, 2016

why not just make FieldElementImpl.isVirtual always return true instead of only returning true when the annotation is there.

Sealed fields are still important for DDC users, and so we wish to maintain the distinction pending syntax for sealing.

How will @checked work in this case? Would that be put on the superclass field definition?

You can place @checked on a superclass field (which enables narrowing on all overrides of that field) or on the overridden field which does the actual narrowing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment