Refactor BackendHelpers to be reusageable with KernelWorldBuilder

Refactor BackendHelpers to be reusageable with KernelWorldBuilder

- introduces ElementEnvironment and uses it to implement most of CommonElements and BackendHelpers

R=sigmund@google.com

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

[Johnni Winther, 2017-02-06 13:45:40]

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

[Siggi Cherem (dart-lang), 2017-02-06 17:15:59]

lgtm

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/core_t...
File pkg/compiler/lib/src/core_types.dart (right):

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/core_t...
pkg/compiler/lib/src/core_types.dart:251: /// Returns the 'raw type' of [cls].
That is, the instantiation of [cls]
raw => this ?

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/backend.dart (left):

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend.dart:1890: void
onLibraryCreated(LibraryElement library) {
yay - I was close to make this change after I refactored the loader, I'm glad to
see it gone :)

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/backend_helpers.dart (right):

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:346: ClassEntity get
typedArrayOfIntClass => _typedArrayOfIntClass ??= _findClass(
I wonder if it's worth writing a small codegen tool so we don't maintain this by
hand in the future :)

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:484: return
_findHelperFunction('badMain');
I am noticing more at this moment that sometimes we cache and sometimes we just
have a plain getter. Is that based on how often we use them in the rest of the
compiler? (e.g. if we use it only once, we don't cache it)?

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:559:
_findHelperFunction(r'wrapException');
nit: remove 'r' for consistency (I know it was from before, but just noticed it)

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:724: //
TODO(johnniwinther): Support this in mocks.
now that we are mainly using the memory compiler, maybe we don't need these
TODOs ?

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:783: class
BackendHelpersImpl extends BackendHelpers {
Any reason why not make BackendHelpers concrete and make it take env, backend,
and commonElements as arguments to its constructor?

https://codereview.chromium.org/2675103003/diff/1/tests/compiler/dart2js/type...
File tests/compiler/dart2js/type_mask_test.dart (left):

https://codereview.chromium.org/2675103003/diff/1/tests/compiler/dart2js/type...
tests/compiler/dart2js/type_mask_test.dart:24: compiler.closeResolution();
I'm surprised this worked before :-/ ?

[Johnni Winther, 2017-02-07 09:12:16]

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/core_t...
File pkg/compiler/lib/src/core_types.dart (right):

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/core_t...
pkg/compiler/lib/src/core_types.dart:251: /// Returns the 'raw type' of [cls].
That is, the instantiation of [cls]
On 2017/02/06 17:15:58, Siggi Cherem (dart-lang) wrote:
> raw => this ?

Done.

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/backend_helpers.dart (right):

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:346: ClassEntity get
typedArrayOfIntClass => _typedArrayOfIntClass ??= _findClass(
On 2017/02/06 17:15:59, Siggi Cherem (dart-lang) wrote:
> I wonder if it's worth writing a small codegen tool so we don't maintain this
by
> hand in the future :)

Acknowledged.

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:484: return
_findHelperFunction('badMain');
On 2017/02/06 17:15:58, Siggi Cherem (dart-lang) wrote:
> I am noticing more at this moment that sometimes we cache and sometimes we
just
> have a plain getter. Is that based on how often we use them in the rest of the
> compiler? (e.g. if we use it only once, we don't cache it)?

No. I just followed the existing strategy.

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:559:
_findHelperFunction(r'wrapException');
On 2017/02/06 17:15:59, Siggi Cherem (dart-lang) wrote:
> nit: remove 'r' for consistency (I know it was from before, but just noticed
it)

Done.

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:724: //
TODO(johnniwinther): Support this in mocks.
On 2017/02/06 17:15:58, Siggi Cherem (dart-lang) wrote:
> now that we are mainly using the memory compiler, maybe we don't need these
> TODOs ?

Acknowledged.

https://codereview.chromium.org/2675103003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/backend_helpers.dart:783: class
BackendHelpersImpl extends BackendHelpers {
On 2017/02/06 17:15:58, Siggi Cherem (dart-lang) wrote:
> Any reason why not make BackendHelpers concrete and make it take env, backend,
> and commonElements as arguments to its constructor?

No ;) Done.

[Johnni Winther, 2017-02-07 09:29:28]

Description was changed from

==========
Refactor BackendHelpers to be reusageable with KernelWorldBuilder

- introduces ElementEnvironment and uses it to implement most of CommonElements
and BackendHelpers
==========

to

==========
Refactor BackendHelpers to be reusageable with KernelWorldBuilder

- introduces ElementEnvironment and uses it to implement most of CommonElements
and BackendHelpers

R=sigmund@google.com

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

[Johnni Winther, 2017-02-07 09:29:30]

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

[Emily Fortuna, 2017-02-07 22:25:13]

On 2017/02/07 09:29:30, Johnni Winther wrote:
> Committed patchset #2 (id:20001) manually as
> fe702abc5510169c47fa5f14d02ed00b8d32151c (presubmit successful).

Hey Johnni,
This CL broke the following tests on Kernel dart2js:

<     python tools/test.py -mrelease -cdart2js -rd8 --host-checked -t960
--use-sdk --dart2js-batch --dart2js-options --use-kernel
corelib/uri_example_test
<     python tools/test.py -mrelease -cdart2js -rd8 --host-checked -t960
--use-sdk --dart2js-batch --dart2js-options --use-kernel
language/closure_with_super_field_test
<     python tools/test.py -mrelease -cdart2js -rd8 --host-checked -t960
--use-sdk --dart2js-batch --dart2js-options --use-kernel
language/getters_setters_test
<     python tools/test.py -mrelease -cdart2js -rd8 --host-checked -t960
--use-sdk --dart2js-batch --dart2js-options --use-kernel
language/parameter_initializer_test
<     python tools/test.py -mrelease -cdart2js -rd8 --host-checked -t960
--use-sdk --dart2js-batch --dart2js-options --use-kernel
language/super_assign_test
<     python tools/test.py -mrelease -cdart2js -rd8 --host-checked -t960
--use-sdk --dart2js-batch --dart2js-options --use-kernel
language/super_field_2_test
<     python tools/test.py -mrelease -cdart2js -rd8 --host-checked -t960
--use-sdk --dart2js-batch --dart2js-options --use-kernel
language/super_field_access_test
<     python tools/test.py -mrelease -cdart2js -rd8 --host-checked -t960
--use-sdk --dart2js-batch --dart2js-options --use-kernel
language/super_field_test

Might you be able to submit a follow up patch so these tests can pass, or revert
this CL?
Thank you!