Specification adjustments for covariant overrides in Dart 1.

Specification adjustments for covariant overrides in Dart 1.

Addresses the 1.50 bullet in sdk issue #28164.

R=lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/1e483f0bccd170f644841d64f26a88827273c9ba

[eernst, 2017-02-06 13:16:50]

eernst@google.com changed reviewers:
+ lrn@google.com

[eernst, 2017-02-06 13:16:50]

Covariant overrides spec for Dart 1.

[Lasse Reichstein Nielsen, 2017-02-06 17:07:05]

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:396: declaredIdentifier (`,' identifier)*
You cannot apply "covariant" to the second identifier?

That is:
  class C {
    int x, covariant y;
  }
?

Or does the "covariant" apply to all the variables? (I guess it does, since
there is also only one metadata).

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:400: metadata \COVARIANT{}? finalConstVarOrType
identifier
This allows "covariant" on any variable declaration, not just instance
variables.

There should be text somewhere disallowing it on local, static and top-level
variables. E.g.,

"It's a compile-time error if \COVARIANT{} occurs on a variable declaration that
is not an instance variable."

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:792: This modifier has no effect.
This also allows "covariant" on parameters of static or top-level functions,
static setters and constructors. Those should be prohibited somehow. E.g.,

"It's a compile-time error if \COVARIANT{} occurs on a parameter of a function
that is not an instance method or instance setter."

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:936: (\STATIC{} $|$ \COVARIANT{})? (\VAR{} $|$
type) initializedIdentifierList
I'm not sure what this production corresponds to, but I'm guessing it's
declarations that can occur inside a class (or possibly at top-level).

You add \COVARIANT{} here and at variableDeclaration, but one of them must be
redundant or the grammar is ambiguous, or I'm missing something (pick one!
Preferably one of the first two!)

[eernst, 2017-02-10 14:31:57]

Review response.

Note that some changes will be part of the next patch, as mentioned in the
comments.

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:396: declaredIdentifier (`,' identifier)*
On 2017/02/06 17:07:05, Lasse Reichstein Nielsen wrote:
> You cannot apply "covariant" to the second identifier?
> 
> That is:
>   class C {
>     int x, covariant y;
>   }
> ?
> 
> Or does the "covariant" apply to all the variables? (I guess it does, since
> there is also only one metadata).

We have several situations with a bias toward preventing simultaneous
declaration of multiple variables, and we did at some point almost decide to get
rid of that feature. So I did not pay much attention to the treatment of
multi-variable declarations.

We could go three ways: (1) consistently go for declarations of one variable at
a time, assuming that this will be enforced at some point, and then we might as
well keep every mechanism in a form that fits with single-variable declarations,
(2) consistently support multi-variable declarations, and (3) keep
multi-variable declarations for the cases where they are convenient (that is,
probably: simple cases, `int x,y,z;), but keep them "dumb" such that developers
will have to stick to single-variable declarations whenever they want the
complete expressive power.

I tend to think we can be happy with (3).

In that case we would just go for expressive power for single-variable
declarations, but ignore the fact that we cannot get the same level of
expressive power for each variable with multi-variable declarations, which would
fit nicely with a model where every property can be declared once and applies to
all variables in that declaration.

This is just like `final int x, y, z;` and not `int final x, final y, z;`.

That, of course, would work fine with (1) as well. So as long as we don't expect
(2) to happen, we should be fine. ;-)

So I did not make any changes here.

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:400: metadata \COVARIANT{}? finalConstVarOrType
identifier
On 2017/02/06 17:07:05, Lasse Reichstein Nielsen wrote:
> This allows "covariant" on any variable declaration, not just instance
> variables.
> 
> There should be text somewhere disallowing it on local, static and top-level
> variables. E.g.,
> 
> "It's a compile-time error if \COVARIANT{} occurs on a variable declaration
that
> is not an instance variable."

As mentioned IRL, the intention was to specify the usage of `covariant` for spec
mode such that programs using `covariant` could be written in a strong mode
context and reused in spec mode (nobody would ever write `covariant` in a spec
mode context, because it is never required and actually has no effect). As long
as the spec mode language is syntactically large enough to tolerate all the
correct strong mode programs wrt `covariant`, we're OK.

I'll try to formulate those constraints anyway. We might choose to support at
least some level of sanity checks in spec mode, even though it is not realistic
to expect that a spec mode check could ever guarantee that strong mode will also
be happy. This will at least help keeping the source code more strong mode
compliant, even in the situation where this kind of code is modified in a spec
mode context. New patch for that upcoming later.

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:792: This modifier has no effect.
On 2017/02/06 17:07:05, Lasse Reichstein Nielsen wrote:
> This also allows "covariant" on parameters of static or top-level functions,
> static setters and constructors. Those should be prohibited somehow. E.g.,
> 
> "It's a compile-time error if \COVARIANT{} occurs on a parameter of a function
> that is not an instance method or instance setter."

I believe the required constraints are already mentioned in the informal spec
(they should be there, it is intended to specify full support), so I'll take
them from there. Also coming up in that new patch later today.

https://codereview.chromium.org/2678003002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:936: (\STATIC{} $|$ \COVARIANT{})? (\VAR{} $|$
type) initializedIdentifierList
On 2017/02/06 17:07:05, Lasse Reichstein Nielsen wrote:
> I'm not sure what this production corresponds to, but I'm guessing it's
> declarations that can occur inside a class (or possibly at top-level).
> 
> You add \COVARIANT{} here and at variableDeclaration, but one of them must be
> redundant or the grammar is ambiguous, or I'm missing something (pick one!
> Preferably one of the first two!)

Yes, this declares class members that are variables. `initializedIdentifierList`
only derives a comma separated list whose members are `identifier` or
`identifier = expression`, so if we want to allow instance variables to have a
`covariant` modifier then we need to add it here (following the same rule, that
we can't add the modifier to each variable, only to the declaration as a whole).
For the same reason there is no ambiguity problem, we never reach
`declaredIdentifier` from here.

But we need to have `covariant` in `declaredIdentifier` as well, such that we
can declare a covariant `simpleFormalParameter`.

We could change the grammar to use a new `maybeCovariantDeclaredIdentifier` with
`covariant` in `simpleFormalParameter`, and thereby avoid adding syntactic
support for `covariant` in a number of places where it cannot be.

Did that, it's actually much nicer (we avoid for loop decls, local variables,
top level variables syntactically, so we don't have to filter them out
semantically).

[eernst, 2017-02-10 14:39:08]

Patchset #4 (id:60001) has been deleted

[eernst, 2017-02-10 14:39:17]

Patchset #3 (id:40001) has been deleted

[eernst, 2017-02-10 15:45:49]

Compile-time errors now specified; grammar simplified, too!

[Lasse Reichstein Nielsen, 2017-02-25 11:57:33]

lgtm

https://codereview.chromium.org/2678003002/diff/100001/docs/language/dartLang...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2678003002/diff/100001/docs/language/dartLang...
docs/language/dartLangSpec.tex:923: ((\EXTERNAL{} \STATIC{} ?))?
getterSignature;
Remove space before '?' (just for consistency).

https://codereview.chromium.org/2678003002/diff/100001/docs/language/dartLang...
docs/language/dartLangSpec.tex:2441: (`@' qualified ({\escapegrammar `.'}
identifier)? (arguments)?)*
Shouldn't we allow type-parameters for the class here?

[eernst, 2017-03-02 12:41:47]

Description was changed from

==========
Specification adjustments for covariant overrides in Dart 1.

Addresses the 1.50 bullet in sdk issue #28164.
==========

to

==========
Specification adjustments for covariant overrides in Dart 1.

Addresses the 1.50 bullet in sdk issue #28164.

R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1e483f0bccd170f644841d64f26a88827273c9ba
==========

[eernst, 2017-03-02 12:41:49]

Committed patchset #6 (id:140001) manually as
1e483f0bccd170f644841d64f26a88827273c9ba (presubmit successful).

[eernst, 2017-03-02 12:43:01]

Review response. Did not change metadata: Goes to another CL.

https://codereview.chromium.org/2678003002/diff/100001/docs/language/dartLang...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2678003002/diff/100001/docs/language/dartLang...
docs/language/dartLangSpec.tex:923: ((\EXTERNAL{} \STATIC{} ?))?
getterSignature;
On 2017/02/25 11:57:32, Lasse Reichstein Nielsen wrote:
> Remove space before '?' (just for consistency).

Done.

https://codereview.chromium.org/2678003002/diff/100001/docs/language/dartLang...
docs/language/dartLangSpec.tex:2441: (`@' qualified ({\escapegrammar `.'}
identifier)? (arguments)?)*
On 2017/02/25 11:57:32, Lasse Reichstein Nielsen wrote:
> Shouldn't we allow type-parameters for the class here?

That's not related to the overall contents of the CL, and it would have
implications for implementations. However, the tools dart, dart2js, and
dartanalyzer allow for this:

  class C<X> { final X x; const C(this.x); }
  const c = const C<String>('Hello!');
  @c main() {}

So they can all handle it "semantically", just not syntactically. Just checked
with the ANTLR grammar, and it does not create any ambiguities or other parsing
issues there.

All tools that I could think of (including, e.g., dartfmt) reject the type
arguments, which just shows that they follow the current grammar in the spec.

This means that we can just do it, and then request an implementation from
various tool groups, but I'll take that to a separate CL, and leave the text
as-is here for now.