check for invalid git and path deps in publishable packages
Fixes: https://github.com/dart-lang/sdk/issues/43930
Change-Id: Ia1e7899cf04644387f2686e997e338baa197dac3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169246
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
diff --git a/pkg/analyzer/lib/error/error.dart b/pkg/analyzer/lib/error/error.dart
index 30ed3d0..0ef80e5 100644
--- a/pkg/analyzer/lib/error/error.dart
+++ b/pkg/analyzer/lib/error/error.dart
@@ -16,6 +16,7 @@
import 'package:analyzer/src/generated/parser.dart' show ParserErrorCode;
import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/manifest/manifest_warning_code.dart';
+import 'package:analyzer/src/pubspec/pubspec_warning_code.dart';
export 'package:_fe_analyzer_shared/src/base/errors.dart'
show ErrorCode, ErrorSeverity, ErrorType;
@@ -795,6 +796,9 @@
ParserErrorCode.WITH_BEFORE_EXTENDS,
ParserErrorCode.WRONG_SEPARATOR_FOR_POSITIONAL_PARAMETER,
ParserErrorCode.WRONG_TERMINATOR_FOR_PARAMETER_GROUP,
+ PubspecWarningCode.INVALID_DEPENDENCY,
+ PubspecWarningCode.PATH_DOES_NOT_EXIST,
+ PubspecWarningCode.PATH_PUBSPEC_DOES_NOT_EXIST,
ScannerErrorCode.EXPECTED_TOKEN,
ScannerErrorCode.ILLEGAL_CHARACTER,
ScannerErrorCode.MISSING_DIGIT,
diff --git a/pkg/analyzer/lib/src/pubspec/pubspec_validator.dart b/pkg/analyzer/lib/src/pubspec/pubspec_validator.dart
index 83c2031..af82bcf 100644
--- a/pkg/analyzer/lib/src/pubspec/pubspec_validator.dart
+++ b/pkg/analyzer/lib/src/pubspec/pubspec_validator.dart
@@ -8,6 +8,7 @@
import 'package:analyzer/src/generated/engine.dart';
import 'package:analyzer/src/generated/source.dart';
import 'package:analyzer/src/pubspec/pubspec_warning_code.dart';
+import 'package:analyzer/src/util/yaml.dart';
import 'package:path/path.dart' as path;
import 'package:source_span/src/span.dart';
import 'package:yaml/yaml.dart';
@@ -27,12 +28,21 @@
/// configuration data.
static const String FLUTTER_FIELD = 'flutter';
+ /// The name of the field whose value is a git dependency.
+ static const String GIT_FIELD = 'git';
+
/// The name of the field whose value is the name of the package.
static const String NAME_FIELD = 'name';
/// The name of the field whose value is a path to a package dependency.
static const String PATH_FIELD = 'path';
+ /// The name of the field whose value is the where to publish the package.
+ static const String PUBLISH_TO_FIELD = 'publish_to';
+
+ /// The name of the field whose value is the version of the package.
+ static const String VERSION_FIELD = 'version';
+
/// The resource provider used to access the file system.
final ResourceProvider provider;
@@ -89,6 +99,16 @@
return false;
}
+ String _asString(dynamic node) {
+ if (node is String) {
+ return node;
+ }
+ if (node is YamlScalar && node.value is String) {
+ return node.value as String;
+ }
+ return null;
+ }
+
/// Return a map whose keys are the names of declared dependencies and whose
/// values are the specifications of those dependencies. The map is extracted
/// from the given [contents] using the given [key].
@@ -122,8 +142,17 @@
Map<dynamic, YamlNode> declaredDevDependencies =
_getDeclaredDependencies(reporter, contents, DEV_DEPENDENCIES_FIELD);
+ bool isPublishablePackage = false;
+ var version = contents[VERSION_FIELD];
+ if (version != null) {
+ var publishTo = _asString(contents[PUBLISH_TO_FIELD]);
+ if (publishTo != 'none') {
+ isPublishablePackage = true;
+ }
+ }
+
for (var dependency in declaredDependencies.entries) {
- _validatePathEntries(reporter, dependency.value);
+ _validatePathEntries(reporter, dependency.value, isPublishablePackage);
}
for (var dependency in declaredDevDependencies.entries) {
@@ -132,7 +161,7 @@
_reportErrorForNode(reporter, packageName,
PubspecWarningCode.UNNECESSARY_DEV_DEPENDENCY, [packageName.value]);
}
- _validatePathEntries(reporter, dependency.value);
+ _validatePathEntries(reporter, dependency.value, false);
}
}
@@ -209,33 +238,44 @@
/// Valid paths are directories that:
///
/// 1. exist,
- /// 2. contain a pubspec.yaml file, and
- /// 3. `lib` dir
- void _validatePathEntries(ErrorReporter reporter, YamlNode dependency) {
+ /// 2. contain a pubspec.yaml file
+ ///
+ /// If [checkForPathAndGitDeps] is true, `git` or `path` dependencies will
+ /// be marked invalid.
+ void _validatePathEntries(ErrorReporter reporter, YamlNode dependency,
+ bool checkForPathAndGitDeps) {
if (dependency is YamlMap) {
- for (var node in dependency.nodes.entries) {
- var pathEntry = dependency[PATH_FIELD];
- if (pathEntry != null) {
- var context = provider.pathContext;
- var normalizedPath = context.joinAll(path.posix.split(pathEntry));
- var packageRoot = context.dirname(source.fullName);
- var dependencyPath = context.join(packageRoot, normalizedPath);
- dependencyPath = context.absolute(dependencyPath);
- dependencyPath = context.normalize(dependencyPath);
- var packageFolder = provider.getFolder(dependencyPath);
- if (!packageFolder.exists) {
- _reportErrorForNode(reporter, node.value,
- PubspecWarningCode.PATH_DOES_NOT_EXIST, [pathEntry]);
- } else {
- if (!packageFolder
- .getChild(AnalysisEngine.PUBSPEC_YAML_FILE)
- .exists) {
- _reportErrorForNode(reporter, node.value,
- PubspecWarningCode.PATH_PUBSPEC_DOES_NOT_EXIST, [pathEntry]);
- }
- // todo (pq): test for presence of a lib dir.
+ var pathEntry = _asString(dependency[PATH_FIELD]);
+ if (pathEntry != null) {
+ YamlNode pathKey() => getKey(dependency, PATH_FIELD);
+ var context = provider.pathContext;
+ var normalizedPath = context.joinAll(path.posix.split(pathEntry));
+ var packageRoot = context.dirname(source.fullName);
+ var dependencyPath = context.join(packageRoot, normalizedPath);
+ dependencyPath = context.absolute(dependencyPath);
+ dependencyPath = context.normalize(dependencyPath);
+ var packageFolder = provider.getFolder(dependencyPath);
+ if (!packageFolder.exists) {
+ _reportErrorForNode(reporter, pathKey(),
+ PubspecWarningCode.PATH_DOES_NOT_EXIST, [pathEntry]);
+ } else {
+ if (!packageFolder
+ .getChild(AnalysisEngine.PUBSPEC_YAML_FILE)
+ .exists) {
+ _reportErrorForNode(reporter, pathKey(),
+ PubspecWarningCode.PATH_PUBSPEC_DOES_NOT_EXIST, [pathEntry]);
}
}
+ if (checkForPathAndGitDeps) {
+ _reportErrorForNode(reporter, pathKey(),
+ PubspecWarningCode.INVALID_DEPENDENCY, [PATH_FIELD]);
+ }
+ }
+
+ var gitEntry = dependency[GIT_FIELD];
+ if (gitEntry != null && checkForPathAndGitDeps) {
+ _reportErrorForNode(reporter, getKey(dependency, GIT_FIELD),
+ PubspecWarningCode.INVALID_DEPENDENCY, [GIT_FIELD]);
}
}
}
diff --git a/pkg/analyzer/lib/src/pubspec/pubspec_warning_code.dart b/pkg/analyzer/lib/src/pubspec/pubspec_warning_code.dart
index ecc7124..a93cca0 100644
--- a/pkg/analyzer/lib/src/pubspec/pubspec_warning_code.dart
+++ b/pkg/analyzer/lib/src/pubspec/pubspec_warning_code.dart
@@ -52,6 +52,17 @@
"The value of the 'flutter' field is expected to be a map.",
correction: "Try converting the value to be a map.");
+ /// A code indicating that a versioned package has an invalid dependency (git
+ /// or path).
+ ///
+ /// Parameters:
+ /// 0: the kind of dependency.
+ static const PubspecWarningCode INVALID_DEPENDENCY = PubspecWarningCode(
+ 'INVALID_DEPENDENCY', "Publishable packages can't have {0} dependencies.",
+ correction:
+ "Try adding a 'publish_to: none' entry to mark the package as not "
+ "for publishing or remove the {0} dependency.");
+
/// A code indicating that the name field is missing.
static const PubspecWarningCode MISSING_NAME = PubspecWarningCode(
'MISSING_NAME', "The name field is required but missing.",
diff --git a/pkg/analyzer/test/src/pubspec/pubspec_validator_test.dart b/pkg/analyzer/test/src/pubspec/pubspec_validator_test.dart
index 32f7cf3..8e8944a 100644
--- a/pkg/analyzer/test/src/pubspec/pubspec_validator_test.dart
+++ b/pkg/analyzer/test/src/pubspec/pubspec_validator_test.dart
@@ -196,6 +196,73 @@
''');
}
+ test_dependencyGit_malformed_empty() {
+ // todo (pq): consider validating.
+ assertNoErrors('''
+name: sample
+dependencies:
+ foo:
+ git:
+''');
+ }
+
+ test_dependencyGit_malformed_list() {
+ // todo (pq): consider validating.
+ assertNoErrors('''
+name: sample
+dependencies:
+ foo:
+ git:
+ - baz
+''');
+ }
+
+ test_dependencyGit_malformed_scalar() {
+ // todo (pq): consider validating.
+ assertNoErrors('''
+name: sample
+dependencies:
+ foo:
+ git: baz
+''');
+ }
+
+ test_dependencyGit_noVersion_valid() {
+ assertNoErrors('''
+name: sample
+dependencies:
+ foo:
+ git:
+ url: git@github.com:foo/foo.git
+ path: path/to/foo
+''');
+ }
+
+ test_dependencyGit_version_error() {
+ assertErrors('''
+name: sample
+version: 0.1.0
+dependencies:
+ foo:
+ git:
+ url: git@github.com:foo/foo.git
+ path: path/to/foo
+''', [PubspecWarningCode.INVALID_DEPENDENCY]);
+ }
+
+ test_dependencyGit_version_valid() {
+ assertNoErrors('''
+name: sample
+version: 0.1.0
+publish_to: none
+dependencies:
+ foo:
+ git:
+ url: git@github.com:foo/foo.git
+ path: path/to/foo
+''');
+ }
+
test_dependencyGitPath() {
// git paths are not validated
assertNoErrors('''
@@ -208,6 +275,40 @@
''');
}
+ test_dependencyPath_malformed_empty() {
+ // todo (pq): consider validating.
+ assertNoErrors('''
+name: sample
+dependencies:
+ foo:
+ path:
+''');
+ }
+
+ test_dependencyPath_malformed_list() {
+ // todo (pq): consider validating.
+ assertNoErrors('''
+name: sample
+dependencies:
+ foo:
+ path:
+ - baz
+''');
+ }
+
+ test_dependencyPath_noVersion_valid() {
+ newFolder('/foo');
+ newFile('/foo/pubspec.yaml', content: '''
+name: foo
+''');
+ assertNoErrors('''
+name: sample
+dependencies:
+ foo:
+ path: /foo
+''');
+ }
+
test_dependencyPath_pubspecDoesNotExist() {
newFolder('/foo');
assertErrors('''
@@ -257,6 +358,35 @@
''');
}
+ test_dependencyPath_version_error() {
+ newFolder('/foo');
+ newFile('/foo/pubspec.yaml', content: '''
+name: foo
+''');
+ assertErrors('''
+name: sample
+version: 0.1.0
+dependencies:
+ foo:
+ path: /foo
+''', [PubspecWarningCode.INVALID_DEPENDENCY]);
+ }
+
+ test_dependencyPath_version_valid() {
+ newFolder('/foo');
+ newFile('/foo/pubspec.yaml', content: '''
+name: foo
+''');
+ assertNoErrors('''
+name: sample
+version: 0.1.0
+publish_to: none
+dependencies:
+ foo:
+ path: /foo
+''');
+ }
+
test_dependencyPathDoesNotExist_path_error() {
assertErrors('''
name: sample
@@ -288,6 +418,19 @@
''');
}
+ test_devDependencyGit_version_no_error() {
+ // Git paths are OK in dev_dependencies
+ assertNoErrors('''
+name: sample
+version: 0.1.0
+dev_dependencies:
+ foo:
+ git:
+ url: git@github.com:foo/foo.git
+ path: path/to/foo
+''');
+ }
+
test_devDependencyPathDoesNotExist_path_error() {
assertErrors('''
name: sample
