Remove check for scheme in source-loader.

Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).

This is necessary to fix dartbug.com/30141

R=ahe@google.com

Committed: https://github.com/dart-lang/sdk/commit/d9ae60e14fdec52bda977ed114f6ad70dc2f0bc4
Committed: https://github.com/dart-lang/sdk/commit/1a2870f798a5daab46162171e33d01fe7dc0e2e3

[Siggi Cherem (dart-lang), 2017-08-23 20:36:45]

Description was changed from

==========
Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).
==========

to

==========
Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).

This is necessary to fix dartbug.com/30141
==========

[Siggi Cherem (dart-lang), 2017-08-23 20:38:32]

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

[Siggi Cherem (dart-lang), 2017-08-23 20:38:33]

https://codereview.chromium.org/3006483002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (left):

https://codereview.chromium.org/3006483002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_loader.dart:101: if (uri == null ||
uri.scheme != "file" && uri.scheme != "multi-root") {
note that the `fileSystem.entityForUri` call below will report an error when uri
is `null`. It is now up to the file system to define what is supported.

We could add a `fileSystem.exists(uri)` here as well if we want to produce a
different error, but it feels redundant.

[ahe, 2017-08-24 10:08:53]

I think this should work: keep the if, but change the condition to be:

if (uri == null || !uri.hasScheme) {
...

https://codereview.chromium.org/3006483002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (left):

https://codereview.chromium.org/3006483002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_loader.dart:101: if (uri == null ||
uri.scheme != "file" && uri.scheme != "multi-root") {
On 2017/08/23 20:38:33, Siggi Cherem (dart-lang) wrote:
> note that the `fileSystem.entityForUri` call below will report an error when
uri
> is `null`. It is now up to the file system to define what is supported.
> 
> We could add a `fileSystem.exists(uri)` here as well if we want to produce a
> different error, but it feels redundant.

The following situations are still erroneous at this point:

uri == null
!uri.hasScheme

Regardless if fileSystem can handle scheme-less URIs, that's a problem here as
it would imply that a library.uri didn't have a scheme. That would be a major
invariant violation that would cause problems elsewhere in Fasta.

Make sure that this doesn't crash:

./pkg/front_end/tool/fasta testing compile/rasta/unsupported_platform_library

[Siggi Cherem (dart-lang), 2017-08-24 18:55:19]

PTAL

https://codereview.chromium.org/3006483002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (left):

https://codereview.chromium.org/3006483002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_loader.dart:101: if (uri == null ||
uri.scheme != "file" && uri.scheme != "multi-root") {
On 2017/08/24 10:08:53, ahe wrote:
> On 2017/08/23 20:38:33, Siggi Cherem (dart-lang) wrote:
> > note that the `fileSystem.entityForUri` call below will report an error when
> uri
> > is `null`. It is now up to the file system to define what is supported.
> > 
> > We could add a `fileSystem.exists(uri)` here as well if we want to produce a
> > different error, but it feels redundant.
> 
> The following situations are still erroneous at this point:
> 
> uri == null
> !uri.hasScheme
> 
> Regardless if fileSystem can handle scheme-less URIs, that's a problem here as
> it would imply that a library.uri didn't have a scheme. That would be a major
> invariant violation that would cause problems elsewhere in Fasta.
> 
> Make sure that this doesn't crash:
> 
> ./pkg/front_end/tool/fasta testing compile/rasta/unsupported_platform_library

Done - as you suggested now I'm checking for null and !hasScheme.

The `null` case would have been handled by the filesystem regardless, but it's
fine to have it here. For the hasScheme I added a new branch so we can provide a
more informative error message. I switched to use internalProblem for it.

[Siggi Cherem (dart-lang), 2017-08-24 19:29:18]

(I may land this TBR to unblock Johnni and I'd be happy to address any concerns
as a follow up.)

[Siggi Cherem (dart-lang), 2017-08-24 23:24:37]

Description was changed from

==========
Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).

This is necessary to fix dartbug.com/30141
==========

to

==========
Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).

This is necessary to fix dartbug.com/30141

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

[Siggi Cherem (dart-lang), 2017-08-24 23:24:38]

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

[ahe, 2017-08-25 08:52:31]

https://codereview.chromium.org/3006483002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (left):

https://codereview.chromium.org/3006483002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_loader.dart:101: if (uri == null ||
uri.scheme != "file" && uri.scheme != "multi-root") {
On 2017/08/24 18:55:19, Siggi Cherem (dart-lang) wrote:
> On 2017/08/24 10:08:53, ahe wrote:
> > On 2017/08/23 20:38:33, Siggi Cherem (dart-lang) wrote:
> > > note that the `fileSystem.entityForUri` call below will report an error
when
> > uri
> > > is `null`. It is now up to the file system to define what is supported.
> > > 
> > > We could add a `fileSystem.exists(uri)` here as well if we want to produce
a
> > > different error, but it feels redundant.
> > 
> > The following situations are still erroneous at this point:
> > 
> > uri == null
> > !uri.hasScheme
> > 
> > Regardless if fileSystem can handle scheme-less URIs, that's a problem here
as
> > it would imply that a library.uri didn't have a scheme. That would be a
major
> > invariant violation that would cause problems elsewhere in Fasta.
> > 
> > Make sure that this doesn't crash:
> > 
> > ./pkg/front_end/tool/fasta testing
compile/rasta/unsupported_platform_library
> 
> Done - as you suggested now I'm checking for null and !hasScheme.
> 
> The `null` case would have been handled by the filesystem regardless, but it's

That's odd. When I ran the test with this code removed, it crashed. Also, I'm
not sure how FileSystem would be able to diagnose that "dart:html" is missing
based on being passed null?

> fine to have it here. For the hasScheme I added a new branch so we can provide
a
> more informative error message. I switched to use internalProblem for it.

Great idea.

[Siggi Cherem (dart-lang), 2017-08-25 15:06:25]

PTAL at the last patch set.

We are seeing crashes because we are capturing FileSystemExceptions and not
other kind of exceptions from the file system abstraction. Some of the failing
tests were passing a `data:*` URI, for example.

I believe PhysicalFileSystem should also use a FSE in this situation because it
is a valid argument of the FileSystem interface (we can't complain with an
argument error if users of the interface have no way to know that it is not
supported).

[Siggi Cherem (dart-lang), 2017-08-25 18:31:22]

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

[Siggi Cherem (dart-lang), 2017-08-25 18:31:23]

FYI Johnni - I'll wait for an lgtm from Peter, if you need this sooner, feel
free to coordinate with him/land on my behalf.

[Siggi Cherem (dart-lang), 2017-08-25 23:24:37]

Description was changed from

==========
Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).

This is necessary to fix dartbug.com/30141

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

to

==========
Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).

This is necessary to fix dartbug.com/30141

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

[ahe, 2017-08-28 12:54:39]

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

https://codereview.chromium.org/3006483002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_loader.dart:106: return
internalProblem(
I'm not sure these checks belong here anymore. Let me take a stab at it.

[ahe, 2017-08-29 15:49:55]

lgtm

I can make my changes in a separate CL, so let's just land this for now.

[Siggi Cherem (dart-lang), 2017-08-29 18:35:20]

Description was changed from

==========
Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).

This is necessary to fix dartbug.com/30141

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

to

==========
Remove check for scheme in source-loader.

Context: this prevents us from using other schemes in unit tests, and causes
issues when processign a URI with no scheme, that happens to be supported by the
underlying file-system (e.g. PhysicalFileSystem will make it a `file:*` URI if
no scheme was present).

This is necessary to fix dartbug.com/30141

R=ahe@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d9ae60e14fdec52bda977ed114f6ad70dc2f0bc4
Committed:
https://github.com/dart-lang/sdk/commit/1a2870f798a5daab46162171e33d01fe7dc0e2e3
==========

[Siggi Cherem (dart-lang), 2017-08-29 18:35:21]

Committed patchset #3 (id:40001) manually as
1a2870f798a5daab46162171e33d01fe7dc0e2e3 (presubmit successful).