Create closed world for hello world using .dill file

Create closed world for hello world using .dill file

Currently results in compile time error because of a bug in the kernel reuse of mixin applications.

R=sigmund@google.com

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

[Johnni Winther, 2017-05-05 14:44:54]

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

[Johnni Winther, 2017-05-05 14:44:54]

Is patched_dart2js_sdk generated on the buildbots? If not, we need to add it
make this test work.

[Siggi Cherem (dart-lang), 2017-05-05 17:55:56]

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

[Siggi Cherem (dart-lang), 2017-05-05 17:55:57]

changes lgtm, but we need a .status file change before we can land it, and some
infrastructure changes to get this test covered:

(1) add dart2js unit tests without "--use-sdk" to the waterfall

    Today, bots that runs dart2js unit tests don't build dart, they download an
SDK built by the other bots. Such SDK doesn't have `patched_dart2js_sdk` and we
don't plan to add it there.

    I'm thinking we need a bot that builds dart to run the dart2js unit tests.
Today the dart2js-d8-hostchecked bots do build dart directly, so we could add
the tests there.


(2) add patched_dart2js_sdk to some .gn rule that gets built by the bots. 

    The patched_dart2js_sdk target is not built anywhere by default. We can add
it either to 'runtime' (so it is built by default with ./tools/build.py with no
args) or in 'create_sdk' (so it gets noticed by sdk library maintainers), or
just in 'dart2js_bot' in the root BUILD.gn file.

    I am inclined to just do "dart2js_bot" for now.

(3) Update .status file to exclude the dill test when "$use_sdk" is true. We
should do it with this CL.


Bill - does (1) and (2) seem like a good plan?

https://codereview.chromium.org/2865693002/diff/1/tests/compiler/dart2js/kern...
File tests/compiler/dart2js/kernel/compiler_helper.dart (right):

https://codereview.chromium.org/2865693002/diff/1/tests/compiler/dart2js/kern...
tests/compiler/dart2js/kernel/compiler_helper.dart:158: /*ProcessResult
processResult = await Process.runSync(
delete comment?

[Johnni Winther, 2017-05-15 14:14:34]

johnniwinther@google.com changed reviewers:
- whesse@google.com

[Johnni Winther, 2017-05-15 14:14:34]

Updated to match the latest .dill files. Temporarily renamed
`closed_world_from_dill.dart` until the build bot has a patched_dart2js_sdk.

[Johnni Winther, 2017-05-15 14:14:47]

PTAL

[Siggi Cherem (dart-lang), 2017-05-15 17:05:50]

lgtm with suggestions below

https://codereview.chromium.org/2865693002/diff/40001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/enum_creator.dart (right):

https://codereview.chromium.org/2865693002/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/enum_creator.dart:204: static bool
USE_CONSTANT_MAP_IN_TO_STRING = false;
a few nits:
- just use the common camelCaseNotation
(https://www.dartlang.org/guides/language/effective-dart/style#prefer-using-lo...)

- I'd be nice to say "testing" in the field name, some ideas:
   - useConstantMapImplementationForTesting
   - implementToStringViaConstantMapForTesting
   - matchKernelRepresentationForTesting

- update the dartdoc to indicate what corresponds to the spec-based encoding or
what matches kernel. For example:

/// Whether to use a constant-map to encode the result of toString on an enum
value.
///
/// Dart2js by default uses a compact representation of enum values that include
their name. The spec and the kernel-based implementation use a const-map to
encode the result of toString. This flag is used to disable the default
representation, and instead match the kernel representation to make it easier to
test the equivalence of resolution-based and kernel-based world builders.

https://codereview.chromium.org/2865693002/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/enum_creator.dart:295:
valueBuilder.argumentList([valueBuilder.literalInt(index)]),
reuse indexValue

https://codereview.chromium.org/2865693002/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/kernel/closed_world_from_dill.dart (right):

https://codereview.chromium.org/2865693002/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/kernel/closed_world_from_dill.dart:7: //
TODO(johnniwinther): Rename this file to `closed_wordl_from_dill_test.dart`
I rather keep this as a test name so you can run it locally, but add to the
.status file an exception:

[ $use_sdk ]
dart2js/closed_world_from_dill_test: Fail # Issue XYZ

https://codereview.chromium.org/2865693002/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/kernel/compiler_helper.dart (right):

https://codereview.chromium.org/2865693002/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/kernel/compiler_helper.dart:159: /*ProcessResult
processResult = await Process.runSync(
delete commented code

[Johnni Winther, 2017-05-16 08:29:55]

https://codereview.chromium.org/2865693002/diff/40001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/enum_creator.dart (right):

https://codereview.chromium.org/2865693002/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/enum_creator.dart:204: static bool
USE_CONSTANT_MAP_IN_TO_STRING = false;
On 2017/05/15 17:05:50, Siggi Cherem (dart-lang) wrote:
> a few nits:
> - just use the common camelCaseNotation
>
(https://www.dartlang.org/guides/language/effective-dart/style#prefer-using-lo...)
> 
> - I'd be nice to say "testing" in the field name, some ideas:
>    - useConstantMapImplementationForTesting
>    - implementToStringViaConstantMapForTesting
>    - matchKernelRepresentationForTesting
> 
> - update the dartdoc to indicate what corresponds to the spec-based encoding
or
> what matches kernel. For example:
> 
> /// Whether to use a constant-map to encode the result of toString on an enum
> value.
> ///
> /// Dart2js by default uses a compact representation of enum values that
include
> their name. The spec and the kernel-based implementation use a const-map to
> encode the result of toString. This flag is used to disable the default
> representation, and instead match the kernel representation to make it easier
to
> test the equivalence of resolution-based and kernel-based world builders.

Done.

https://codereview.chromium.org/2865693002/diff/40001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/enum_creator.dart:295:
valueBuilder.argumentList([valueBuilder.literalInt(index)]),
On 2017/05/15 17:05:50, Siggi Cherem (dart-lang) wrote:
> reuse indexValue

Done.

https://codereview.chromium.org/2865693002/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/kernel/closed_world_from_dill.dart (right):

https://codereview.chromium.org/2865693002/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/kernel/closed_world_from_dill.dart:7: //
TODO(johnniwinther): Rename this file to `closed_wordl_from_dill_test.dart`
On 2017/05/15 17:05:50, Siggi Cherem (dart-lang) wrote:
> I rather keep this as a test name so you can run it locally, but add to the
> .status file an exception:
> 
> [ $use_sdk ]
> dart2js/closed_world_from_dill_test: Fail # Issue XYZ

Done.

https://codereview.chromium.org/2865693002/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/kernel/compiler_helper.dart (right):

https://codereview.chromium.org/2865693002/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/kernel/compiler_helper.dart:159: /*ProcessResult
processResult = await Process.runSync(
On 2017/05/15 17:05:50, Siggi Cherem (dart-lang) wrote:
> delete commented code

Done.

[Johnni Winther, 2017-05-16 08:30:16]

Description was changed from

==========
Create closed world for hello world using .dill file

Currently results in compile time error because of a bug in the kernel reuse of
mixin applications.
==========

to

==========
Create closed world for hello world using .dill file

Currently results in compile time error because of a bug in the kernel reuse of
mixin applications.

R=sigmund@google.com

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

[Johnni Winther, 2017-05-16 08:30:18]

Committed patchset #4 (id:60001) manually as
dff23682a1317c7e25e0662fd5bd02ddaf701177 (presubmit successful).