Adjusted the optional-const/new specs.

Adjusted the optional-const/new specs.

The informal specifications of optional-const and optional-new used to
be incomplete, and this CL adds missing parts (transformation rules for
lists and maps, and for `assignableExpression`).

This version of the documents preserves the property that no `const` is
added without immediate syntactic justification.

The alternative (where `const` is used also in other contexts where
either `const` or `new` must be added, just because we can do that)
is not specified here, that'll be another CL. The corrections done in
this CL will be needed, anyway, so I separated the two.

R=lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/00e98af7efb66a68c744528042fdb2b9a1838854

[eernst, 2017-08-30 09:02:01]

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

[eernst, 2017-08-30 09:02:01]

This CL fixes several omissions in the optional-const and optional-new informal
specs.

It does _not_ introduce the "magic `const`" mechanism whereby `A()` becomes
`const A()` just because it's possible, that will be another CL (upcoming).

[eernst, 2017-08-30 09:02:34]

Description was changed from

==========
Adjusted the optional-const/new specs.

The informal specifications of optional-const and optional-new used to
be incomplete, and this CL adds missing parts (transformation rules for
lists and maps, and for `assignableExpression`).

This version of the documents preserve the property that no `const` is
added without immediate syntactic justification.

The alternative (where `const` is used also in other contexts where
either `const` or `new` must be added, just because we can do that)
is not specified here, that'll be another CL. The corrections done in
this CL will be needed, anyway, so I separated the two.
==========

to

==========
Adjusted the optional-const/new specs.

The informal specifications of optional-const and optional-new used to
be incomplete, and this CL adds missing parts (transformation rules for
lists and maps, and for `assignableExpression`).

This version of the documents preserves the property that no `const` is
added without immediate syntactic justification.

The alternative (where `const` is used also in other contexts where
either `const` or `new` must be added, just because we can do that)
is not specified here, that'll be another CL. The corrections done in
this CL will be needed, anyway, so I separated the two.
==========

[Lasse Reichstein Nielsen, 2017-08-30 09:29:50]

lgtm

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
File docs/language/informal/optional-const.md (right):

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
docs/language/informal/optional-const.md:125: assignableExpressionTail ::=
Is it referenced from anywhere?

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
File docs/language/informal/optional-new.md (right):

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
docs/language/informal/optional-new.md:147: then perform new/const insertion on
_e_.
That sounds wrong, it must always be a `new` insertion.

If you write 
  const [ Foo().x ]
then by the definition here,
  Foo().x 
matches "constructorInvocation assignableSelector" and it occurs in a const
context, so you must insert `const`. 
I know it'll give an error in either case, but I think it's more correct to only
ever insert const in front of `constructorInvocation` and map/list literals.


That is, we allow inserting `new` in more places than inserting `const` because
we allow it in syntactic contexts that can never be const.

[eernst, 2017-08-30 16:51:52]

Review response.

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
File docs/language/informal/optional-const.md (right):

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
docs/language/informal/optional-const.md:90: `postfixExpression` and as part of
an `assignableExpression`. For all other
Oops, adjusted the text here to avoid talking about `assignableExpression`: With
the updated grammar modification that case is not relevant.

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
docs/language/informal/optional-const.md:125: assignableExpressionTail ::=
On 2017/08/30 09:29:49, Lasse Reichstein Nielsen wrote:
> Is it referenced from anywhere?

It used to be referenced from the rewrite rules below, but some simplifications
turned out to be possible and now it's unused (in this document).

Deleted lines 121-126.

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
File docs/language/informal/optional-new.md (right):

https://codereview.chromium.org/3005833002/diff/1/docs/language/informal/opti...
docs/language/informal/optional-new.md:147: then perform new/const insertion on
_e_.
On 2017/08/30 09:29:50, Lasse Reichstein Nielsen wrote:
> That sounds wrong, it must always be a `new` insertion.

Right, that would always insert `new` when the rule was "use `const` unless the
resulting transformed program has a compile-time error", but for the "no magic"
approach that we're now using, it must be fixed. I forgot to do that.

In fact, _no_ correct constant expression is nor contains an
`assignableExpression`, so we should just always insert `new`. Adjusted the
rules to do that.

> If you write 
>   const [ Foo().x ]
> then by the definition here,
>   Foo().x 
> matches "constructorInvocation assignableSelector" and it occurs in a const
> context, so you must insert `const`.

That doesn't actually match constructorInvocation, because that only covers the
"missing" construct (the one which is inevitably a constructor invocation:
`Foo<int>.bar()`, where `typeArguments?` is not omitted, and it invokes a named
constructor; all other cases will just be parsed as regular invocations).

But I agree that `const [ Foo<int>.bar().x ]` shouldn't be transformed to `const
[ const Foo<int>.bar().x ]`, which will then just be rejected with an error
message that developers won't understand.

But^2, if we return to a "magic" approach, we could introduce `const` into an
assignableExpression:

  Foo<int>.bar().x = 27;

could become

  const Foo<int>.bar().x = 27;

and it's certainly possible for `Foo` to declare an `x` setter (and getter)
which can be called even though it's on a const instance. Similarly for the
simpler forms:

  Foo().x = 27;     -->  const Foo().x = 27;
  p.Foo().x++;      -->  const p.Foo().x++;
  --p.Foo.bar().x;  -->  --const p.Foo.bar().x;
  
I guess this just illustrates how magic the magic approach would be. ;-)

> I know it'll give an error in either case, but I think it's more correct to
only
> ever insert const in front of `constructorInvocation` and map/list literals.

Right, that's what we are doing after the review response updates (except that
it's all constructor invocations, not just `constructorInvocation`).

> That is, we allow inserting `new` in more places than inserting `const`
because
> we allow it in syntactic contexts that can never be const.

Indeed, and I think we would do that with every approach (possibly after having
debugged it a bit ;-).

[eernst, 2017-08-30 16:53:47]

Description was changed from

==========
Adjusted the optional-const/new specs.

The informal specifications of optional-const and optional-new used to
be incomplete, and this CL adds missing parts (transformation rules for
lists and maps, and for `assignableExpression`).

This version of the documents preserves the property that no `const` is
added without immediate syntactic justification.

The alternative (where `const` is used also in other contexts where
either `const` or `new` must be added, just because we can do that)
is not specified here, that'll be another CL. The corrections done in
this CL will be needed, anyway, so I separated the two.
==========

to

==========
Adjusted the optional-const/new specs.

The informal specifications of optional-const and optional-new used to
be incomplete, and this CL adds missing parts (transformation rules for
lists and maps, and for `assignableExpression`).

This version of the documents preserves the property that no `const` is
added without immediate syntactic justification.

The alternative (where `const` is used also in other contexts where
either `const` or `new` must be added, just because we can do that)
is not specified here, that'll be another CL. The corrections done in
this CL will be needed, anyway, so I separated the two.

R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/00e98af7efb66a68c744528042fdb2b9a1838854
==========

[eernst, 2017-08-30 16:53:48]

Committed patchset #2 (id:20001) manually as
00e98af7efb66a68c744528042fdb2b9a1838854 (presubmit successful).