fix #25578, implement @covariant parameter overrides

fix #25578, implement @covariant parameter overrides

R=leafp@google.com

Committed: https://github.com/dart-lang/sdk/commit/28247a3daa1e0ca592667cfcc31614b2e374a3f9

[Jennifer Messerly, 2016-09-12 18:16:13]

jmesserly@google.com changed reviewers:
+ leafp@google.com

[Jennifer Messerly, 2016-09-12 18:16:14]

Hi Leaf,

Here's my initial take on `@covariant` parameter overrides.

I still need to do the DDC side for runtime checks. But let me know your
thoughts on the Analyzer checking. It ended up pretty simple & elegant IMO,
thanks to your help along the way :)

Suggestions for additional tests definitely welcome. I've added some but not
sure I'm covering everything well enough.

[Jennifer Messerly, 2016-09-12 18:24:11]

jmesserly@google.com changed reviewers:
+ rnystrom@google.com

[Jennifer Messerly, 2016-09-12 18:24:12]

+Bob who had thoughts on the name.

I agree @covariant is not appropriate as a user-facing term. So if we might have
this syntax tried out by more than a few folks perhaps @checked or something
like that.

[Jennifer Messerly, 2016-09-12 18:24:49]

On 2016/09/12 18:24:12, John Messerly wrote:
> +Bob who had thoughts on the name.
> 
> I agree @covariant is not appropriate as a user-facing term. So if we might
have
> this syntax tried out by more than a few folks perhaps @checked or something
> like that.

or maybe @check?

(@check String x)

[Bob Nystrom, 2016-09-12 23:03:42]

On 2016/09/12 18:24:49, John Messerly wrote:
> On 2016/09/12 18:24:12, John Messerly wrote:
> > +Bob who had thoughts on the name.
> > 
> > I agree @covariant is not appropriate as a user-facing term. So if we might
> have
> > this syntax tried out by more than a few folks perhaps @checked or something
> > like that.
> 
> or maybe @check?
> 
> (@check String x)

I was thinking "checked". I believe it conveys the idea that there is some
runtime testing involved. If we add a similar feature for covariant callbacks, I
think it looks good there too:

typedef Shape ShapeGrower(@checked Shape shape);

It looks a little strange in the covariant override case because you put it in
the superclass, not the subclass, when it's the subclass method that actually
does the check, but I think that's OK.

I lean towards "checked" instead of "check" because the latter is a verb and
noun. I try to leave those available for users to use. For language-y things and
annotations, I like adjectives ("abstract"), adverbs, and prepositions ("for",
"in", "while"), because they're less likely to collide with identifiers a user
might want to use.

Cheers!

– bob

[Jennifer Messerly, 2016-09-12 23:16:54]

On 2016/09/12 23:03:42, Bob Nystrom wrote:
> On 2016/09/12 18:24:49, John Messerly wrote:
> > On 2016/09/12 18:24:12, John Messerly wrote:
> > > +Bob who had thoughts on the name.
> > > 
> > > I agree @covariant is not appropriate as a user-facing term. So if we
might
> > have
> > > this syntax tried out by more than a few folks perhaps @checked or
something
> > > like that.
> > 
> > or maybe @check?
> > 
> > (@check String x)
> 
> I was thinking "checked". I believe it conveys the idea that there is some
> runtime testing involved. If we add a similar feature for covariant callbacks,
I
> think it looks good there too:
> 
> typedef Shape ShapeGrower(@checked Shape shape);
> 
> It looks a little strange in the covariant override case because you put it in
> the superclass, not the subclass, when it's the subclass method that actually
> does the check, but I think that's OK.
> 
> I lean towards "checked" instead of "check" because the latter is a verb and
> noun. I try to leave those available for users to use. For language-y things
and
> annotations, I like adjectives ("abstract"), adverbs, and prepositions ("for",
> "in", "while"), because they're less likely to collide with identifiers a user
> might want to use.
> 
> Cheers!
> 
> – bob

Oh right. There's also some precedent with "checked" in C#:
https://msdn.microsoft.com/en-us/library/74b4xzyw.aspx

[Jennifer Messerly, 2016-09-12 23:23:13]

FYI -- changed to @checked. Tweaked a few comments too.

[Leaf, 2016-09-12 23:42:37]

lgtm!

https://codereview.chromium.org/2336503003/diff/60001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/2336503003/diff/60001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:808: return relate(this, type,
(DartType t, DartType s, _, __) => t.isAssignableTo(s),
length

https://codereview.chromium.org/2336503003/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/2336503003/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error_verifier.dart:6236: // TODO(leafp): Support
strict downcasts
At least the second of these TODOs is dead, you've already done it... :)  

The point of the first TODO is lost in the mists of time, but I think it's also
dead.

[Jennifer Messerly, 2016-09-13 00:16:21]

Thanks!!!

https://codereview.chromium.org/2336503003/diff/60001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/2336503003/diff/60001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/type.dart:808: return relate(this, type,
(DartType t, DartType s, _, __) => t.isAssignableTo(s),
On 2016/09/12 23:42:37, Leaf wrote:
> length

Done.

https://codereview.chromium.org/2336503003/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/2336503003/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/error_verifier.dart:6236: // TODO(leafp): Support
strict downcasts
On 2016/09/12 23:42:37, Leaf wrote:
> At least the second of these TODOs is dead, you've already done it... :)  
> 
> The point of the first TODO is lost in the mists of time, but I think it's
also
> dead.

Good catch, done!

[Jennifer Messerly, 2016-09-13 21:57:29]

Description was changed from

==========
fix #25578, implement @covariant parameter overrides
==========

to

==========
fix #25578, implement @covariant parameter overrides

R=leafp@google.com

Committed:
https://github.com/dart-lang/sdk/commit/28247a3daa1e0ca592667cfcc31614b2e374a3f9
==========

[Jennifer Messerly, 2016-09-13 21:57:30]

Committed patchset #6 (id:100001) manually as
28247a3daa1e0ca592667cfcc31614b2e374a3f9 (presubmit successful).