Start using FileState/FileSystemState to provide consistent view.

pkg/front_end/lib/src/incremental/file_state.dart

Index: pkg/front_end/lib/src/incremental/file_state.dart
diff --git a/pkg/front_end/lib/src/incremental/file_state.dart b/pkg/front_end/lib/src/incremental/file_state.dart
new file mode 100644
index 0000000000000000000000000000000000000000..556fad12383af9ece0423c0133d7a501bae5d40f
--- /dev/null
+++ b/pkg/front_end/lib/src/incremental/file_state.dart
@@ -0,0 +1,229 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:async';
+import 'dart:convert';
+import 'dart:typed_data';
+
+import 'package:front_end/file_system.dart';
+import 'package:front_end/src/fasta/parser/top_level_parser.dart';
+import 'package:front_end/src/fasta/scanner.dart';
+import 'package:front_end/src/fasta/source/directive_listener.dart';
+import 'package:front_end/src/fasta/translate_uri.dart';
+
+/// Information about a file being analyzed, explicitly or implicitly.

[Siggi Cherem (dart-lang), 2017/05/08 23:55:17]

nit: now that this includes compilation maybe "analyzed" is too specific to
analyzer :)?

Other ideas "compiled" or "processed"?

[scheglov, 2017/05/09 00:16:28]

I will changed it to "compiled".

+///
+/// It provides a consistent view on its properties.
+///
+/// The properties are not guaranteed to represent the most recent state
+/// of the file system. To update the file to the most recent state, [refresh]
+/// should be called.
+class FileState {
+  final FileSystemState _fsState;
+
+  /// The resolved URI of the file in the file system.
+  final Uri fileUri;
+
+  bool _exists;
+  List<int> _contentBytes;
+  String _content;

[Paul Berry, 2017/05/08 22:10:48]

Is it necessary to keep both _contentBytes and _content?  Seems like a waste of
memory.

[scheglov, 2017/05/09 00:16:28]

I think you're right.
I will fix this in the next CL.

+
+  List<FileState> _importedFiles;
+  List<FileState> _exportedFiles;
+  List<FileState> _partedFiles;

[Siggi Cherem (dart-lang), 2017/05/08 23:55:18]

nit: rename to _partfiles

[scheglov, 2017/05/09 00:16:28]

Done.

+
+  Set<FileState> _directReferencedFiles = new Set<FileState>();
+
+  FileState._(this._fsState, this.fileUri);
+
+  /// The content of the file.
+  String get content => _content;
+
+  /// The content bytes of the file.
+  List<int> get contentBytes => _contentBytes;
+
+  /// Whether the file exists.
+  bool get exists => _exists;
+
+  @override
+  int get hashCode => fileUri.hashCode;
+
+  /// Return the set of transitive files - the file itself and all of the
+  /// directly or indirectly referenced files.
+  Set<FileState> get transitiveFiles {
+    // TODO(scheglov) add caching.
+    var transitiveFiles = new Set<FileState>();
+
+    void appendReferenced(FileState file) {
+      if (transitiveFiles.add(file)) {
+        file._directReferencedFiles.forEach(appendReferenced);
+      }
+    }
+
+    appendReferenced(this);
+    return transitiveFiles;
+  }
+
+  @override
+  bool operator ==(Object other) {
+    return other is FileState && other.fileUri == fileUri;
+  }
+
+  /// Read the file content and ensure that all of the file properties are
+  /// consistent with the read content, including all its dependencies.
+  Future<Null> refresh() async {
+    // Read the content.

[Siggi Cherem (dart-lang), 2017/05/08 23:55:17]

I expect `refresh` will only get called on files that someone marked via
"IncrementalAPI.invalidate", correct (that's way if I'm watching for
modifications outside the front_end to provide fine-grain invalidate() calls,
then there wont be a referesh for unmodified files?)

[scheglov, 2017/05/09 00:16:28]

Correct.

We read each file at least once when refresh() the entry point and create the
graph of FileState instances (see FileSystemState.getFile()). Then any file is
re-read only if it is invalidated explicitly.  If it is imported by an
invalidated file, and is not invalidated itself, then it will not be re-read.

+    try {
+      FileSystemEntity entry = _fsState.fileSystem.entityForUri(fileUri);
+      _contentBytes = await entry.readAsBytes();
+      _content = UTF8.decode(_contentBytes);
+      _exists = true;
+    } catch (_) {
+      _contentBytes = new Uint8List(0);
+      _content = '';
+      _exists = false;
+    }
+
+    // Parse directives.
+    ScannerResult scannerResults = scanString(_content);
+    var listener = new DirectiveListener();
+    new TopLevelParser(listener).parseUnit(scannerResults.tokens);
+
+    // Build the graph.
+    _importedFiles = <FileState>[];
+    _exportedFiles = <FileState>[];
+    _partedFiles = <FileState>[];
+    for (String import in listener.imports) {

[Paul Berry, 2017/05/08 22:10:49]

Can we rename "import" to "importedFile" or something?  Seeing the keyword
"import" in this line and the one below is really confusing.

[Siggi Cherem (dart-lang), 2017/05/08 23:55:18]

nit: I'd use `var` instead of `String`

For the variable name: `uri` seems descriptive enoguh for all three loops.

[scheglov, 2017/05/09 00:16:28]

Done.

[scheglov, 2017/05/09 00:16:28]

I don't see the type of "listener.imports".
It is not created, and it is not a typed local variable.
I agree about `uri`.

[Siggi Cherem (dart-lang), 2017/05/09 00:26:15]

Not sure why that's not working: listener.imports is declared as a Set<String>
in DirectiveListener, so I expect this should be inferred here :-/

[scheglov, 2017/05/09 00:29:43]

Ah, sorry, I should have been more explicit.
The type is inferred.
But I don't know it just by looking on code, without using IDE to learn it.
I'm fine with switching to not typing such cycles, if this is the code style we
want in front_end.

+      await _addFileForRelativeUri(_importedFiles, import);
+    }
+    for (String export in listener.exports) {

[Paul Berry, 2017/05/08 22:10:49]

Similar issue here

[scheglov, 2017/05/09 00:16:28]

Done.

+      await _addFileForRelativeUri(_exportedFiles, export);

[Siggi Cherem (dart-lang), 2017/05/08 23:55:17]

optional - it appears to me that it is safe to parallelize here. For example,
do:


addImport(uri) => _addFileForRelativeUri(_importedFiles, uri);
_importedFiles = await Future.wait(listener.imports.map(addImport).toList());

addExport(uri) => _addFileForRelativeUri(_exportedFiles, uri);
_exportedFiles = await Future.wait(listener.exports.map(addExport).toList());

addPart(uri) => _addFileForRelativeUri(_partFiles, uri);
_partFiles = await Future.wait(listener.parts.map(addPart).toList());

(technically we could also do all three group in parallel too).

[scheglov, 2017/05/09 00:16:28]

That's an interesting idea.
I will keep it in mind while experimenting with performance optimizations.

+    }
+    for (String export in listener.parts) {

[Paul Berry, 2017/05/08 22:10:49]

Here we should rename "export" to something like "partFile"

[scheglov, 2017/05/09 00:16:28]

Done.

+      await _addFileForRelativeUri(_partedFiles, export);
+    }
+    // TODO(scheglov) make this optional

[Paul Berry, 2017/05/08 22:10:49]

Why?  All files import 'dart:core' (either implicitly or explicitly), so there's
no circumstance where we can truly drop the dependency.  The only benefit would
be to optimize the case case where the file explicitly imports 'dart:core', but
it's hard to imagine that having a measurable performance impact.

[scheglov, 2017/05/09 00:16:28]

Well, yeah, I thought about adding it only if dart:core is not imported
explicitly.
I will change to make it imported unconditionally.

+    await _addFileForRelativeUri(_importedFiles, 'dart:core');
+
+    // Compute referenced files.
+    _directReferencedFiles = new Set<FileState>()
+      ..addAll(_importedFiles)
+      ..addAll(_exportedFiles)
+      ..addAll(_partedFiles);
+  }
+
+  /// Add the [FileState] for the given [relativeUri] to the [files].
+  /// Do nothing if the URI cannot be parsed, cannot correspond any file, etc.
+  Future<Null> _addFileForRelativeUri(
+      List<FileState> files, String relativeUri) async {
+    if (relativeUri.isEmpty) {

[Siggi Cherem (dart-lang), 2017/05/08 23:55:17]

optional nit: I tend to prefer no braces on these one-line ifs, especially for
`return` statements (see second example of
https://www.dartlang.org/guides/language/effective-dart/style#do-use-curly-br...)

[scheglov, 2017/05/09 00:16:28]

Done.

+      return;
+    }
+
+    // Resolve the relative URI into absolute.
+    // The result is either:
+    //   1) The absolute file URI.
+    //   2) The absolute non-file URI, e.g. `package:foo/foo.dart`.
+    Uri absoluteUri;
+    try {
+      absoluteUri = this.fileUri.resolve(relativeUri);
+    } on FormatException {
+      return;
+    }
+
+    // Resolve the absolute URI into the absolute file URI.
+    Uri fileUri = _fsState.uriTranslator.translate(absoluteUri);

[Siggi Cherem (dart-lang), 2017/05/08 23:55:17]

note that this shadows FielState.fileUri - it might be worth using a different
name here. Possibly: resolvedUri?

[scheglov, 2017/05/09 00:16:28]

Done.

+    if (fileUri == null) {

[Siggi Cherem (dart-lang), 2017/05/08 23:55:17]

same here about one-line if ("if (fileUri == null) return;")

[scheglov, 2017/05/09 00:16:28]

Done.

+      return;
+    }
+
+    FileState file = await _fsState.getFile(fileUri);
+    files.add(file);
+  }
+}
+
+/// Information about known file system state.
+class FileSystemState {
+  final FileSystem fileSystem;
+  final TranslateUri uriTranslator;
+
+  _FileSystemView _fileSystemView;
+
+  /// Mapping from file URIs to corresponding [FileState]s.
+  final Map<Uri, FileState> _fileUriToFile = {};
+
+  FileSystemState(this.fileSystem, this.uriTranslator);
+
+  /// Return the [FileSystem] that is backed by this [FileSystemState].  The
+  /// files in this [FileSystem] always have the same content as the
+  /// corresponding [FileState]s, thus avoiding race conditions when a file
+  /// is updated on the actual file system.
+  FileSystem get fileSystemView {
+    return _fileSystemView ??= new _FileSystemView(this);
+  }
+
+  /// Return the [FileState] for the given resolved file [fileUri].
+  /// The returned file has the last known state since it was last refreshed.
+  Future<FileState> getFile(Uri fileUri) async {
+    FileState file = _fileUriToFile[fileUri];
+    if (file == null) {
+      file = new FileState._(this, fileUri);
+      _fileUriToFile[fileUri] = file;
+
+      // Build the sub-graph of the file.
+      await file.refresh();
+    }
+    return file;
+  }
+}
+
+/// [FileSystemState] based implementation of [FileSystem].
+/// It provides a consistent view on the known file system state.
+class _FileSystemView implements FileSystem {
+  final FileSystemState fsState;
+
+  _FileSystemView(this.fsState);
+
+  @override
+  FileSystemEntity entityForUri(Uri uri) {
+    FileState file = fsState._fileUriToFile[uri];
+    return new _FileSystemViewEntry(uri, file);
+  }
+}
+
+/// [FileSystemState] based implementation of [FileSystemEntity].
+class _FileSystemViewEntry implements FileSystemEntity {
+  @override
+  final Uri uri;
+
+  final FileState file;
+
+  _FileSystemViewEntry(this.uri, this.file);
+
+  @override
+  Future<bool> exists() async => file?.exists ?? false;
+
+  @override
+  Future<DateTime> lastModified() async {
+    throw new UnimplementedError();

[Paul Berry, 2017/05/08 22:10:49]

Maybe throw a StateError() here with a message saying something like
"FileSystemViewEntry modification stamp should not be queried", just so that
someone doesn't come along in the future and think that they need to implement
this method.

(I'm still hoping we can eliminate this method from the API so that the
front_end never thinks about timestamps.  Rationale: some of the concrete
filesystems we might imagine hooking up to in the future don't provide
timestamps (e.g. reading files using HTTP get requests))

[scheglov, 2017/05/09 00:16:28]

Done.
I agree, this API might not always be accessible.

+  }
+
+  @override
+  Future<List<int>> readAsBytes() async {
+    _throwIfDoesNotExist();
+    return file.contentBytes;
+  }
+
+  @override
+  Future<String> readAsString() async {
+    _throwIfDoesNotExist();
+    return file.content;
+  }
+
+  void _throwIfDoesNotExist() {
+    if (file == null) {
+      throw new FileSystemException(uri, 'File $uri does not exist.');
+    }
+  }
+}