Introduce "CommonElements"

Introduce "CommonElements"

Moves common elements that the compiler refers to into a standalone class "CommonElements". Along the way I:

  - made computation of these elements lazy: it's only done when needed. This meant also changing some checks of the form `x == someCommonElement` into `isSomeCommonElement(x)` so that we don't forcefully resolve elements if we haven't seen them.

  - I tried also to remove elements that were only used to know whether something happened. Instead we model that directly with a boolean.

Next steps after this:
 - move CommonElements under common/elements.dart
 - merge CoreClasses + CommonElements
 - introduce TargetElements: elements that the backend refers to that we need to know about during resolution/analysis. I expect most of it to be already in BackendHelpers.
 - get rid of onLibraryScanned/onLibrariesLoaded

R=het@google.com

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

[Siggi Cherem (dart-lang), 2016-08-22 15:49:07]

Description was changed from

==========
Introduce "CoreElements" (TODO: rename)
==========

to

==========
Introduce "CommonElements"

Moves common elements that the compiler refers to into a standalone class
"CommonElements". Along the way I:

  - made computation of these elements lazy: it's only done when needed. This
meant also changing some checks of the form `x == someCommonElement` into
`isSomeCommonElement(x)` so that we don't forcefully resolve elements if we
haven't seen them.

  - I tried also to remove elements that were only used to know whether
something happened. Instead we model that directly with a boolean.

Next steps after this:
 - move CommonElements under common/elements.dart
 - merge CoreClasses + CommonElements
 - introduce TargetElements: elements that the backend refers to that we need to
know about during resolution/analysis. I expect most of it to be already in
BackendHelpers.
 - get rid of onLibraryScanned/onLibrariesLoaded
==========

[Siggi Cherem (dart-lang), 2016-08-22 15:49:14]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2016-08-22 15:49:19]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2016-08-22 15:49:25]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2016-08-22 15:50:47]

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

[Harry Terkelsen, 2016-08-22 19:36:20]

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:362: _coreTypes.onLibraryCreated(library);
seems like this is a place where LibraryLoader can expose a broadcast stream of
libraries that are created instead of having a callback.

Right now, CommonElements still has an implicit dependency on Compiler since it
needs compiler to tell it when libraries are created in order to work correctly.
Maybe CommonElements can take Stream<LibraryElement> as a constructor argument
or something?

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1257: e.enclosingClass == symbolClass && e ==
symbolConstructor;
doesn't this end up loading the symbol constructor?

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1320: ClassElement cls =
compiler._coreTypes.mirrorsUsedClass;
why not mirrorsUsedClass?

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/core_types.dart (right):

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/core_types.dart:122: ClassElement get deferredLibraryClass;
I think we already issue a warning when we see @DeferredLibrary. I think we can
get rid of this now and just ignore @DeferredLibrary annotations (i.e. no
warning)

https://codereview.chromium.org/2265473004/diff/60001/tests/compiler/dart2js/...
File tests/compiler/dart2js/minimal_resolution_test.dart (right):

https://codereview.chromium.org/2265473004/diff/60001/tests/compiler/dart2js/...
tests/compiler/dart2js/minimal_resolution_test.dart:18: await analyze('main() =>
proxy;', proxyConstantComputed: true);
strongly consider just deleting this test case... I think there are many other
ways to test that there is minimal resolution without needlessly adding fields
to compiler/resolution

[Siggi Cherem (dart-lang), 2016-08-22 21:21:45]

thanks harry!

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:362: _coreTypes.onLibraryCreated(library);
On 2016/08/22 19:36:20, Harry Terkelsen wrote:
> seems like this is a place where LibraryLoader can expose a broadcast stream
of
> libraries that are created instead of having a callback.
> 
> Right now, CommonElements still has an implicit dependency on Compiler since
it
> needs compiler to tell it when libraries are created in order to work
correctly.
> Maybe CommonElements can take Stream<LibraryElement> as a constructor argument
> or something?

Good idea - I'll do this on a separate CL though. I have a CL in progress moving
patch-parsing to the loader, so that will get rid of onLibraryScanned, I might
get rid of all callbacks together there.

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1257: e.enclosingClass == symbolClass && e ==
symbolConstructor;
On 2016/08/22 19:36:20, Harry Terkelsen wrote:
> doesn't this end up loading the symbol constructor?

turns out that the first check is only true if the symbol class was already
resolved, so at this point we are not adding more work to the resolver, we are
only looking up the ctor within the previously resolved class.

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1320: ClassElement cls =
compiler._coreTypes.mirrorsUsedClass;
On 2016/08/22 19:36:20, Harry Terkelsen wrote:
> why not mirrorsUsedClass?

oops. Done - it used to be somewhere else and I missed that when I finally moved
it here.

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/core_types.dart (right):

https://codereview.chromium.org/2265473004/diff/60001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/core_types.dart:122: ClassElement get deferredLibraryClass;
On 2016/08/22 19:36:20, Harry Terkelsen wrote:
> I think we already issue a warning when we see @DeferredLibrary. I think we
can
> get rid of this now and just ignore @DeferredLibrary annotations (i.e. no
> warning)

I'm still worried that it would be a breaking change :( -- it appears that
besides the warning we still accept it and implement the deferred import with
that syntax...

https://codereview.chromium.org/2265473004/diff/60001/tests/compiler/dart2js/...
File tests/compiler/dart2js/minimal_resolution_test.dart (right):

https://codereview.chromium.org/2265473004/diff/60001/tests/compiler/dart2js/...
tests/compiler/dart2js/minimal_resolution_test.dart:18: await analyze('main() =>
proxy;', proxyConstantComputed: true);
On 2016/08/22 19:36:20, Harry Terkelsen wrote:
> strongly consider just deleting this test case... I think there are many other
> ways to test that there is minimal resolution without needlessly adding fields
> to compiler/resolution

I totally agree with you. I'll add a big TODO on this. I rather expose the full
list of resolved classes rather than having these adhoc fields just for testing.

[Harry Terkelsen, 2016-08-22 21:31:00]

lgtm

[Siggi Cherem (dart-lang), 2016-08-22 21:53:31]

Description was changed from

==========
Introduce "CommonElements"

Moves common elements that the compiler refers to into a standalone class
"CommonElements". Along the way I:

  - made computation of these elements lazy: it's only done when needed. This
meant also changing some checks of the form `x == someCommonElement` into
`isSomeCommonElement(x)` so that we don't forcefully resolve elements if we
haven't seen them.

  - I tried also to remove elements that were only used to know whether
something happened. Instead we model that directly with a boolean.

Next steps after this:
 - move CommonElements under common/elements.dart
 - merge CoreClasses + CommonElements
 - introduce TargetElements: elements that the backend refers to that we need to
know about during resolution/analysis. I expect most of it to be already in
BackendHelpers.
 - get rid of onLibraryScanned/onLibrariesLoaded
==========

to

==========
Introduce "CommonElements"

Moves common elements that the compiler refers to into a standalone class
"CommonElements". Along the way I:

  - made computation of these elements lazy: it's only done when needed. This
meant also changing some checks of the form `x == someCommonElement` into
`isSomeCommonElement(x)` so that we don't forcefully resolve elements if we
haven't seen them.

  - I tried also to remove elements that were only used to know whether
something happened. Instead we model that directly with a boolean.

Next steps after this:
 - move CommonElements under common/elements.dart
 - merge CoreClasses + CommonElements
 - introduce TargetElements: elements that the backend refers to that we need to
know about during resolution/analysis. I expect most of it to be already in
BackendHelpers.
 - get rid of onLibraryScanned/onLibrariesLoaded

R=het@google.com

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

[Siggi Cherem (dart-lang), 2016-08-22 21:53:32]

Committed patchset #2 (id:80001) manually as
b16d5ca237338e64a33064d6127397ce026b7b2a (presubmit successful).