Make ?? a compile-time constant operator.

Make ?? a compile-time constant operator.

Update specification, change analyzer, VM and dart2js.

Fixes issue #25054.
BUG= http://dartbug.com/25054
R=floitsch@google.com, gbracha@google.com

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

[Lasse Reichstein Nielsen, 2015-12-02 11:21:09]

lrn@google.com changed reviewers:
+ floitsch@google.com, gbracha@google.com, rnystrom@google.com

[Lasse Reichstein Nielsen, 2015-12-02 11:21:10]

Proposal for spec fix-up.
If accepted, also contains implementations.

[Ivan Posva, 2015-12-02 15:58:47]

iposva@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2015-12-02 15:58:47]

Please add tests so that we can see what kind of behaviour would be allowed
after your proposed change.

Thanks,
-Ivan

[Brian Wilkerson, 2015-12-02 16:10:24]

brianwilkerson@google.com changed reviewers:
+ brianwilkerson@google.com

[Brian Wilkerson, 2015-12-02 16:10:24]

Is there a DEP proposal for this?

The analyzer changes, while fine, are incomplete. I'll be happy to work with you
to identify other changes that need to be made, but I think it would be better
to pull that out into a separate CL.

[sra1, 2015-12-02 17:21:18]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-12-02 17:21:20]

dart2js lgtm with tests.

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

https://codereview.chromium.org/1493693002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:38: A conforming  implementation of the Dart
programming language must provide and support all the  APIs (libraries, types,
functions, getters, setters, whether top-level, static, instance or local)
mandated in this specification.
Please fix the whitespace in a separate CL and rebase so we can find the changed
text :-)

[hausner, 2015-12-02 18:29:37]

hausner@google.com changed reviewers:
+ hausner@google.com

[hausner, 2015-12-02 18:29:38]

DBC: why only the ?? operator. Seems arbitrary. Could also consider ?. 

On the other hand, what is the use of this? If you know the left-hand side is
null at compile-time, why even use the ?? operator?

https://codereview.chromium.org/1493693002/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1493693002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:10555: if (lhs->IsPotentiallyConst()) {
This is a bit more complicated.

You need to use lhs->EvalConstExpr() to determine whether the expression is a
compile-time const. If that returns non-NULL, use FoldConstExpr() to evaluate
the expression to get the actual value.

Also, background compilation has recently made all this more complex. The
optimizing compiler runs in another thread and must not evaluate dart code.
Thus, you probably have to cache the evaluated constant, similar to the code
used in, for example, ParseExternalInitializedField().

[sra1, 2015-12-02 18:50:08]

On 2015/12/02 18:29:38, hausner wrote:
> DBC: why only the ?? operator. Seems arbitrary. Could also consider ?. 

c?.foo makes sense only where c.foo makes sense. Do we have any getters or
methods that are constant expressions?
I believe only c.length. We should add it, but don't let that stop ??.

> 
> On the other hand, what is the use of this? If you know the left-hand side is
> null at compile-time, why even use the ?? operator?

const name = const String.fromEnvironment('NAME') ??
             const String.fromEnvironment('HERO_NAME') ??
             'Hercules';

I know this constructor has a defaultValue: parameter, but this is more concise
(and will become idiomatic) and we would never have bothered with the
defaultValue: parameter if we had ?? first.

> 
> https://codereview.chromium.org/1493693002/diff/1/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/1493693002/diff/1/runtime/vm/parser.cc#newcod...
> runtime/vm/parser.cc:10555: if (lhs->IsPotentiallyConst()) {
> This is a bit more complicated.
> 
> You need to use lhs->EvalConstExpr() to determine whether the expression is a
> compile-time const. If that returns non-NULL, use FoldConstExpr() to evaluate
> the expression to get the actual value.
> 
> Also, background compilation has recently made all this more complex. The
> optimizing compiler runs in another thread and must not evaluate dart code.
> Thus, you probably have to cache the evaluated constant, similar to the code
> used in, for example, ParseExternalInitializedField().

[hausner, 2015-12-02 19:00:18]

That is only marginally different from

var name ??= String.fromEnvironment('NAME') ??
             String.fromEnvironment('HERO_NAME') ??
             'Hercules';

In both cases, the value is only calculated once.

[sra1, 2015-12-02 19:08:21]

On 2015/12/02 19:00:18, hausner wrote:
> That is only marginally different from
> 
> var name ??= String.fromEnvironment('NAME') ??
>              String.fromEnvironment('HERO_NAME') ??
>              'Hercules';
> 
> In both cases, the value is only calculated once.

I don't understand your point. In my example, 'name' is a constant and can be
used as an argument to a const constructor:

const wrapped = const Wrapper(name);

[Lasse Reichstein Nielsen, 2015-12-02 20:33:11]

I'm adding tests - and finding out the the VM changes are also not complete,
they don't cover const constructor initializer lists.

Const constructor initializer lists is one of my main reasons for wanting this:
  const C(foo) : this.foo = foo ?? someDefaultValue;
That has already happened a few times, and I have had to rewrite as:
  const C(foo) : this.foo = (foo != null) ? foo : someDefaultValue;
This is exactly the kind of expression ?? was introduced to help with, so it's
annoying when it doesn't work just because the expression is const.

About ?? and not ?. - the former is a shorthand for certain ?: expressions
containing only "== null" which are already const, while ?. is doing a member
access which is never const (no, not even length).

No DEP proposal. If the change is big enough to warrant one, I'll write one. It
won't be long (basically: ?: is const, ?? is a shorthand for ?:, so ?? should
also be const).

[floitsch, 2015-12-02 20:54:18]

On 2015/12/02 19:08:21, sra1 wrote:
> On 2015/12/02 19:00:18, hausner wrote:
> > That is only marginally different from
> > 
> > var name ??= String.fromEnvironment('NAME') ??
> >              String.fromEnvironment('HERO_NAME') ??
> >              'Hercules';
> > 
> > In both cases, the value is only calculated once.
> 
> I don't understand your point. In my example, 'name' is a constant and can be
> used as an argument to a const constructor:
> 
> const wrapped = const Wrapper(name);

The most common use-case is probably in the initializer-list of a
const-constructor:

const A(foo) : this.bar = foo ?? "not given";

[floitsch, 2015-12-02 20:59:26]

LGTM.
Couldn't find the changed spec changes, and Matthias already commented on the VM
changes.

https://codereview.chromium.org/1493693002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/constant.dart (right):

https://codereview.chromium.org/1493693002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/constant.dart:1588: return
_dartObjectComputer.questionQuestion(node, leftResult, rightResult);
long line

[Ivan Posva, 2015-12-03 21:23:00]

On 2015/12/02 20:59:26, floitsch wrote:
> LGTM.
> Couldn't find the changed spec changes, and Matthias already commented on the
VM
> changes.

Yes, Matthias commented and the way I read the comments they are far from a
LGTM. If anything it was clear that it needs more work.

-Ivan


>
https://codereview.chromium.org/1493693002/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/constant.dart (right):
> 
>
https://codereview.chromium.org/1493693002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/constant.dart:1588: return
> _dartObjectComputer.questionQuestion(node, leftResult, rightResult);
> long line

[Ivan Posva, 2015-12-03 21:29:04]

DBC

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/ast.cc#newco...
runtime/vm/ast.cc:377: if (left_val->IsNull()) return right_val;
{} as is the style elsewhere in this file.

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10555: if (lhs->IsPotentiallyConst() &&
lhs->EvalConstExpr() != NULL) {
() ditto: Please keep the style of the file.

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10565: if (expr_value.IsNull()) return rhs;
ditto

[Lasse Reichstein Nielsen, 2015-12-04 12:11:46]

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

https://codereview.chromium.org/1493693002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:38: A conforming  implementation of the Dart
programming language must provide and support all the  APIs (libraries, types,
functions, getters, setters, whether top-level, static, instance or local)
mandated in this specification.
White-space changes undone.
(Removing trailing whitespace on save is a great feature, but only if everybody
uses it :)

https://codereview.chromium.org/1493693002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/constant.dart (right):

https://codereview.chromium.org/1493693002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/constant.dart:1588: return
_dartObjectComputer.questionQuestion(node, leftResult, rightResult);
On 2015/12/02 20:59:26, floitsch wrote:
> long line

Done.

https://codereview.chromium.org/1493693002/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1493693002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:10555: if (lhs->IsPotentiallyConst()) {
Tried to do that.

Still doesn't work for const constructor initializer lists (but I guess there is
special code for that since it's only potentially constant).

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/ast.cc#newco...
runtime/vm/ast.cc:377: if (left_val->IsNull()) return right_val;
On 2015/12/03 21:29:04, Ivan Posva wrote:
> {} as is the style elsewhere in this file.

Done.

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10555: if (lhs->IsPotentiallyConst() &&
lhs->EvalConstExpr() != NULL) {
On 2015/12/03 21:29:04, Ivan Posva wrote:
> () ditto: Please keep the style of the file.

Done.

https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10565: if (expr_value.IsNull()) return rhs;
On 2015/12/03 21:29:04, Ivan Posva wrote:
> ditto

Done.

[floitsch, 2015-12-04 22:03:43]

On 2015/12/04 12:11:46, Lasse Reichstein Nielsen wrote:
>
https://codereview.chromium.org/1493693002/diff/1/docs/language/dartLangSpec.tex
> File docs/language/dartLangSpec.tex (right):
> 
>
https://codereview.chromium.org/1493693002/diff/1/docs/language/dartLangSpec....
> docs/language/dartLangSpec.tex:38: A conforming  implementation of the Dart
> programming language must provide and support all the  APIs (libraries, types,
> functions, getters, setters, whether top-level, static, instance or local)
> mandated in this specification.
> White-space changes undone.
> (Removing trailing whitespace on save is a great feature, but only if
everybody
> uses it :)
> 
>
https://codereview.chromium.org/1493693002/diff/1/pkg/analyzer/lib/src/genera...
> File pkg/analyzer/lib/src/generated/constant.dart (right):
> 
>
https://codereview.chromium.org/1493693002/diff/1/pkg/analyzer/lib/src/genera...
> pkg/analyzer/lib/src/generated/constant.dart:1588: return
> _dartObjectComputer.questionQuestion(node, leftResult, rightResult);
> On 2015/12/02 20:59:26, floitsch wrote:
> > long line
> 
> Done.
> 
> https://codereview.chromium.org/1493693002/diff/1/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/1493693002/diff/1/runtime/vm/parser.cc#newcod...
> runtime/vm/parser.cc:10555: if (lhs->IsPotentiallyConst()) {
> Tried to do that.
> 
> Still doesn't work for const constructor initializer lists (but I guess there
is
> special code for that since it's only potentially constant).
> 
> https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/ast.cc
> File runtime/vm/ast.cc (right):
> 
>
https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/ast.cc#newco...
> runtime/vm/ast.cc:377: if (left_val->IsNull()) return right_val;
> On 2015/12/03 21:29:04, Ivan Posva wrote:
> > {} as is the style elsewhere in this file.
> 
> Done.
> 
> https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc#ne...
> runtime/vm/parser.cc:10555: if (lhs->IsPotentiallyConst() &&
> lhs->EvalConstExpr() != NULL) {
> On 2015/12/03 21:29:04, Ivan Posva wrote:
> > () ditto: Please keep the style of the file.
> 
> Done.
> 
>
https://codereview.chromium.org/1493693002/diff/40001/runtime/vm/parser.cc#ne...
> runtime/vm/parser.cc:10565: if (expr_value.IsNull()) return rhs;
> On 2015/12/03 21:29:04, Ivan Posva wrote:
> > ditto
> 
> Done.

Spec changes LGTM (but wait for Gilad).

[Lasse Reichstein Nielsen, 2015-12-18 11:13:11]

lrn@google.com changed reviewers:
+ kasperl@google.com
- brianwilkerson@google.com, hausner@google.com, iposva@google.com,
rnystrom@google.com, sra@google.com

[Lasse Reichstein Nielsen, 2015-12-18 11:13:11]

Yes/no?

[gbracha, 2015-12-21 18:45:48]

On 2015/12/18 11:13:11, Lasse Reichstein Nielsen wrote:
> Yes/no?

AFIK, no for now. No minor language tweaks for a while.

[floitsch, 2016-01-11 15:44:06]

On 2015/12/21 18:45:48, gbracha wrote:
> On 2015/12/18 11:13:11, Lasse Reichstein Nielsen wrote:
> > Yes/no?
> 
> AFIK, no for now. No minor language tweaks for a while.

I would argue that we should still be able to fix oversights.
Especially, when it's a non-breaking change with implementations for all
platforms.

[gbracha, 2016-01-11 18:39:57]

On 2016/01/11 15:44:06, floitsch wrote:
> On 2015/12/21 18:45:48, gbracha wrote:
> > On 2015/12/18 11:13:11, Lasse Reichstein Nielsen wrote:
> > > Yes/no?
> > 
> > AFIK, no for now. No minor language tweaks for a while.
> 
> I would argue that we should still be able to fix oversights.
> Especially, when it's a non-breaking change with implementations for all
> platforms.

I believe we made a decision and should stick to it.  But feel free to take this
up with Kasper.

[gbracha, 2016-01-11 19:00:28]

lgtm

[Lasse Reichstein Nielsen, 2016-01-12 07:02:39]

Excellent. I'll split the VM and analyzer changes into separate CLs since they
need more work, and land the rest.

[Lasse Reichstein Nielsen, 2016-01-12 11:30:12]

Description was changed from

==========
Make ?? a compile-time constant operator.

Update specification, change analyzer, VM and dart2js.

Fixes issue #25054.
BUG= http://dartbug.com/25054
==========

to

==========
Make ?? a compile-time constant operator.

Update specification, change analyzer, VM and dart2js.

Fixes issue #25054.
BUG= http://dartbug.com/25054
R=floitsch@google.com, gbracha@google.com

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

[Lasse Reichstein Nielsen, 2016-01-12 11:30:14]

Committed patchset #5 (id:80001) manually as
fd59c87146cabc0ba359ad17d28ffe17cfa7cf19 (presubmit successful).