Enhance DirectiveListener to collect import/exports with combinators.

Enhance DirectiveListener to collect import/exports with combinators.

We need combinators to build export scopes for DILL libraries.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Paul Berry, 2017-05-15 20:28:30]

lgtm

https://codereview.chromium.org/2884783002/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2884783002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:134:
_importedLibraries.add(coreFile);
Should we report an error?  It seems like this should never happen if we are
pointing to a proper SDK.

https://codereview.chromium.org/2884783002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:184:
_importedLibraries.add(file);
Similar question here.

[scheglov, 2017-05-15 20:59:19]

Description was changed from

==========
Enhance DirectiveListener to collect import/exports with combinators.

We need combinators to build export scopes for DILL libraries.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Enhance DirectiveListener to collect import/exports with combinators.

We need combinators to build export scopes for DILL libraries.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-05-15 20:59:21]

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

[Siggi Cherem (dart-lang), 2017-05-15 22:18:53]

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/directive_listener.dart (right):

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/directive_listener.dart:63: List<String>
names = _popList(count);
I'm not opposed to keeping it as it is, but given that this DirectiveListener is
so specialized for a small piece of the language, I'd be open to do this without
the stack and simply by having an extra field or two to help us with the
parsing. For example, in the same way you have `_inDirective`, we could add a
`Set<String> _combinatorNames` and `List<NamespaceCombinator> _combinators` that
gets filled as you parse them.

- they all get cleared on every new import/export
- handleIdentifier will append to _combinatorNames
- endShow/endHide consume the _combinatorNames and add an object to _combinators
- endCombinators does almost nothing, or changes the state somehow (e.g.
_inCombinators = false)
- endImport/endExport simply use _combinators.

WDYT?

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/directive_listener.dart:145: final
List<NamespaceCombinator> combinators;
Just double checking - I was wondering whether we need a list here - I can't
recall if the spec allows more than one combinator, but I'm guessing we might
want it regardless in the case of error-recovery. Correct?

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/directive_listener.dart:150: class
ImportDirective {
consider using a single Directive class for both import/export. Other than
keeping them separate as you do on the lists above, there seems no need to have
different types for their internal representation.

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:178: /// TODO(scheglov) Ask VM
people whether all these libraries are required.
I think it's safe to remove this.

The VM needs these libraries, but not from us :) - we want it to get them from a
platform.dill file that is built separately and not from the app. The VM now
supports this, but fasta still includes them because we haven't finished moving
over to just use the platform.dill for the VM tests and performance bots. As
Slava said on that email, in the future these platform.dill will be replaced
with a VM snapshot.

If those libraries get imported by the user, then yes we will need to be handle
them, but that's a regular import at that point.

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:288: bool filter(String name)
{
nit: consider renaming this method. It might be just me, but when I read
`filter`, I think that "true" means that `name` is not visible.
Some ideas: `isIncluded`, `isVisible`, `isExported`, for example:

/// Whether [name] is exposed by the underlying export
/// after applying all [combinators].
bool isExposed(name) { ... }

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:289: for (NamespaceCombinator
combinator in combinators) {
how much do we expect to use these functions? In practice do we expect these
combinator lists to be big?

I just wonder whether it is worth flattening the list of combinators to make
lookups efficient. For instance, I believe it can be shrink down to a boolean
(isShow/isHide) and a Set of names.

[scheglov, 2017-05-16 00:14:24]

Thank you for the comment.
I've uploaded a new CL with the changes.

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/directive_listener.dart (right):

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/directive_listener.dart:63: List<String>
names = _popList(count);
On 2017/05/15 22:18:53, Siggi Cherem (dart-lang) wrote:
> I'm not opposed to keeping it as it is, but given that this DirectiveListener
is
> so specialized for a small piece of the language, I'd be open to do this
without
> the stack and simply by having an extra field or two to help us with the
> parsing. For example, in the same way you have `_inDirective`, we could add a
> `Set<String> _combinatorNames` and `List<NamespaceCombinator> _combinators`
that
> gets filled as you parse them.
> 
> - they all get cleared on every new import/export
> - handleIdentifier will append to _combinatorNames
> - endShow/endHide consume the _combinatorNames and add an object to
_combinators
> - endCombinators does almost nothing, or changes the state somehow (e.g.
> _inCombinators = false)
> - endImport/endExport simply use _combinators.
> 
> WDYT?

Thank you, good idea.
I will send a new CL with these changes.

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/directive_listener.dart:145: final
List<NamespaceCombinator> combinators;
On 2017/05/15 22:18:53, Siggi Cherem (dart-lang) wrote:
> Just double checking - I was wondering whether we need a list here - I can't
> recall if the spec allows more than one combinator, but I'm guessing we might
> want it regardless in the case of error-recovery. Correct?

The specification does not limit the number of combinators.
importSpecification:
    import uri (as identifier)? combinator* ‘;’

That said, I have never seen more than one outside of tests.

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/directive_listener.dart:150: class
ImportDirective {
On 2017/05/15 22:18:53, Siggi Cherem (dart-lang) wrote:
> consider using a single Directive class for both import/export. Other than
> keeping them separate as you do on the lists above, there seems no need to
have
> different types for their internal representation.

Done.

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:178: /// TODO(scheglov) Ask VM
people whether all these libraries are required.
On 2017/05/15 22:18:53, Siggi Cherem (dart-lang) wrote:
> I think it's safe to remove this.
> 
> The VM needs these libraries, but not from us :) - we want it to get them from
a
> platform.dill file that is built separately and not from the app. The VM now
> supports this, but fasta still includes them because we haven't finished
moving
> over to just use the platform.dill for the VM tests and performance bots. As
> Slava said on that email, in the future these platform.dill will be replaced
> with a VM snapshot.
> 
> If those libraries get imported by the user, then yes we will need to be
handle
> them, but that's a regular import at that point.

It's not safe to remove here.
TargetImplemenation.loadExtraRequiredLibraries() schedules all of them.
Maybe something else should also be changed in Fasta?

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:288: bool filter(String name)
{
On 2017/05/15 22:18:53, Siggi Cherem (dart-lang) wrote:
> nit: consider renaming this method. It might be just me, but when I read
> `filter`, I think that "true" means that `name` is not visible.
> Some ideas: `isIncluded`, `isVisible`, `isExported`, for example:
> 
> /// Whether [name] is exposed by the underlying export
> /// after applying all [combinators].
> bool isExposed(name) { ... }
> 

Done.

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:289: for (NamespaceCombinator
combinator in combinators) {
On 2017/05/15 22:18:53, Siggi Cherem (dart-lang) wrote:
> how much do we expect to use these functions? In practice do we expect these
> combinator lists to be big?
> 
> I just wonder whether it is worth flattening the list of combinators to make
> lookups efficient. For instance, I believe it can be shrink down to a boolean
> (isShow/isHide) and a Set of names.

I think usually there are zero combinators.
Sometimes one.
I have never seen more than one.
But this is still allowed by the specification.

I don't think we need flattening here.

[Siggi Cherem (dart-lang), 2017-05-16 21:43:47]

thanks

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/file_state.dart (right):

https://codereview.chromium.org/2884783002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/file_state.dart:178: /// TODO(scheglov) Ask VM
people whether all these libraries are required.
On 2017/05/16 00:14:24, scheglov wrote:
> On 2017/05/15 22:18:53, Siggi Cherem (dart-lang) wrote:
> > I think it's safe to remove this.
> > 
> > The VM needs these libraries, but not from us :) - we want it to get them
from
> a
> > platform.dill file that is built separately and not from the app. The VM now
> > supports this, but fasta still includes them because we haven't finished
> moving
> > over to just use the platform.dill for the VM tests and performance bots. As
> > Slava said on that email, in the future these platform.dill will be replaced
> > with a VM snapshot.
> > 
> > If those libraries get imported by the user, then yes we will need to be
> handle
> > them, but that's a regular import at that point.
> 
> It's not safe to remove here.
> TargetImplemenation.loadExtraRequiredLibraries() schedules all of them.
> Maybe something else should also be changed in Fasta?

I see, then yes :) we need to update fasta to stop doing it unless we pass a
flag with the list of libraries to enqueue