First pass at strict dependencies #1508

matanlurey · 2017-01-24T03:26:25Z

First step of #343.

Implemented StrictDependenciesValidator, which crawls all source files, finds all referenced import and export directives, and compares them against all declared dependencies and dev_dependencies.

IMO this should be an error, not a warning, but wanted input first to see if I've possibly missed legitimate use cases where people should be able to override and upload anyway. Should this also be added as a flag to pub to opt-in or just added by default? I didn't do anything but implement the validator yet.

~~Future work:~~

~~- [ ] Validate you haven't declared dependencies you don't use (?)~~

matanlurey · 2017-01-24T03:26:41Z

/cc @kevmoo

kevmoo · 2017-01-24T04:30:56Z

Crazy excited to see this!

lrhn · 2017-01-24T07:00:58Z

One use-case that might be affected is to have a package that is only used through Isolate.spawnUri.
It's probably rare - a package intended for such use will likely have its own launcher code that you would include in the main application too, instead of just doing Isolate.spawnUri(Uri.parse("package:otherwise_unused/main.dart")

nex3

Generally we only make new validations warnings, because we don't want to get into a situation where users are avoiding updating pub because older versions will publish their packages and newer versions won't. We may upgrade some of these to errors for 2.0.

Validate you haven't declared dependencies you don't use (?)

I don't like this. It's valid for a package to only use assets from its dependencies, for example.

nex3 · 2017-01-24T22:48:09Z

lib/src/validator/strict_dependencies.dart

+class StrictDependenciesValidator extends Validator {
+  StrictDependenciesValidator(Entrypoint entrypoint) : super(entrypoint);
+
+  @override


Pub doesn't use @override.

nex3 · 2017-01-24T22:53:55Z

lib/src/validator/strict_dependencies.dart

+import 'package:pub/src/validator.dart';
+
+/// Returns a map of files -> package names imported or exported within [files].
+Map<String, Iterable<String>> _findUsedPackages(Iterable<String> files) {


Make this an instance method of StrictDependenciesValidator.

nex3 · 2017-01-24T22:56:47Z

lib/src/validator/strict_dependencies.dart

+  var packageNames = <String, Iterable<String>>{};
+  for (var file in files) {
+    var usedPackages = <String>[];
+    var compilationUnit = parseDirectives(new File(file).readAsStringSync());


Pub uses utilities in lib/src/io.dart to do filesystem stuff with logging—in this case, readTextFile().

Also, you can use parseImportsAndExports() from lib/src/dart.dart to do this more easily.

This should gracefully handle a parse failure. It's not great UI if pub lish just crashes with an analysis error.

Done, thanks.

Added a simple try/catch but let me know if you want something more sophisticated...

nex3 · 2017-01-24T22:59:08Z

lib/src/validator/strict_dependencies.dart

+
+  @override
+  Future validate() {
+    return new Future.sync(() async {


Why not make the method itself async?

nex3 · 2017-01-24T23:01:03Z

lib/src/validator/strict_dependencies.dart

+    return new Future.sync(() async {
+      var declared = new Set<String>()
+        ..addAll(entrypoint.root.dependencies.map((d) => d.name))
+        ..addAll(entrypoint.root.devDependencies.map((d) => d.name));


This should also verify that files in lib and bin don't use dev dependencies.

Is that always true? For example I've seen packages that have something like

lib/src/test_utils.dart

I'm not saying it's right, but this seems like a much bigger change than just enforcing that you've declared your dependencies so far and I'd rather tackle it in a future improvement with some consensus.

It's definitely invalid for any Dart file in lib/ to import anything that's not a non-dev dependency. The fact that people may be doing this now is all the more reason to add a warning and notify them that it violates pub's dependency semantics.

nex3 · 2017-01-24T23:08:24Z

test/validator/strict_dependencies_test.dart

+}
+
+main() {
+  group('should consider a package valid if', () {


"if it"

Concatenated test names should generally read as sentences.

nex3 · 2017-01-24T23:10:01Z

test/validator/strict_dependencies_test.dart

+          "not_declared": "^1.2.3"
+        }, sdk: ">=1.8.0 <2.0.0")
+      ]).create();
+      d.file(path.join(appPath, 'lib', 'library.dart'), r'''


Nest this in the previous d.dir() call.

nex3 · 2017-01-24T23:10:59Z

test/validator/strict_dependencies_test.dart

+    integration('declares an "import" as a dependency', () {
+      d.dir(appPath, [
+        d.libPubspec("test_pkg", "1.0.0", deps: {
+          "not_declared": "^1.2.3"


Declaring a dependency on a package named "not_declared" is pretty confusing :p.

Renamed to @kevmoo's favorite, silly_monkey

nex3 · 2017-01-24T23:16:49Z

lib/src/validator/strict_dependencies.dart

+    for (final directive in compilationUnit.directives) {
+      if (directive is UriBasedDirective) {
+        usedPackages
+            .add(Uri.parse(directive.uri.stringValue).pathSegments.first);


What if it's not a package: import?

Done, thanks.

nex3 · 2017-01-24T23:21:24Z

test/validator/strict_dependencies_test.dart

+      expectValidationWarning(strictDeps);
+    });
+  });
+}


Also test:

Files outside lib/.

A matching dev dependency.

A non-Dart file that looks like a Dart file that contains an invalid dependency.

A Dart file with a parse error early enough that parseDirectives() runs into it.

A Dart file with a relative import.

A Dart file with an absolute, non-package: import.

Dart files with invalid package: URIs like "package:", "package:foo", "package:/", "package:/]".

I did not add a file outside lib/ as per my comment above, nor invalid packages - I could use some guidance on the latter if there is already something in pub that can do simple validation or if I should just check the parsed Uri.

The rest are done!

I did not add a file outside lib/ as per my comment above

Which comment?

I could use some guidance on the latter if there is already something in pub that can do simple validation or if I should just check the parsed Uri.

All the validators were written before the analyzer library existed, so we currently validate nothing that involves parsing Dart code. For invalid URLs I think we should probably just emit a warning that says "Invalid URL" and points to the source span.

I did not add a file outside lib/ as per my comment above

Which comment?

Got outdated, but I am not sure how to do this. Do you have any pointers?

I could use some guidance on the latter if there is already something in pub that can do simple validation or if I should just check the parsed Uri.

All the validators were written before the analyzer library existed, so we currently validate nothing that involves parsing Dart code. For invalid URLs I think we should probably just emit a warning that says "Invalid URL" and points to the source span.

How can I determine an invalid URL? Other than reinventing the world.

I'm guessing you already parse URIs with Uri.parse, or similar. That should ensure that the URI is actually valid URIs.

Package URIs have a more restricted syntax that you might also want to check. To validate those, you can either reuse the code in package:package_config or aim lower (that code is written for .packages files which are very specific, actual imports might want to be more permissive).

The base rules for package URIs is:

Starts with package:.

Has no authority part.

The path doesn't start with a /,

but with a path segment containing at least one non-. character and no : or % characters,

followed by a /.

Has no query part (but that's probably not important).

You can check that at the string level with a RegExp:

var pchar = r"!$&-\-0-9;=@-Z_a-z~"; const pcharDot = r"!$&-.0-9;=@-Z_a-z~"; var packageUriRE = new RegExp("^package:[$pcharDot]*[$pchar]$[pcharDot]*/");

or on a parsed URI:

const pchar = r"!$&-\-0-9;=@-Z_a-z~"; const pcharDot = r"!$&-.0-9;=@-Z_a-z~"; final packagePathUriRE = new RegExp("^[$pcharDot]*[$pchar]$[pcharDot]*/"); bool isPackageUriValid(Uri uri) { return (uri.scheme == "package" && !uri.hasAuthority && uri.path.matches(packageUriPathRE) && !uri.hasQuery); }

Got outdated, but I am not sure how to do this. Do you have any pointers?

You could add a _DependencyUse.inLib getter, only check against normal dependencies if it's set, then check against dev dependencies when generating a message to provide more information.

How can I determine an invalid URL? Other than reinventing the world.

I don't think you need to do anything as complex as Lasse is suggesting. Uri.parse() will throw a FormatError if the URL doesn't parse at all, and if it does I think it's sufficient to check that the package name isn't null—any other weird names will fail to match any dependencies.

Sounds good, I've implemented that.

nex3 · 2017-01-25T21:48:13Z

lib/src/validator/strict_dependencies.dart

+    return new Future.sync(() async {
+      var declared = new Set<String>()
+        ..addAll(entrypoint.root.dependencies.map((d) => d.name))
+        ..addAll(entrypoint.root.devDependencies.map((d) => d.name));


It's definitely invalid for any Dart file in lib/ to import anything that's not a non-dev dependency. The fact that people may be doing this now is all the more reason to add a warning and notify them that it violates pub's dependency semantics.

nex3 · 2017-01-25T21:52:18Z

test/validator/strict_dependencies_test.dart

+      expectValidationWarning(strictDeps);
+    });
+  });
+}


I did not add a file outside lib/ as per my comment above

Which comment?

I could use some guidance on the latter if there is already something in pub that can do simple validation or if I should just check the parsed Uri.

All the validators were written before the analyzer library existed, so we currently validate nothing that involves parsing Dart code. For invalid URLs I think we should probably just emit a warning that says "Invalid URL" and points to the source span.

nex3 · 2017-01-25T22:06:04Z

lib/src/validator/strict_dependencies.dart


 import 'package:analyzer/analyzer.dart';
-import 'package:pub/src/solver/version_solver.dart';
+import 'package:path/path.dart' as path;


as p. Pub is kind of inconsistent about this, but that's only because it's old—new files always use as p.

nex3 · 2017-01-25T22:06:09Z

lib/src/validator/strict_dependencies.dart

-        usedPackages
-            .add(Uri.parse(directive.uri.stringValue).pathSegments.first);
+class StrictDependenciesValidator extends Validator {
+  static bool _isDartFile(String file) => path.extension(file) == '.dart';


I'd just inline this.

nex3 · 2017-01-25T22:08:07Z

lib/src/validator/strict_dependencies.dart

-      if (directive is UriBasedDirective) {
-        usedPackages
-            .add(Uri.parse(directive.uri.stringValue).pathSegments.first);
+class StrictDependenciesValidator extends Validator {


Document this, and also the class below and its members.

nex3 · 2017-01-25T22:17:35Z

lib/src/validator/strict_dependencies.dart

+      : _parsedUri = Uri.parse(directive.uri.stringValue),
+        _directive = directive;
+
+  bool get isPubPackage => _parsedUri.scheme == 'package';


This is kind of misleading, since everything (including the entrypoint) is a pub package. Maybe isPackageUrl?

nex3 · 2017-01-25T22:20:28Z

lib/src/validator/strict_dependencies.dart

+  String get package => _parsedUri.pathSegments.first;
+
+  String toErrorMessage() {
+    return new SourceFile(_contents, url: _file)


I'm guessing that you're creating the SourceFile here rather than sharing one between all the directives for a given file so that you avoid parsing the line endings in the common case of a file that has only valid directives? If so, mention that in a comment.

nex3 · 2017-01-25T22:20:38Z

lib/src/validator/strict_dependencies.dart

+        .span(_directive.offset, _directive.length)
+        .message(
+            '$_file imports $package, but this package doesn\'t depend '
+            'on $package');


nex3 · 2017-01-25T22:22:13Z

test/validator/strict_dependencies_test.dart

@@ -17,30 +17,88 @@ Validator strictDeps(Entrypoint entrypoint) {
 }

 main() {
-  group('should consider a package valid if', () {
+  group('should consider a package valid if it', () {
    setUp(d.validPackage.create);

    integration('looks normal', () => expectNoValidationError(strictDeps));

    integration('declares an "import" as a dependency', () {


We don't need to duplicate every test, but we should probably have at least one test for an export that doesn't produce a warning.

nex3 · 2017-01-25T22:24:10Z

lib/src/validator/strict_dependencies.dart

+
+  String toErrorMessage() {
+    return new SourceFile(_contents, url: _file)
+        .span(_directive.offset, _directive.length)


.span() takes a start and an end, not a start and a length. It's also 0-based, so make sure that the analyzer is as well or add some - 1s if not.

matanlurey · 2017-01-27T20:20:14Z

@nex3 I think I've tackled everything you asked for - PTAL.

If there are only incremental things left I'd be happy to fix them in more follow-up PRs since it's not yet enabled by default or behind a flag or anything. Let me know.

nex3

If there are only incremental things left I'd be happy to fix them in more follow-up PRs since it's not yet enabled by default or behind a flag or anything. Let me know.

I generally prefer to keep master in a releasable state if at all possible, especially for pub where master is frequently pulled into the SDK and released.

nex3 · 2017-01-27T22:24:31Z

lib/src/validator/strict_dependencies.dart

+
+/// Validates that Dart source files only import declared dependencies.
+class StrictDependenciesValidator extends Validator {
+  static Iterable<String> _combine(Iterable<String> a, Iterable<String> b) {


I'd name this combineIterables() and put it in utils.dart.

nex3 · 2017-01-27T22:28:54Z

lib/src/validator/strict_dependencies.dart

+import 'package:pub/src/dart.dart';
+import 'package:pub/src/entrypoint.dart';
+import 'package:pub/src/io.dart';
+import 'package:pub/src/log.dart';


Import this with the log prefix.

nex3 · 2017-01-27T22:33:21Z

lib/src/validator/strict_dependencies.dart

+        directives = parseImportsAndExports(contents, name: file);
+      } on AnalyzerErrorGroup catch (e, s) {
+        // Ignore files that do not parse.
+        exception(e, s);


log.exception() will print the exception message at the "error" level, which is visible by default. I think we want to hide these by default—it'll be pretty confusing if pub lish surfaces a bunch of parse errors that aren't in the typical warning or error format. I'd just do:

log.fine(getErrorMessage(error)); log.fine(new Chain.forTrace(stackTrace).terse);

nex3 · 2017-01-27T22:44:59Z

lib/src/validator/strict_dependencies.dart

+
+  StrictDependenciesValidator(Entrypoint entrypoint) : super(entrypoint);
+
+  Set<String> _dependencies;


Please document private members.

nex3 · 2017-01-27T22:45:44Z

lib/src/validator/strict_dependencies.dart

+          .toSet();
+    }
+    return entrypoint.root.name == package || _dependencies.contains(package);
+  }


Rather than making these instance methods, consider creating the two sets in validate() and just passing them to the two _validate...() functions.

nex3 · 2017-01-27T22:50:20Z

lib/src/validator/strict_dependencies.dart

+  bool get isUriValid => _uri != null && _uri.pathSegments.length >= 2;
+
+  /// Returns whether the directive references a pub package.
+  bool get isPackageUrl => _uri.scheme == 'package';


What if _uri is null?

Simplified, so this is gone now.

nex3 · 2017-01-27T22:54:17Z

lib/src/validator/strict_dependencies.dart

+  String uriInvalidMessage() {
+    assert(!isUriValid);
+    var uri = _directive.uri.stringValue;
+    return _toMessage('$_file references an invalid URI: $uri');


I'd just say Invalid URL. here. The source span message will already show the URL and the filename.

Fun fact: the term "URI" is deprecated. Dart is very inconsistent about this, but I'm trying to use "URL" where possible. Maybe we can change the class name in 2.0...

nex3 · 2017-01-27T22:55:04Z

lib/src/validator/strict_dependencies.dart

+  /// Returns an error message saying the package is not listed in dependencies.
+  String dependencyMissingMessage() {
+    return _toMessage(
+      '$_file imports $package, but this package doesn\'t depend on $package.'


This package doesn't depend on $package.

nex3 · 2017-01-27T23:01:45Z

lib/src/validator/strict_dependencies.dart

+  /// Returns an error message saying the package should be in `dependencies`.
+  String dependencyMisplaceMessage() {
+    return _toMessage(
+      '$_file imports $package, but is only listed in `devDependencies`. Files '


$package is a dev dependency. Packages used in ${p.split(p.relative(_file)).first}/ must be declared as normal dependencies.

The newline is important—we try to wrap publish messages around 80 characters.

nex3 · 2017-01-27T23:10:16Z

test/validator/strict_dependencies_test.dart

+      ]).create();
+      expectValidationWarning(strictDeps);
+    });
+  });


Still needs no-warning tests for:

An export of a dependency.

A top level .dart file that contains an invalid dependency.

A non-Dart file that looks like a Dart file that contains an invalid dependency.

And a warning test for Dart files with URIs like "package:", "package:foo", and "package:/]".

Done. Nice catch on the non-Dart file.

nex3 · 2017-01-30T22:06:55Z

lib/src/validator/strict_dependencies.dart

+            uri.scheme == 'package' &&
+            uri.pathSegments.length < 2 ||
+            uri.pathSegments.any((s) => s.isEmpty)) {
+          warnings.add('Invalid URL');


When you moved this out here, you lost the source span context.

I pulled it out to a static function to re-use here with minimal boiler.

nex3 · 2017-01-30T22:08:25Z

lib/src/validator/strict_dependencies.dart

+        if (uri == null ||
+            uri.scheme == 'package' &&
+            uri.pathSegments.length < 2 ||
+            uri.pathSegments.any((s) => s.isEmpty)) {


Please parenthesize sub-expressions with different boolean operators. I'm sure the order of operation works out here, but it's a lot harder to visually verify that without explicit grouping.

Sure, I understand. Done.

nex3 · 2017-01-30T22:10:47Z

lib/src/validator/strict_dependencies.dart

-    return _toMessage(
-      '$_file imports $package, but this package doesn\'t depend on $package.'
-    );
+    return _toMessage('This packagee doesn\'t depend on $package.');


nex3 · 2017-01-30T22:12:22Z

test/validator/strict_dependencies_test.dart

+        ]).create();
+        expectValidationWarning(strictDeps);
+      });
+    });


We still need to test for package:/]. This is meaningfully different because it's invalid URL syntax, whereas the rest of these are invalid only as package: URLs in particular.

nex3 · 2017-01-30T23:53:43Z

Thanks for working so hard on this, Matan!

First pass at strict dependencies

d5837f8

matanlurey requested review from nex3 and munificent January 24, 2017 03:26

googlebot added the cla: yes label Jan 24, 2017

nex3 suggested changes Jan 24, 2017

View reviewed changes

Address feedback

3747b13

nex3 suggested changes Jan 25, 2017

View reviewed changes

Address feedback, small rewrite, more tests

9244fa0

nex3 suggested changes Jan 27, 2017

View reviewed changes

Address more cases, simplify.

33ca738

nex3 suggested changes Jan 30, 2017

View reviewed changes

matanlurey and others added 2 commits January 30, 2017 14:47

Address comments.

cb490a8

Style tweaks.

7097a70

nex3 approved these changes Jan 30, 2017

View reviewed changes

nex3 merged commit d5a5233 into dart-lang:master Jan 30, 2017

zoechi mentioned this pull request Feb 4, 2017

add a command to find unused package dependencies google/tuneup.dart#9

Closed

nex3 mentioned this pull request May 27, 2017

Add a missing stack_trace dependency. dart-archive/matcher#51

Merged


		StrictDependenciesValidator(Entrypoint entrypoint) : super(entrypoint);

		Set<String> _dependencies;

First pass at strict dependencies #1508

First pass at strict dependencies #1508

Conversation

matanlurey commented Jan 24, 2017 • edited Loading

matanlurey commented Jan 24, 2017

kevmoo commented Jan 24, 2017

lrhn commented Jan 24, 2017

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matanlurey Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matanlurey commented Jan 27, 2017

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matanlurey commented Jan 24, 2017 •

edited

Loading

matanlurey Jan 25, 2017 •

edited

Loading