Add assert-in-initializer-list to formal specification.

Add assert-in-initializer-list to formal specification.

Add tests for new syntax.
Not dart-formatted yet since the formatter doesn't accept the new syntax.

R=eernst@google.com, floitsch@google.com

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

[Lasse Reichstein Nielsen, 2017-06-29 12:03:49]

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

[Lasse Reichstein Nielsen, 2017-06-29 12:03:49]

You should be able to remove the flags and enable the feature by default.

[floitsch, 2017-06-29 12:08:57]

LGTM.

https://codereview.chromium.org/2962153002/diff/1/tests/language/assertion_in...
File tests/language/assertion_initializer_const_error2_test.dart (right):

https://codereview.chromium.org/2962153002/diff/1/tests/language/assertion_in...
tests/language/assertion_initializer_const_error2_test.dart:29: const C.cc01(2,
1);  //# cc01: compile-time error
I would put the `//#` lines on the "assert" lines:

const C.cc01(this.x, y)
  : assert(x < y)  //# cc01: compile-time error
;

https://codereview.chromium.org/2962153002/diff/1/tests/language/assertion_in...
File tests/language/assertion_initializer_const_error_test.dart (right):

https://codereview.chromium.org/2962153002/diff/1/tests/language/assertion_in...
tests/language/assertion_initializer_const_error_test.dart:12: const
C.bc03(this.x, y) : assert(check(x, y));  //# 01: compile-time error
ditto: move the "assert(check(x, y))" on a separate line. Then leave the call
below unaffected.

https://codereview.chromium.org/2962153002/diff/1/tests/language/assertion_in...
File tests/language/assertion_initializer_const_function_error_test.dart
(right):

https://codereview.chromium.org/2962153002/diff/1/tests/language/assertion_in...
tests/language/assertion_initializer_const_function_error_test.dart:16: const
C.bc02(this.x, y) : assert(staticTrue); //# 01: static type warning
ditto: move 'assert' into a separate line.

[eernst, 2017-07-03 16:36:38]

LGTM. A few typos noted. The only real issue is that we might want to make
`assert(staticTrue)` in a constructor declaration a compile-time error in
itself, which would change a few things. Also, some status entries look like
they are bugs waiting for a fix, but I think they're "by design".

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6759: comma removed.
The two most recent sentences are redundant. I'd suggest that we simply delete
`That comma, if present, has no effect.`

Typo: `comme` --> `comma`.

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6781: then we say that the assertion failed,
Cf. the comments on assertion_initializer_const_function_error_test.dart: I
suspect that we ought to make this an error in its own right, rather than making
it an assertion failure.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_error2_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_error2_test.dart:1: // Copyright (c)
201, the Dart project authors.  Please see the AUTHORS file
Typo: `2017`

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_error_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_error_test.dart:1: // Copyright (c)
201, the Dart project authors.  Please see the AUTHORS file
Typo: `2017`.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_error_test.dart:18: c = new C.bc03(1,
2);  //# 01: continued
It's a tricky twist that (1) `new C.bc03` may not be compiled even though an
instance of `C` is created, so we have to include something like line 18, and
(2) using this `const` constructor in a `new` expression is sufficient to ensure
that the constructor is compiled, and hence that the checks on its initializers
are performed.

But I guess this subtle way to achieve the compile-time error is justified
because it tests another path in the tools than a plain `const C.bc03(1,2)`
would have done.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_function_error_test.dart
(right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_error_test.dart:1: //
Copyright (c) 201, the Dart project authors.  Please see the AUTHORS file
Typo: `2017`.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_error_test.dart:16: const
C.bc02(this.x, y) : assert(staticTrue); //# 01: static type warning
It's weird that we get a static type warning (at compile time, of course) at the
declaration of the constructor (cf. line 6793), and then (also at compile time)
an assertion error at each `const` invocation of the constructor (cf. line
6781). It is as if we had evaluated a user-defined correctness check and failed,
but we did in fact encounter an assertion which is mal-formed.

Maybe we should simply make it a compile-time error to have a function as an
argument to the assert in a const constructor initializer?

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_error_test.dart:23: //
Production mode will succeed because the assertion isn't evaluated.
But then this test will fail by design in production mode (missing compile time
error). Maybe it should be marked as skipByDesign? (A related comment was added
below on the status file).

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_function_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_test.dart:1: // Copyright
(c) 201, the Dart project authors.  Please see the AUTHORS file
Typo: `2017`.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_test.dart:23:
Expect.throws(() => new C.bc01(1, 2), (e) => e is AssertionError);  //# 01:
continued
This test file as a whole would go away if we change the treatment of
`assert(staticTrue)` to make it a compile-time error for the declaration of
C.bc01.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_test.dart:1: // Copyright (c) 201, the Dart
project authors.  Please see the AUTHORS file
Typo: `2017`.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/language...
File tests/language/language.status (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/language...
tests/language/language.status:24: [ !$checked && $compiler == none ]
Why isn't this block of status entries ending in `, OK`, based on the same
reasoning as above? Actually, `MissingCompileTimeError` would be OK, but `Fail`
would not.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/language...
tests/language/language.status:218:
assertion_initializer_const_function_error_test/01: MissingCompileTimeError
The `..const_function_error_test` status would be `, OK` because
`assert(staticTrue)` is a static warning in the constructor declaration and
ignored for the compile-time evaluation of `const` expressions.

[Lasse Reichstein Nielsen, 2017-07-04 05:36:48]

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6759: comma removed.
On 2017/07/03 16:36:37, eernst wrote:
> The two most recent sentences are redundant. I'd suggest that we simply delete
> `That comma, if present, has no effect.`
> 
> Typo: `comme` --> `comma`.

Acknowledged.

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6781: then we say that the assertion failed,
I can see why we'd want that - fail early, since it'll always fail at runtime in
checked mode. I just don't think it fits in Dart 1.
I don't want to introduce type-based compile-time errors in Dart 1.

It's a *potentially* compile-time constant expression which happens to not be a
Boolean value. It's actually a compile-time constant expression here, but it
could have been an argument variables instead. 
In checked mode, it will be a compile-time error to const-instantiate the
constructor, and the analyzer is analyzing checked mode (AFAIK), so you will
detect it if it makes any difference.

The only case that won't be caught is when the constructor is never used with
const. In that case, we cannot reason about the values, only about the static
type of the variable, and that means that:

bool staticFunction() => true;
const Object variable = staticFunction;
class C() {
  const C(): assert(variable); // not error
}

This may or may not cause an error (Object is assignable to bool, is that the
test? Or must it be bool?), but it depends on the type annotation on the
`variable` declaration, which Dart 1 must not.

(I'll add tests for the case where the entire expression is an argument too)

[eernst, 2017-07-04 09:09:21]

Still LGTM, noting that several comments have not yet been processed.

Agreed on keeping static warning (rather than going to compile-time error) for
certain constructor declarations, but suggesting changes to spec to allow a
clear message "your code is wrong" rather than "your assertion expression
returned false".

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6781: then we say that the assertion failed,
On 2017/07/04 05:36:48, Lasse Reichstein Nielsen wrote:
> I can see why we'd want that - fail early, since it'll always fail at runtime
in
> checked mode. I just don't think it fits in Dart 1.
> I don't want to introduce type-based compile-time errors in Dart 1.
> 
> It's a *potentially* compile-time constant expression which happens to not be
a
> Boolean value. It's actually a compile-time constant expression here, but it
> could have been an argument variables instead. 
> In checked mode, it will be a compile-time error to const-instantiate the
> constructor, and the analyzer is analyzing checked mode (AFAIK), so you will
> detect it if it makes any difference.
> 
> The only case that won't be caught is when the constructor is never used with
> const. In that case, we cannot reason about the values, only about the static
> type of the variable, and that means that:
> 
> bool staticFunction() => true;
> const Object variable = staticFunction;
> class C() {
>   const C(): assert(variable); // not error
> }
> 
> This may or may not cause an error (Object is assignable to bool, is that the
> test? Or must it be bool?), but it depends on the type annotation on the
> `variable` declaration, which Dart 1 must not.
> 
> (I'll add tests for the case where the entire expression is an argument too)

OK, "don't introduce type-based compile-time errors in Dart 1"
is a strong argument for not having these compile-time errors on
non-bools. So let's stick to that.

The remaining issue is that it is confusing that we get an assertion
failure (when it occurs, i.e., at compile-time when the `const`
instance creation expression is evaluated). That seems to send the 
message "your domain specific correctness check returned false", but
it should send the message "the compile-time evaluation failed because 
that code is wrong" (maybe "it tried to use a function as a bool",
focusing on actual values as in the spec, but in any case we need to
say that the code was plain wrong, not that it returned false).

The spec has only one way out for constant expressions: It is a compile-time
error if 'its evaluation would throw'. That may happen because of
an assertion error or it may be things like `NoSuchMethodError`
from `null + 2`, etc.

So we could say 'if the assertion occurs in the initializer list of
a \CONST{} constructor, an exception is thrown (\ref{completion})
with a stack trace corresponding to the current execution state.'

We may or may not wish to specify the precise type for that exception
(the spec mentions 8 such types), but in any case it doesn't have to
be `AssertionError`.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_function_error_test.dart
(right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_error_test.dart:16: const
C.bc02(this.x, y) : assert(staticTrue); //# 01: static type warning
On 2017/07/03 16:36:37, eernst wrote:
> It's weird that we get a static type warning (at compile time, of course) at
the
> declaration of the constructor (cf. line 6793), and then (also at compile
time)
> an assertion error at each `const` invocation of the constructor (cf. line
> 6781). It is as if we had evaluated a user-defined correctness check and
failed,
> but we did in fact encounter an assertion which is mal-formed.
> 
> Maybe we should simply make it a compile-time error to have a function as an
> argument to the assert in a const constructor initializer?

Next iteration: I'm no longer proposing that we make the declaration a
compile-time error: No changes requested here.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_function_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_test.dart:23:
Expect.throws(() => new C.bc01(1, 2), (e) => e is AssertionError);  //# 01:
continued
On 2017/07/03 16:36:37, eernst wrote:
> This test file as a whole would go away if we change the treatment of
> `assert(staticTrue)` to make it a compile-time error for the declaration of
> C.bc01.

Next iteration: We keep `assert(staticTrue)` as a static warning and not an
error. When reaching this point, however, the failure might not be an
`AssertionError` (it's misleading, so I'm proposing to make it some other
error).

[Lasse Reichstein Nielsen, 2017-07-04 09:42:18]

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6781: then we say that the assertion failed,
Good point. How about:
---
If the class of $o$ is a subtype of \code{Function} and the assertion does not
occur in the initializer list of a CONST{} constructor, then
let $r$ be the result of invoking $o$ with no argument, otherwise let $r$ be
$o$.

It is a dynamic type error if $r$ is not of type \code{bool}.
---
This way the error will be a TypeError, not a *plain* AssertionError (TypeError
is-an AssertionError) saying that "this function type is not a bool".

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_function_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_test.dart:1: // Copyright
(c) 201, the Dart project authors.  Please see the AUTHORS file
good catch

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_test.dart:23:
Expect.throws(() => new C.bc01(1, 2), (e) => e is AssertionError);  //# 01:
continued
I was actually expecting this to be a type error. I can see that I failed to
make it one.

Asserts are only enabled in checked mode, and the problem is that the value is
not a bool.
If I fix the spec, then this will properly throw in checked mode. 

I don't know if that's exactly what the VM does, but it should be possible to
fix.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/language...
File tests/language/language.status (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/language...
tests/language/language.status:24: [ !$checked && $compiler == none ]
I'll get back to the status files. They are still not correct, and my tool was
misleading me on how to fix them :)

[eernst, 2017-07-04 09:45:48]

LGTM

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6781: then we say that the assertion failed,
On 2017/07/04 09:42:17, Lasse Reichstein Nielsen wrote:
> Good point. How about:
> ---
> If the class of $o$ is a subtype of \code{Function} and the assertion does not
> occur in the initializer list of a CONST{} constructor, then
> let $r$ be the result of invoking $o$ with no argument, otherwise let $r$ be
> $o$.
> 
> It is a dynamic type error if $r$ is not of type \code{bool}.
> ---
> This way the error will be a TypeError, not a *plain* AssertionError
(TypeError
> is-an AssertionError) saying that "this function type is not a bool".

I like that!

[Lasse Reichstein Nielsen, 2017-07-10 10:57:03]

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2962153002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6759: comma removed.
On 2017/07/04 05:36:48, Lasse Reichstein Nielsen wrote:
> On 2017/07/03 16:36:37, eernst wrote:
> > The two most recent sentences are redundant. I'd suggest that we simply
delete
> > `That comma, if present, has no effect.`
> > 
> > Typo: `comme` --> `comma`.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_error2_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_error2_test.dart:1: // Copyright (c)
201, the Dart project authors.  Please see the AUTHORS file
On 2017/07/03 16:36:37, eernst wrote:
> Typo: `2017`

Done.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_error_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_error_test.dart:1: // Copyright (c)
201, the Dart project authors.  Please see the AUTHORS file
On 2017/07/03 16:36:37, eernst wrote:
> Typo: `2017`.

Done.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_error_test.dart:18: c = new C.bc03(1,
2);  //# 01: continued
I moved the //#01 to the assert itself.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_const_function_error_test.dart
(right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_error_test.dart:16: const
C.bc02(this.x, y) : assert(staticTrue); //# 01: static type warning
The interesting thing is that a maltyped and failing assert statement is a
compile-time error in *checked* mode, but a static-warning in production mode.
We actually do have to handle both.
I think only status files can do that for us.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_const_function_error_test.dart:23: //
Production mode will succeed because the assertion isn't evaluated.
It should be a "static type warning" in production mode. The analyzer should
detect that the type is wrong.

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
File tests/language/assertion_initializer_test.dart (right):

https://codereview.chromium.org/2962153002/diff/20001/tests/language/assertio...
tests/language/assertion_initializer_test.dart:1: // Copyright (c) 201, the Dart
project authors.  Please see the AUTHORS file
On 2017/07/03 16:36:37, eernst wrote:
> Typo: `2017`.

Done.

[eernst, 2017-07-10 11:05:49]

LGTM as usual, just noting that
assertion_initializer_const_function_error_test.dart is still copyright 201.

[Lasse Reichstein Nielsen, 2017-07-10 11:59:42]

Description was changed from

==========
Add assert-in-initializer-list to formal specification.

Add tests for new syntax.
Not dart-formatted yet since the formatter doesn't accept the new syntax.
==========

to

==========
Add assert-in-initializer-list to formal specification.

Add tests for new syntax.
Not dart-formatted yet since the formatter doesn't accept the new syntax.

R=eernst@google.com, floitsch@google.com

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

[Lasse Reichstein Nielsen, 2017-07-10 11:59:44]

Committed patchset #5 (id:80001) manually as
609d26a2274ccde0f74725f4df7e081ebc8ea020.

[floitsch, 2017-07-10 13:44:29]

Description was changed from

==========
Add assert-in-initializer-list to formal specification.

Add tests for new syntax.
Not dart-formatted yet since the formatter doesn't accept the new syntax.

R=eernst@google.com, floitsch@google.com

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

to

==========
Add assert-in-initializer-list to formal specification.

Add tests for new syntax.
Not dart-formatted yet since the formatter doesn't accept the new syntax.

R=eernst@google.com, floitsch@google.com

Committed:
https://github.com/dart-lang/sdk/commit/609d26a2274ccde0f74725f4df7e081ebc8ea020
Reverted:
https://github.com/dart-lang/sdk/commit/5d4a786f4466ebcb06c36ed44f262c9e120a7273
==========

[floitsch, 2017-07-10 13:45:43]

Description was changed from

==========
Add assert-in-initializer-list to formal specification.

Add tests for new syntax.
Not dart-formatted yet since the formatter doesn't accept the new syntax.

R=eernst@google.com, floitsch@google.com

Committed:
https://github.com/dart-lang/sdk/commit/609d26a2274ccde0f74725f4df7e081ebc8ea020
Reverted:
https://github.com/dart-lang/sdk/commit/5d4a786f4466ebcb06c36ed44f262c9e120a7273
==========

to

==========
Add assert-in-initializer-list to formal specification.

Add tests for new syntax.
Not dart-formatted yet since the formatter doesn't accept the new syntax.

R=eernst@google.com, floitsch@google.com

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