Add support for multi-roots

Add support for multi-roots

R=paulberry@google.com

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

[Siggi Cherem (dart-lang), 2017-07-01 03:38:55]

Description was changed from

==========
Add support for multi-roots (in progress)
==========

to

==========
Add support for multi-roots
==========

[Siggi Cherem (dart-lang), 2017-07-01 03:39:08]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2017-07-01 03:52:26]

Patchset #2 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-01 03:52:32]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-01 04:01:45]

Patchset #1 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-01 04:01:51]

Patchset #1 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-01 04:09:57]

sigmund@google.com changed reviewers:
+ ahe@google.com, paulberry@google.com

[Siggi Cherem (dart-lang), 2017-07-01 04:09:57]

Note: this CL sits on top of the previous CL regarding public APIs (so it can't
be patched directly)

We need to discuss in person what we want to do about the multi-root scheme.

Brainstorming one alternative idea here:
 - remove CompilerOptions.multiRoot
 - keep the multi-root file system implementation in package:front_end
 - let users (bazel) set it up by hand by supplying the filesystem directly, and
 - add to file-system an extra property (e.g. supported schemes) so the
source_loader is agnostic of the scheme.

Thoughts?

Jake - in it's current state, you should be able to use this in combination with
the other CL to test things out in your experiment.

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/source_loader.dart:84: if (uri == null ||
(scheme != "file" && scheme != "multi-root")) {
I'm not super happy with this part of the change, other ideas?

[Paul Berry, 2017-07-04 15:32:17]

lgtm

Feel free to address my comments in a follow-up CL

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/source_loader.dart:84: if (uri == null ||
(scheme != "file" && scheme != "multi-root")) {
On 2017/07/01 04:09:57, Siggi Cherem (dart-lang) wrote:
> I'm not super happy with this part of the change, other ideas?

Agreed, I'm not thrilled with this either.  A particular disadvantage of this
approach is that if the user uses a "multi-root" scheme in a compilation where
multi-root is disabled, source_loader will think the URI is fine, and then the
user will see an ArgumentError bubble up from PhysicalFileSystem and it will
look like they've hit a compiler bug :(

IMO it should be the responsibility of the fileSystem to know what kinds of URIs
are valid/invalid, not the source_loader (this includes checking for a null
URI).  So my preference would be to modify PhysicalFileSystem.entityForUri so
that if it's passed a bad or null URI, it returns a new kind of FileSystemEntity
which responds to any read/write request with some sort of "bad URI" exception. 
(That way the behavior for a bad URI will be similar to the behavior for a
non-existent file).  Once we've done that, we should be able to remove lines
83-86 from this file entirely.

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/test/src...
File pkg/front_end/test/src/multi_root_file_system_test.dart (right):

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/test/src...
pkg/front_end/test/src/multi_root_file_system_test.dart:95: }
Can we add some tests to check that "multi-root:///../foo.dart" can't be used to
access parent directories of the roots?

[Paul Berry, 2017-07-04 15:38:47]

On 2017/07/01 04:09:57, Siggi Cherem (dart-lang) wrote:
> Note: this CL sits on top of the previous CL regarding public APIs (so it
can't
> be patched directly)
> 
> We need to discuss in person what we want to do about the multi-root scheme.
> 
> Brainstorming one alternative idea here:
>  - remove CompilerOptions.multiRoot
>  - keep the multi-root file system implementation in package:front_end
>  - let users (bazel) set it up by hand by supplying the filesystem directly,
and
>  - add to file-system an extra property (e.g. supported schemes) so the
> source_loader is agnostic of the scheme.

My concern with this is that bazel isn't a direct client of the front end. 
Bazel's only interaction with the front end is by running executables and
passing them command-line arguments.  I believe in the long run, we're going to
have multiple top level executables that we will want to support multi-root
functionality: dart2js, DDC, whatever executable generates summaries, and maybe
even analyzer.  We're going to want all of those top level executables to
implement multi-root functionality in the same way, and the easiest way to do
that is to have it in CompilerOptions so that the front end does it the same way
for all tools.  Removing the functionality from the front end just places a
burden on all of the top level executables to implement the feature consistently
with each other.

What's the motivation for removing CompilerOptions.multiRoot?  Are there some
people who don't like the command-line functionality we're exposing to the end
user?  If so, then we should discuss what command-line functionality we want to
implement instead (and make sure it satisfies the Bazel use case).  Whatever
functionality we decide on, I think the front end is the right place to
implement it, and an option in CompilerOptions is the right way to enable it.

[Siggi Cherem (dart-lang), 2017-07-05 22:48:30]

Thanks Paul for all the comments.

Good point about the risk of multiple clients and inconsistent ways of providing
the multi-root argument.

My main motivation was to not build into front-end any specific scheme. Other
ways we could achieve that would be to add a default value for what the scheme
would be. Now that we have a plan to hide the use of the scheme entirely under
the file-system, I'm less concerned about this though.

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/source_loader.dart:84: if (uri == null ||
(scheme != "file" && scheme != "multi-root")) {
On 2017/07/04 15:32:17, Paul Berry wrote:
> On 2017/07/01 04:09:57, Siggi Cherem (dart-lang) wrote:
> > I'm not super happy with this part of the change, other ideas?
> 
> Agreed, I'm not thrilled with this either.  A particular disadvantage of this
> approach is that if the user uses a "multi-root" scheme in a compilation where
> multi-root is disabled, source_loader will think the URI is fine, and then the
> user will see an ArgumentError bubble up from PhysicalFileSystem and it will
> look like they've hit a compiler bug :(
> 
> IMO it should be the responsibility of the fileSystem to know what kinds of
URIs
> are valid/invalid, not the source_loader (this includes checking for a null
> URI).  So my preference would be to modify PhysicalFileSystem.entityForUri so
> that if it's passed a bad or null URI, it returns a new kind of
FileSystemEntity
> which responds to any read/write request with some sort of "bad URI"
exception. 
> (That way the behavior for a bad URI will be similar to the behavior for a
> non-existent file).  Once we've done that, we should be able to remove lines
> 83-86 from this file entirely.

I like your suggestion. In dart2js we also have a `memory:` scheme we used for
unit testing, hiding this check behind the FileSystem will let us support that
use case as well.

I'll do the change on a separate CL though.

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/test/src...
File pkg/front_end/test/src/multi_root_file_system_test.dart (right):

https://codereview.chromium.org/2964323002/diff/100001/pkg/front_end/test/src...
pkg/front_end/test/src/multi_root_file_system_test.dart:95: }
On 2017/07/04 15:32:17, Paul Berry wrote:
> Can we add some tests to check that "multi-root:///../foo.dart" can't be used
to
> access parent directories of the roots?

Done.

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

Patchset #2 (id:120001) has been deleted

[Paul Berry, 2017-07-06 17:18:24]

Patch2 lgtm as well

[Siggi Cherem (dart-lang), 2017-07-10 18:49:01]

Description was changed from

==========
Add support for multi-roots
==========

to

==========
Add support for multi-roots

R=paulberry@google.com

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

[Siggi Cherem (dart-lang), 2017-07-10 18:49:02]

Committed patchset #2 (id:140001) manually as
f422249fcd1a5f6db5857a2611e1964ac05693c7 (presubmit successful).