Test that subpackages of front_end don't have undesired dependencies.

pkg/front_end/test/subpackage_relationships_test.dart

Index: pkg/front_end/test/subpackage_relationships_test.dart
diff --git a/pkg/front_end/test/subpackage_relationships_test.dart b/pkg/front_end/test/subpackage_relationships_test.dart
new file mode 100644
index 0000000000000000000000000000000000000000..3af7b72eae7923171907e99c98157daa07c42c41
--- /dev/null
+++ b/pkg/front_end/test/subpackage_relationships_test.dart
@@ -0,0 +1,144 @@
+// 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:io';
+
+import 'package:front_end/compiler_options.dart';
+import 'package:front_end/dependency_grapher.dart';
+import 'package:path/path.dart' as pathos;
+
+main() async {
+  exit(await new _SubpackageRelationshipsTest().run());
+}
+
+/// Map from subpackage name to the rules for what the subpackage is allowed to
+/// depend directly on.
+///
+/// Each subdirectory of `lib/src` is considered a subpackage.  Files in
+/// `lib/src` but not in a subdirectory are considered to be in the `lib/src`
+/// subpackage.  Files outside of `lib/src` (but still in `lib`) are considered
+/// to be in the `lib` subpackage.
+///
+/// TODO(paulberry): stuff in lib/src shouldn't depend on lib; lib should just
+/// re-export stuff in lib/src.
+/// TODO(paulberry): remove dependencies on analyzer.
+final subpackageRules = {

[Paul Berry, 2017/01/12 21:32:16]

Note: this describes the dependencies that exist today.  My expectation is that
we'll update this variable as necessary whenever new subpackages are added or
the dependencies change.

+  'lib': new SubpackageRules(
+      mayImportAnalyzer: true,
+      allowedDependencies: ['lib/src', 'lib/src/base']),
+  'lib/src': new SubpackageRules(
+      mayImportAnalyzer: true,
+      allowedDependencies: ['lib', 'lib/src/base', 'lib/src/scanner']),
+  'lib/src/base': new SubpackageRules(
+      mayImportAnalyzer: true, allowedDependencies: ['lib']),
+  'lib/src/scanner': new SubpackageRules(allowedDependencies: ['lib/src/base']),
+};
+
+/// Rules for what a subpackage may depend directly on.
+class SubpackageRules {
+  /// Indicates whether the subpackage may directly depend on analyzer.
+  final bool mayImportAnalyzer;
+
+  /// Indicates which other subpackages a given subpackage may directly depend
+  /// on.
+  final List<String> allowedDependencies;
+
+  SubpackageRules(
+      {this.mayImportAnalyzer: false, this.allowedDependencies: const []});
+}
+
+class _SubpackageRelationshipsTest {
+  /// File uri of the root of the front_end package.
+  final frontEndRootUri = Platform.script.resolve('..');

[danrubel, 2017/01/12 21:54:54]

Will this test fail if run from the wrong directory?
Should we add a fast fail sanity check?

[Paul Berry, 2017/01/12 22:25:01]

Platform.script is the path to the script file (meaning this file), so it should
be fine regardless of which directory the script is run from.

+
+  /// Indicates whether any problems have been reported yet.
+  bool problemsReported = false;
+
+  /// Check for problems resulting from URI [src] having a direct dependency on
+  /// URI [dst].
+  void checkDependency(Uri src, Uri dst) {
+    if (dst.scheme == 'dart') return;
+    if (dst.scheme != 'package') {
+      problem('$src depends on $dst, which is neither a package: or dart: URI');
+      return;
+    }
+    var srcSubpackage = subpackageForUri(src);
+    if (srcSubpackage == null) return;
+    var srcSubpackageRules = subpackageRules[srcSubpackage];
+    if (srcSubpackageRules == null) {
+      problem('$src is in subpackage "$srcSubpackage", which is not found in '
+          'subpackageRules');
+      return;
+    }
+    if (!srcSubpackageRules.mayImportAnalyzer &&
+        dst.pathSegments[0] == 'analyzer') {
+      problem('$src depends on $dst, but subpackage "$srcSubpackage" may not '
+          'import analyzer');
+    }
+    var dstSubPackage = subpackageForUri(dst);
+    if (dstSubPackage == null) return;
+    if (dstSubPackage == srcSubpackage) return;
+    if (!srcSubpackageRules.allowedDependencies.contains(dstSubPackage)) {
+      problem('$src depends on $dst, but subpackage "$srcSubpackage" is not '
+          'allowed to depend on subpackage "$dstSubPackage"');
+    }
+  }
+
+  /// Finds all files in the front_end's "lib" directory and returns their Uris
+  /// (as "package:" URIs).
+  Future<List<Uri>> findFrontEndUris() async {

[Siggi Cherem (dart-lang), 2017/01/12 22:39:12]

FWIW - for simple unit tests, I'm in favor of keeping things simple and use sync
IO if that helps.

[Paul Berry, 2017/01/12 23:35:57]

Fair enough.  Done.

+    var frontEndUris = <Uri>[];
+    var frontEndRootPath = pathos.fromUri(frontEndRootUri);
+    await for (var entity in new Directory(frontEndRootPath)
+        .list(recursive: true, followLinks: false)) {
+      if (entity is File && entity.path.endsWith('.dart')) {
+        var posixRelativePath = pathos
+            .relative(entity.path, from: frontEndRootPath)
+            .replaceAll(pathos.separator, '/');

[Siggi Cherem (dart-lang), 2017/01/12 22:39:12]

alternatively, use the various pathos contexts (in this case pathos.url)?

  pathos.url.joinAll(pathos.split(pathos.relative(...)));

[Paul Berry, 2017/01/12 23:35:57]

Done.

+        if (!posixRelativePath.startsWith('lib/')) continue;

[Siggi Cherem (dart-lang), 2017/01/12 22:39:12]

consider making the frontEndRootPath be `Platform.script.resolve('../lib/');`,
then you can focus the crawling to just the lib folder.

[Paul Berry, 2017/01/12 23:35:57]

Done.

+        frontEndUris.add(Uri.parse(
+            posixRelativePath.replaceFirst('lib/', 'package:front_end/')));
+      }
+    }
+    return frontEndUris;
+  }
+
+  /// Reports a single problem.
+  void problem(String description) {
+    print(description);
+    problemsReported = true;
+  }
+
+  /// Tests all subpackage relationships in the front end, and returns an
+  /// appropriate exit code.
+  Future<int> run() async {
+    var frontEndUris = await findFrontEndUris();
+    var packagesFileUri = frontEndRootUri.resolve('../../.packages');
+    var graph = await graphForProgram(
+        frontEndUris,
+        new CompilerOptions()
+          ..packagesFileUri = packagesFileUri
+          ..chaseDependencies = true);
+    for (var i = 0; i < graph.topologicallySortedCycles.length; i++) {

[Siggi Cherem (dart-lang), 2017/01/12 22:39:12]

Do we need to use the graph/walker API and compute SSCs?

If it is sufficient, it would be nice to only iterate over the files on
pkg/front_end and check URIs by parsing directives directly, without chasing
dependencies into other packages entirely.

[Paul Berry, 2017/01/12 23:35:57]

Regarding computation of library cycles, I agree that it's not needed but I
think there's little harm in using an API that computes it as a side effect (the
algorithm used by the front end is O(N)).  I'd like to continue using the
graph/walker API on the grounds that we should eat our own dogfood as much as
possible.

Regarding the fact that we waste our time iterating over files in other
packages, I agree that this is unfortunate.  I plan to address this in a future
CL by implementing support for CompilerOptions.chaseDependencies = false in the
graph/walker API, and that will prevent it from looking into any files other
than the ones in front_end/lib.

[Siggi Cherem (dart-lang), 2017/01/13 00:02:06]

Sounds good, no problem.

Last year some unit tests were blocking some work in dart2js because of extra
dependencies that weren't necessary, so I'm now more alert when it comes to unit
tests dependencies :)

+      for (var library in graph.topologicallySortedCycles[i].libraries.values) {
+        for (var dependency in library.dependencies) {
+          checkDependency(library.uri, dependency.uri);
+        }
+      }
+    }
+    return problemsReported ? 1 : 0;
+  }
+
+  /// Determines which subpackage [src] is in.
+  ///
+  /// If [src] is not part of the front end, `null` is returned.
+  String subpackageForUri(Uri src) {
+    if (src.scheme != 'package') return null;
+    if (src.pathSegments[0] != 'front_end') return null;
+    if (src.pathSegments[1] != 'src') return 'lib';
+    if (src.pathSegments.length == 3) return 'lib/src';
+    return 'lib/src/${src.pathSegments[2]}';
+  }
+}