Reapply CL: first step for modular support in fasta

Reapply CL: first step for modular support in fasta

BUG=
R=scheglov@google.com

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

[Siggi Cherem (dart-lang), 2017-05-19 22:44:58]

Patchset #2 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-19 22:46:23]

Patchset #2 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-19 22:47:20]

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

[Siggi Cherem (dart-lang), 2017-05-19 22:47:20]

patchset 1  is the old CL, patchset 2 contains the new changes (only
kernel_target was updated)

[scheglov, 2017-05-19 22:54:57]

LGTM

[Siggi Cherem (dart-lang), 2017-05-20 02:47:00]

Patchset #3 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2017-05-20 02:47:42]

Description was changed from

==========
Reapply CL: first step for modular support in fasta

BUG=
==========

to

==========
Reapply CL: first step for modular support in fasta

BUG=
R=scheglov@google.com

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

[Siggi Cherem (dart-lang), 2017-05-20 02:47:43]

Committed patchset #2 (id:60001) manually as
011f8c5873ebd7214ea5d281b97746eb7d8210e6 (presubmit successful).

[Siggi Cherem (dart-lang), 2017-05-20 02:51:25]

FYI - unit tests are a bit slower now that I moved the trimming after the
transformations are run. The main reason tests get slower is that I don't reuse
the outline on every test anymore, I have to clone it for each test.

If this is an issue, let me know and I can revert that. I can also create a new
suite to get coverage for this separately if needed, or add an option so that we
only do it on compile tests, but not on strong tests

[ahe, 2017-05-22 11:49:02]

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

[ahe, 2017-05-22 11:49:04]

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:268: /// When
[trimDependencies] is true, this also runs a tree-shaker that deletes
This documentation states what is currently going on, but doesn't mention that
it isn't a long-term solution.

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:379:
libraries.addAll(extraLibraries);
Seems like an unnecessary change.

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/verifier.dart (right):

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/verifier.dart:50: var uri = fileUri != null ?
Uri.parse(fileUri) : null;
Why is fileUri null?

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/test/fast...
File pkg/front_end/test/fasta/shaker_test.dart (right):

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/test/fast...
pkg/front_end/test/fasta/shaker_test.dart:48: class TreeShakerContext extends
ChainContext {
This reminds me of code that I've seen before. Is this a copy of something?

[Siggi Cherem (dart-lang), 2017-05-22 23:15:48]

sending a follow up CL to address some of the comments.

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:268: /// When
[trimDependencies] is true, this also runs a tree-shaker that deletes
On 2017/05/22 11:49:04, ahe wrote:
> This documentation states what is currently going on, but doesn't mention that
> it isn't a long-term solution.

added a note about it.

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:379:
libraries.addAll(extraLibraries);
On 2017/05/22 11:49:04, ahe wrote:
> Seems like an unnecessary change.

Ouch, my apologies. I undid a bunch of other changes in this function and I
didn't notice this lingering change. I'll revert it.

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/verifier.dart (right):

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/verifier.dart:50: var uri = fileUri != null ?
Uri.parse(fileUri) : null;
On 2017/05/22 11:49:04, ahe wrote:
> Why is fileUri null?

I ran into this when hitting verification errors for dangling references
initially. Now I cannot easily reproduce this by tweaking the code to produce
those errors, so I don't have a repro to show. If you prefer, I can revert it so
we determine the cause whenever we run into it again.

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/test/fast...
File pkg/front_end/test/fasta/shaker_test.dart (right):

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/test/fast...
pkg/front_end/test/fasta/shaker_test.dart:48: class TreeShakerContext extends
ChainContext {
On 2017/05/22 11:49:04, ahe wrote:
> This reminds me of code that I've seen before. Is this a copy of something?

It was inspired by FastaContext under
pkg/front_end/test/fasta/testing/suite.dart

I could move a subset of the logic into a common base class if you like, it
might not be worthwhile at this time.

The actual steps below are pretty tailored for this test though, in particular,
adding 'lib.dart' as part of the dillTarget to treat it as an external piece for
testing purposes.

[ahe, 2017-05-24 15:45:47]

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/verifier.dart (right):

https://codereview.chromium.org/2894283002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/verifier.dart:50: var uri = fileUri != null ?
Uri.parse(fileUri) : null;
On 2017/05/22 23:15:48, Siggi Cherem (dart-lang) wrote:
> On 2017/05/22 11:49:04, ahe wrote:
> > Why is fileUri null?
> 
> I ran into this when hitting verification errors for dangling references
> initially. Now I cannot easily reproduce this by tweaking the code to produce
> those errors, so I don't have a repro to show. If you prefer, I can revert it
so
> we determine the cause whenever we run into it again.

My preference would be to revert this so we can fix the underlying problem if
this happens again.