Add implementations of the front end FileSystem API.

Add implementations of the front end FileSystem API.

This required some small changes to the API contract.

Note that the tests use supermixins.  I'm assuming that supermixin
functionality will be available on all platforms by the time this is
needed.  If not, I will be happy to rewrite them.

R=scheglov@google.com, sigmund@google.com

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

[Paul Berry, 2016-11-02 20:17:07]

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

[Siggi Cherem (dart-lang), 2016-11-03 00:46:50]

lgtm

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
File pkg/front_end/lib/memory_file_system.dart (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
pkg/front_end/lib/memory_file_system.dart:59: other is MemoryFileSystemEntity &&
other.path == path;
I'm ok not adding it, but any need to also check that the _fileSystem is the
same?

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/test/memory_f...
File pkg/front_end/test/memory_file_system_test.dart (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/test/memory_f...
pkg/front_end/test/memory_file_system_test.dart:14: import
'package:test_reflective_loader/test_reflective_loader.dart';
I've never used this framework, we normally just use package:test alone. I'll
ask you offline later more about it. One concern I have if we later want to run
dart2js tests is whether this is using dart:mirrors, in which case it can make
the building and running of tests more expensive or slower than necessary.

https://codereview.chromium.org/2471283002/diff/1/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/pkg.status#newcode56
pkg/pkg.status:56: front_end/test/memory_file_system_test: CompileTimeError #
Issue 23773
This seems fine, we don't need to have it running in dart2js at this time. We
can revisit this if we have a valid use case for it in the future (e.g. if,
before having kernel working, we integrate FE with ddc and want to run it in the
browser to enable their debugging tools).

https://codereview.chromium.org/2471283002/diff/1/pkg/pkg.status#newcode57
pkg/pkg.status:57: front_end/test/physical_file_system_test: CompileTimeError #
Issue 23773
this one should probably be skipByDesign since it's using dart:io

[scheglov, 2016-11-03 02:15:24]

LGTM

I'm sorry for the delay, I missed it in my mail box.

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
File pkg/front_end/lib/memory_file_system.dart (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
pkg/front_end/lib/memory_file_system.dart:39: if (uri.scheme != 'file') throw
new ArgumentError('File URI expected');
Do we want to check (or TODO) that uri is absolute?

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
pkg/front_end/lib/memory_file_system.dart:91: _fileSystem._files[path] =
UTF8.encode(s);
AFAIK UTF8 returns Uint8List at runtime, but the static type is List<int>.

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/physical_...
File pkg/front_end/lib/physical_file_system.dart (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/physical_...
pkg/front_end/lib/physical_file_system.dart:19: static PhysicalFileSystem
instance = new PhysicalFileSystem._();
+final?

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/test/memory_f...
File pkg/front_end/test/memory_file_system_test.dart (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/test/memory_f...
pkg/front_end/test/memory_file_system_test.dart:159: throwsA(new
isInstanceOf<Error>()));
Maybe just test for the specific isArgumentError that is current thrown.

[Paul Berry, 2016-11-03 15:16:41]

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
File pkg/front_end/lib/memory_file_system.dart (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
pkg/front_end/lib/memory_file_system.dart:39: if (uri.scheme != 'file') throw
new ArgumentError('File URI expected');
On 2016/11/03 02:15:24, scheglov wrote:
> Do we want to check (or TODO) that uri is absolute?

AFAICT, the scheme check is sufficient--it's impossible to create a relative URI
with a non-empty scheme.  Even if you do `new Uri(scheme: 'file', path:
'file.txt')`, the path winds up being changed to '/file.txt'.

I've added a comments to clarify (both here and in PhysicalFileSystem), and I've
added tests just to be certain.

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
pkg/front_end/lib/memory_file_system.dart:59: other is MemoryFileSystemEntity &&
other.path == path;
On 2016/11/03 00:46:50, Siggi Cherem (dart-lang) wrote:
> I'm ok not adding it, but any need to also check that the _fileSystem is the
> same?

Done.

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/memory_fi...
pkg/front_end/lib/memory_file_system.dart:91: _fileSystem._files[path] =
UTF8.encode(s);
On 2016/11/03 02:15:23, scheglov wrote:
> AFAIK UTF8 returns Uint8List at runtime, but the static type is List<int>.

True.  That means that technically this implementation relies on an
implementation detail of UTF8.encode.  I think that's ok--it's an implementation
detail that seems really unlikely to be broken in the future, and if it ever is
broken the test will fail.

I've added a comment and a cast (`as Uint8List`) to make this more explicit.  If

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/physical_...
File pkg/front_end/lib/physical_file_system.dart (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/lib/physical_...
pkg/front_end/lib/physical_file_system.dart:19: static PhysicalFileSystem
instance = new PhysicalFileSystem._();
On 2016/11/03 02:15:24, scheglov wrote:
> +final?

Done.  Thank you.

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/test/memory_f...
File pkg/front_end/test/memory_file_system_test.dart (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/test/memory_f...
pkg/front_end/test/memory_file_system_test.dart:14: import
'package:test_reflective_loader/test_reflective_loader.dart';
On 2016/11/03 00:46:50, Siggi Cherem (dart-lang) wrote:
> I've never used this framework, we normally just use package:test alone. I'll
> ask you offline later more about it. One concern I have if we later want to
run
> dart2js tests is whether this is using dart:mirrors, in which case it can make
> the building and running of tests more expensive or slower than necessary.

This framework adds a lot of functionality which has really helped make test
writing easier in analyzer (and which I believe will really help in the front
end as well).  But you're right that it comes at the cost of relying on mirrors.
 I'm going to commit it as is for now, but I agree that it would be good to talk
offline and make sure it's worth it.

https://codereview.chromium.org/2471283002/diff/1/pkg/front_end/test/memory_f...
pkg/front_end/test/memory_file_system_test.dart:159: throwsA(new
isInstanceOf<Error>()));
On 2016/11/03 02:15:24, scheglov wrote:
> Maybe just test for the specific isArgumentError that is current thrown.

The interface contract is that we simply throw an instance of `Error`.  If we
want to tighten the test, we should tighten the interface contract too, and IMHO
that's not worth doing.

Leaving as is, but I'm open to being convinced if you feel strongly about this.

https://codereview.chromium.org/2471283002/diff/1/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/2471283002/diff/1/pkg/pkg.status#newcode56
pkg/pkg.status:56: front_end/test/memory_file_system_test: CompileTimeError #
Issue 23773
On 2016/11/03 00:46:50, Siggi Cherem (dart-lang) wrote:
> This seems fine, we don't need to have it running in dart2js at this time. We
> can revisit this if we have a valid use case for it in the future (e.g. if,
> before having kernel working, we integrate FE with ddc and want to run it in
the
> browser to enable their debugging tools).

Acknowledged.

https://codereview.chromium.org/2471283002/diff/1/pkg/pkg.status#newcode57
pkg/pkg.status:57: front_end/test/physical_file_system_test: CompileTimeError #
Issue 23773
On 2016/11/03 00:46:50, Siggi Cherem (dart-lang) wrote:
> this one should probably be skipByDesign since it's using dart:io

Done.

[Paul Berry, 2016-11-03 15:21:15]

Description was changed from

==========
Add implementations of the front end FileSystem API.

This required some small changes to the API contract.

Note that the tests use supermixins.  I'm assuming that supermixin
functionality will be available on all platforms by the time this is
needed.  If not, I will be happy to rewrite them.
==========

to

==========
Add implementations of the front end FileSystem API.

This required some small changes to the API contract.

Note that the tests use supermixins.  I'm assuming that supermixin
functionality will be available on all platforms by the time this is
needed.  If not, I will be happy to rewrite them.

R=scheglov@google.com, sigmund@google.com

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

[Paul Berry, 2016-11-03 15:21:16]

Committed patchset #1 (id:1) manually as
28b6a1f7ea81cf1a5ea78e940f8a17f61c0684e6 (presubmit successful).