Add support to dart2js for option --enable-asserts.

Add support to dart2js for option --enable-asserts.

R=johnniwinther@google.com, sigmund@google.com, sra@google.com

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

[eernst, 2017-05-15 17:24:56]

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

[eernst, 2017-05-15 17:24:56]

Feature requested in SDK issue 29601, may or may not be landed.

We have not decided to add this feature, but it is my impression that it may be
helpful, and this CL serves to illustrate how it could be done.

A sanity check on the approach would be appreciated.

[eernst, 2017-05-15 17:50:57]

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

[eernst, 2017-05-15 17:50:58]

Sigmund,

do you think it is a good or a bad idea to add this little feature to dart2js?
The idea comes from Mike Mitterer, and I thought it made sense. I've tried to
motivate it a bit in the issue https://github.com/dart-lang/sdk/issues/29601,
and the original motivation arose in
https://github.com/dart-lang/pub/issues/932.

[sra1, 2017-05-15 20:18:43]

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

[sra1, 2017-05-15 20:18:43]

DBC.

I have some misgivings about the CL.

Mainly, if we support the flag, we must have specific test coverage for the
feature and buildbot configurations that test it.

Tests are not run on the VM in the --enable-asserts only mode. This means that
it is likely that there is coupling between the type assertions and assertion
statements in the codebase. There will be assertion statements that crash in the
condition, and assertion statements that check something unintended.

One reason we don't use the separate undocumented flags on the VM is because, in
practice, the VM --checked mode is 'fast enough'.
Is there a performance target where dart2js full --checked mode would also be
'fast enough'?

I think there is a remedy (the closure hack, if-statements) where the user can
have conditional checking but we do not have to substantially increase our test
coverage.

https://codereview.chromium.org/2879153005/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/2879153005/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:23: static const String
enableAssert = '--enable-assert';
The VM has undocumented options:

    --enable-asserts       // ends in 's'.
    --enable-type-checks

If we proceed with this CL, we should support consistent command-line options
across all compilers.

[Johnni Winther, 2017-05-16 07:38:49]

lgtm

https://codereview.chromium.org/2879153005/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/2879153005/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:23: static const String
enableAssert = '--enable-assert';
On 2017/05/15 20:18:43, sra1 wrote:
> The VM has undocumented options:
> 
>     --enable-asserts       // ends in 's'.
>     --enable-type-checks
> 
> If we proceed with this CL, we should support consistent command-line options
> across all compilers.

Acknowledged.

[floitsch, 2017-05-16 08:53:23]

On 2017/05/16 07:38:49, Johnni Winther wrote:
> lgtm
> 
>
https://codereview.chromium.org/2879153005/diff/1/pkg/compiler/lib/src/comman...
> File pkg/compiler/lib/src/commandline_options.dart (right):
> 
>
https://codereview.chromium.org/2879153005/diff/1/pkg/compiler/lib/src/comman...
> pkg/compiler/lib/src/commandline_options.dart:23: static const String
> enableAssert = '--enable-assert';
> On 2017/05/15 20:18:43, sra1 wrote:
> > The VM has undocumented options:
> > 
> >     --enable-asserts       // ends in 's'.
> >     --enable-type-checks
> > 
> > If we proceed with this CL, we should support consistent command-line
options
> > across all compilers.
> 
> Acknowledged.

DBC: once strong mode is the default, there isn't any --checked anymore. At that
point we need to have a different flag for the asserts.
We can either use '--enable-asserts', or '--debug' (or similar). If we go with
'--enable-asserts', then landing this CL now, is probably ok. Otherwise, I would
maybe wait until we have the new set of flags sorted out.

[eernst, 2017-05-16 17:03:58]

Description was changed from

==========
Add support to dart2js for option --enable-assert.
==========

to

==========
Add support to dart2js for option --enable-asserts.
==========

[Siggi Cherem (dart-lang), 2017-05-16 21:50:55]

lgtm

+1 to Stephen's comments. I'm in favor of it, I prefer the name with "s" for
consistency, but I would also like more test coverage before we advertise this
flag (at least for our own sdk libraries)

[eernst, 2017-05-19 07:18:55]

It sounds like we'll be doing this, so I took a few more steps.

This CL adjusts a bunch of tests such that their test for checked mode uses a
type violation rather than an assert. So they will conclude that a run is in
checked mode with --checked, where they used to do so with --enable-asserts as
well as --checked. They used to fail under --enable-asserts because of missing
type errors, and this fixes that problem.

The CL adds support for `--enable-asserts` to 'tools/test.py', and made it such
that `$enable_asserts` and `$no_enable_asserts` can be used in status file
`[...]` expressions.

The CL marks co19 Assert tests that fail because they follow the spec strictly
(and `--enable-asserts` semantics doesn't) as `SkipByDesign` in the relevant
status file. If we choose to spec this mode then we can adjust later, but for
now this will allow the bots to remain green even if an --enable-asserts run is
included.

A couple of tests needed more detailed treatment: language/assertion_test and
language/bool_check_test have status RuntimeError in a different set of cases
(see issue #29647); language/type_error_test was adjusted to distinguish checked
mode checks and assertion checks, rather than assuming that they would always be
in sync.

We may want to add a buildbot running with --enable-asserts, and we should be
ready for that:

> tools/test.py --host-checked -mrelease -cdart2js -rd8 \
        --dart2js-batch -t960 -j20 --enable-asserts
Test configurations: dart2js_d8_release_x64 dart2js_d8_release_x64
[78:43 | 100% | +17937 | -    0]

[eernst, 2017-05-19 09:51:20]

PTAL, in particular Stephen who's the most skeptical person so far: Are you
happy with the CL at this point?

[sra1, 2017-05-19 16:51:35]

I'm ok with going ahead.
I think the suggested package:expect cleanup could be done in advance of landing
the dart2js and testing changes.

There are some unittest tests which have similar logic.
They could import package:expect and 'show' the predicate(s).

https://codereview.chromium.org/2879153005/diff/80001/tests/language/function...
File tests/language/function_type/function_type10_test.dart (right):

https://codereview.chromium.org/2879153005/diff/80001/tests/language/function...
tests/language/function_type/function_type10_test.dart:30: })();
Looking at this large duplication I think we should add this logic to
package:expect and remove it from all the tests.
We should be precise with the names and have the name be correct for all
implementations.

final bool typeAssertionsEnabled = ...
final bool assertStatementsEnabled = ...

if we also need inCheckedMode, this would mean `--checked` which is a
combination of the two (on the VM you can control them independently).

final bool inCheckedMode = typeAssertionsEnabled && assertStatementsEnabled;

If you have different names that are a little more future-proof that would be
ok.

https://codereview.chromium.org/2879153005/diff/80001/tools/testing/dart/test...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/2879153005/diff/80001/tools/testing/dart/test...
tools/testing/dart/test_options.dart:222: 'Pass the --enable-asserts flag to
dart2js',
This should be made to work for VM tests too.

[eernst, 2017-05-23 17:09:55]

Added typeAssertionsEnabled, assertStatementsEnabled, checkedModeEnabled to
expect.dart and used them in all tests affected by this CL. Generalized the
`--enable-asserts` option on test.py to work for the vm, too. Based on the
feedback (L-G-T-Ms etc.) at this point, I will land this tomorrow (Wednesday,
CEST).

https://codereview.chromium.org/2879153005/diff/1/pkg/compiler/lib/src/comman...
File pkg/compiler/lib/src/commandline_options.dart (right):

https://codereview.chromium.org/2879153005/diff/1/pkg/compiler/lib/src/comman...
pkg/compiler/lib/src/commandline_options.dart:23: static const String
enableAssert = '--enable-assert';
On 2017/05/16 07:38:49, Johnni Winther wrote:
> On 2017/05/15 20:18:43, sra1 wrote:
> > The VM has undocumented options:
> > 
> >     --enable-asserts       // ends in 's'.
> >     --enable-type-checks
> > 
> > If we proceed with this CL, we should support consistent command-line
options
> > across all compilers.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2879153005/diff/80001/tests/language/function...
File tests/language/function_type/function_type10_test.dart (right):

https://codereview.chromium.org/2879153005/diff/80001/tests/language/function...
tests/language/function_type/function_type10_test.dart:30: })();
On 2017/05/19 16:51:35, sra1 wrote:
> Looking at this large duplication I think we should add this logic to
> package:expect and remove it from all the tests.

The duplication is slightly deceptive here because
function_type{0,1,..,99}_test.dart are generated. But it's certainly more
readable to have working definitions of `typeAssertionsEnabled` and
`assertStatementsEnabled` (and I called the third one `checkedModeEnabled`) in a
shared location, and then use that everywhere.

I've added these declarations to pkg/expect/lib/expect.dart and adjusted all
tests touched by this CL to use them.

> We should be precise with the names and have the name be correct for all
> implementations.
> 
> final bool typeAssertionsEnabled = ...
> final bool assertStatementsEnabled = ...
> 
> if we also need inCheckedMode, this would mean `--checked` which is a
> combination of the two (on the VM you can control them independently).
> 
> final bool inCheckedMode = typeAssertionsEnabled && assertStatementsEnabled;
> 
> If you have different names that are a little more future-proof that would be
> ok.

I think it makes sense to use those names: They are long so they don't easily
clash with other things, and they are used relatively rarely so it is important
that they are highly self-explanatory.

https://codereview.chromium.org/2879153005/diff/80001/tools/testing/dart/test...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/2879153005/diff/80001/tools/testing/dart/test...
tools/testing/dart/test_options.dart:222: 'Pass the --enable-asserts flag to
dart2js',
On 2017/05/19 16:51:35, sra1 wrote:
> This should be made to work for VM tests too.

Done.

[sra1, 2017-05-24 01:30:46]

LGTM

https://codereview.chromium.org/2879153005/diff/100001/tests/co19/co19-dart2j...
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/2879153005/diff/100001/tests/co19/co19-dart2j...
tests/co19/co19-dart2js.status:377: [ $compiler == dart2js && $enable_asserts ]
So the problem here is not that we have --enable-asserts, it is that we don't
have --checked (--enable-type-assertions).

Can we add something like !$checked to strengthen the condition.

If not, I would need a comment to say  $enable_asserts implies type assertions
are disabled to understand what is going on here.

https://codereview.chromium.org/2879153005/diff/100001/tests/corelib/int_pars...
File tests/corelib/int_parse_radix_test.dart (right):

https://codereview.chromium.org/2879153005/diff/100001/tests/corelib/int_pars...
tests/corelib/int_parse_radix_test.dart:16: })();
delete

https://codereview.chromium.org/2879153005/diff/100001/tests/corelib/list_rem...
File tests/corelib/list_removeat_test.dart (right):

https://codereview.chromium.org/2879153005/diff/100001/tests/corelib/list_rem...
tests/corelib/list_removeat_test.dart:31: })();
delete

[eernst, 2017-05-24 10:18:57]

Taking Johnni's advice, it's more important to avoid rocking the boat right now
than it is to get this CL into the 1.24 release, so I'll wait until I know that
we're past that release.

[eernst, 2017-07-06 16:15:23]

This CL was delayed in May (in order to avoid problems with the upcoming
release). It has now been ported to the current master (which required several
changes due to updates in tools/testing/dart) and is again working.

If you wish to take another look, note that you can ignore almost all the
`function_type/function_type*_test.dart` changes: These files were regenerated
(`dart test_generator.dart`) and formatted (`dartfmt -w`), so they have all been
changed in "the same way".

[Siggi Cherem (dart-lang), 2017-07-06 17:27:56]

lgtm

It would be nice if we could address Stephen's comment on the .status file.

[eernst, 2017-07-07 08:43:42]

On 2017/07/06 17:27:56, Siggi Cherem (dart-lang) wrote:
> lgtm
> 
> It would be nice if we could address Stephen's comment on the .status file.

Thanks for catching that, done!

[eernst, 2017-07-07 08:50:30]

Review response.

https://codereview.chromium.org/2879153005/diff/100001/tests/co19/co19-dart2j...
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/2879153005/diff/100001/tests/co19/co19-dart2j...
tests/co19/co19-dart2js.status:377: [ $compiler == dart2js && $enable_asserts ]
On 2017/05/24 01:30:46, sra1 wrote:
> So the problem here is not that we have --enable-asserts, it is that we don't
> have --checked (--enable-type-assertions).
> 
> Can we add something like !$checked to strengthen the condition.
> 
> If not, I would need a comment to say  $enable_asserts implies type assertions
> are disabled to understand what is going on here.

$enable_asserts does not imply that type assertions are disabled, so this is a
problem. A similar situation exists in language/language-dart2js.status, but
when I fixed that I forgot to fix this one. Done.

https://codereview.chromium.org/2879153005/diff/100001/tests/corelib/int_pars...
File tests/corelib/int_parse_radix_test.dart (right):

https://codereview.chromium.org/2879153005/diff/100001/tests/corelib/int_pars...
tests/corelib/int_parse_radix_test.dart:16: })();
On 2017/05/24 01:30:46, sra1 wrote:
> delete

Done.

https://codereview.chromium.org/2879153005/diff/100001/tests/corelib/list_rem...
File tests/corelib/list_removeat_test.dart (right):

https://codereview.chromium.org/2879153005/diff/100001/tests/corelib/list_rem...
tests/corelib/list_removeat_test.dart:31: })();
On 2017/05/24 01:30:46, sra1 wrote:
> delete

Done.

[eernst, 2017-07-07 08:59:38]

Description was changed from

==========
Add support to dart2js for option --enable-asserts.
==========

to

==========
Add support to dart2js for option --enable-asserts.

R=johnniwinther@google.com, sigmund@google.com, sra@google.com

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

[eernst, 2017-07-07 08:59:40]

Committed patchset #9 (id:140001) manually as
3b05eb95530b1684a3fbcc69bda1784db393b966 (presubmit successful).