Cleaner pubspec.lock parsing (#3487)
Avoids crashes when lockfile parsing fails.
diff --git a/lib/src/command.dart b/lib/src/command.dart
index e86dd50..4a65fb1 100644
--- a/lib/src/command.dart
+++ b/lib/src/command.dart
@@ -194,7 +194,6 @@
return exit_codes.SUCCESS;
} catch (error, chain) {
log.exception(error, chain);
-
if (_pubTopLevel.trace) {
log.dumpTranscriptToStdErr();
} else if (!isUserFacingException(error)) {
diff --git a/lib/src/entrypoint.dart b/lib/src/entrypoint.dart
index b536cca..36f8047 100644
--- a/lib/src/entrypoint.dart
+++ b/lib/src/entrypoint.dart
@@ -12,6 +12,7 @@
import 'package:path/path.dart' as p;
import 'package:pool/pool.dart';
import 'package:pub_semver/pub_semver.dart';
+import 'package:source_span/source_span.dart';
import 'package:yaml/yaml.dart';
import 'command_runner.dart';
@@ -101,7 +102,17 @@
if (!fileExists(lockFilePath)) {
return _lockFile = LockFile.empty();
} else {
- return _lockFile = LockFile.load(lockFilePath, cache.sources);
+ try {
+ return _lockFile = LockFile.load(lockFilePath, cache.sources);
+ } on SourceSpanException catch (e) {
+ throw SourceSpanApplicationException(
+ e.message,
+ e.span,
+ explanation: 'Failed parsing lock file:',
+ hint:
+ 'Consider deleting the file and running `$topLevelProgram pub get` to recreate it.',
+ );
+ }
}
}
@@ -898,7 +909,7 @@
final keyNode = environment.nodes.entries
.firstWhere((e) => (e.key as YamlNode).value == sdk)
.key as YamlNode;
- throw PubspecException('''
+ throw SourceSpanApplicationException('''
$pubspecPath refers to an unknown sdk '$sdk'.
Did you mean to add it as a dependency?
diff --git a/lib/src/exceptions.dart b/lib/src/exceptions.dart
index ec9f876..39e9c51 100644
--- a/lib/src/exceptions.dart
+++ b/lib/src/exceptions.dart
@@ -7,6 +7,7 @@
import 'package:args/command_runner.dart';
import 'package:http/http.dart' as http;
+import 'package:source_span/source_span.dart';
import 'package:stack_trace/stack_trace.dart';
import 'package:yaml/yaml.dart';
@@ -127,3 +128,34 @@
error is YamlException ||
error is UsageException;
}
+
+/// An exception thrown when parsing a `pubspec.yaml` or a `pubspec.lock`.
+///
+/// These exceptions are often thrown lazily while accessing pubspec properties.
+///
+/// By being an [ApplicationException] this will not trigger a stack-trace on
+/// normal operations.
+///
+/// Works as a [SourceSpanFormatException], but can contain more context:
+/// An optional [explanation] that explains the operation that failed.
+/// An optional [hint] that gives suggestions how to proceed.
+class SourceSpanApplicationException extends SourceSpanFormatException
+ implements ApplicationException {
+ final String? explanation;
+ final String? hint;
+
+ SourceSpanApplicationException(String message, SourceSpan? span,
+ {this.hint, this.explanation})
+ : super(message, span);
+
+ @override
+ String toString({color}) {
+ return [
+ if (explanation != null) explanation,
+ span == null
+ ? message
+ : 'Error on ${span?.message(message, color: color)}',
+ if (hint != null) hint,
+ ].join('\n\n');
+ }
+}
diff --git a/lib/src/lock_file.dart b/lib/src/lock_file.dart
index 46be2bf..f133172 100644
--- a/lib/src/lock_file.dart
+++ b/lib/src/lock_file.dart
@@ -98,50 +98,54 @@
Uri? sourceUrl;
if (filePath != null) sourceUrl = p.toUri(filePath);
- var parsed = loadYamlNode(contents, sourceUrl: sourceUrl);
+ final parsed = _parseNode<YamlMap>(
+ loadYamlNode(contents, sourceUrl: sourceUrl),
+ 'YAML mapping',
+ );
- _validate(parsed is Map, 'The lockfile must be a YAML mapping.', parsed);
- var parsedMap = parsed as YamlMap;
-
- var sdkConstraints = <String, VersionConstraint>{};
- var sdkNode = parsedMap.nodes['sdk'];
+ final sdkConstraints = <String, VersionConstraint>{};
+ final sdkNode =
+ _getEntry<YamlScalar?>(parsed, 'sdk', 'string', required: false);
if (sdkNode != null) {
// Lockfiles produced by pub versions from 1.14.0 through 1.18.0 included
// a top-level "sdk" field which encoded the unified constraint on the
// Dart SDK. They had no way of specifying constraints on other SDKs.
sdkConstraints['dart'] = _parseVersionConstraint(sdkNode);
- } else if (parsedMap.containsKey('sdks')) {
- var sdksField = parsedMap['sdks'];
- _validate(sdksField is Map, 'The "sdks" field must be a mapping.',
- parsedMap.nodes['sdks']);
-
- sdksField.nodes.forEach((name, constraint) {
- _validate(name.value is String, 'SDK names must be strings.', name);
- sdkConstraints[name.value as String] =
- _parseVersionConstraint(constraint);
- });
}
- var packages = <String, PackageId>{};
- var packageEntries = parsedMap['packages'];
- if (packageEntries != null) {
- _validate(packageEntries is Map, 'The "packages" field must be a map.',
- parsedMap.nodes['packages']);
+ final sdksField =
+ _getEntry<YamlMap?>(parsed, 'sdks', 'map', required: false);
- packageEntries.forEach((name, spec) {
+ if (sdksField != null) {
+ _parseEachEntry<String, YamlScalar>(sdksField, (name, constraint) {
+ sdkConstraints[name] = _parseVersionConstraint(constraint);
+ }, 'string', 'string');
+ }
+
+ final packages = <String, PackageId>{};
+
+ final mainDependencies = <String>{};
+ final devDependencies = <String>{};
+ final overriddenDependencies = <String>{};
+
+ final packageEntries =
+ _getEntry<YamlMap?>(parsed, 'packages', 'map', required: false);
+
+ if (packageEntries != null) {
+ _parseEachEntry<String, YamlMap>(packageEntries, (name, spec) {
// Parse the version.
- _validate(spec.containsKey('version'),
- 'Package $name is missing a version.', spec);
- var version = Version.parse(spec['version']);
+ final versionEntry = _getStringEntry(spec, 'version');
+ final version = Version.parse(versionEntry);
// Parse the source.
- _validate(spec.containsKey('source'),
- 'Package $name is missing a source.', spec);
- var sourceName = spec['source'];
+ final sourceName = _getStringEntry(spec, 'source');
- _validate(spec.containsKey('description'),
- 'Package $name is missing a description.', spec);
- var description = spec['description'];
+ var descriptionNode =
+ _getEntry<YamlNode>(spec, 'description', 'description');
+
+ dynamic description = descriptionNode is YamlScalar
+ ? descriptionNode.value
+ : descriptionNode;
// Let the source parse the description.
var source = sources(sourceName);
@@ -150,18 +154,30 @@
id = source.parseId(name, version, description,
containingDir: filePath == null ? null : p.dirname(filePath));
} on FormatException catch (ex) {
- throw SourceSpanFormatException(
- ex.message, spec.nodes['description'].span);
+ _failAt(ex.message, spec.nodes['description']!);
}
// Validate the name.
- _validate(name == id.name,
- "Package name $name doesn't match ${id.name}.", spec);
+ if (name != id.name) {
+ _failAt("Package name $name doesn't match ${id.name}.", spec);
+ }
packages[name] = id;
- });
+ if (spec.containsKey('dependency')) {
+ final dependencyKind = _getStringEntry(spec, 'dependency');
+ switch (dependencyKind) {
+ case _directMain:
+ mainDependencies.add(name);
+ break;
+ case _directDev:
+ devDependencies.add(name);
+ break;
+ case _directOverridden:
+ overriddenDependencies.add(name);
+ }
+ }
+ }, 'string', 'map');
}
-
return LockFile._(
packages,
sdkConstraints,
@@ -170,15 +186,6 @@
const UnmodifiableSetView.empty());
}
- /// Asserts that [node] is a version constraint, and parses it.
- static VersionConstraint _parseVersionConstraint(YamlNode node) {
- _validate(node.value is String,
- 'Invalid version constraint: must be a string.', node);
-
- return _wrapFormatException('version constraint', node.span,
- () => VersionConstraint.parse(node.value));
- }
-
/// Runs [fn] and wraps any [FormatException] it throws in a
/// [SourceSpanFormatException].
///
@@ -195,10 +202,68 @@
}
}
- /// If [condition] is `false` throws a format error with [message] for [node].
- static void _validate(bool condition, String message, YamlNode? node) {
- if (condition) return;
- throw SourceSpanFormatException(message, node!.span);
+ static VersionConstraint _parseVersionConstraint(YamlNode node) {
+ return _parseNode(node, 'version constraint',
+ parse: VersionConstraint.parse);
+ }
+
+ static String _getStringEntry(YamlMap map, String key) {
+ return _parseNode<String>(
+ _getEntry<YamlScalar>(map, key, 'string'), 'string');
+ }
+
+ static T _parseNode<T>(YamlNode node, String typeDescription,
+ {T Function(String)? parse}) {
+ if (node is T) {
+ return node as T;
+ } else if (node is YamlScalar) {
+ final value = node.value;
+ if (parse != null) {
+ if (value is! String) {
+ _failAt('Expected a $typeDescription.', node);
+ }
+ return _wrapFormatException(
+ 'Expected a $typeDescription.', node.span, () => parse(node.value));
+ } else if (value is T) {
+ return value;
+ }
+ _failAt('Expected a $typeDescription.', node);
+ }
+ _failAt('Expected a $typeDescription.', node);
+ }
+
+ static void _parseEachEntry<K, V>(
+ YamlMap map,
+ void Function(K key, V value) f,
+ String keyTypeDescription,
+ String valueTypeDescription) {
+ map.nodes.forEach((key, value) {
+ f(_parseNode(key, keyTypeDescription),
+ _parseNode(value, valueTypeDescription));
+ });
+ }
+
+ static T _getEntry<T>(
+ YamlMap map,
+ String key,
+ String type, {
+ bool required = true,
+ }) {
+ final entry = map.nodes[key];
+ // `null` here always means not present. A value explicitly mapped to `null`
+ // would be a `YamlScalar(null)`.
+ if (entry == null) {
+ if (required) {
+ _failAt('Expected a `$key` entry.', map);
+ } else {
+ return null as T;
+ }
+ }
+ return _parseNode(entry, type);
+ }
+
+ static Never _failAt(String message, YamlNode node) {
+ throw SourceSpanFormatException(message, node.span);
}
/// Returns a copy of this LockFile with a package named [name] removed.
@@ -304,17 +369,22 @@
''';
}
+ static const _directMain = 'direct main';
+ static const _directDev = 'direct dev';
+ static const _directOverridden = 'direct overridden';
+ static const _transitive = 'transitive';
+
/// Returns the dependency classification for [package].
String _dependencyType(String package) {
- if (_mainDependencies.contains(package)) return 'direct main';
- if (_devDependencies.contains(package)) return 'direct dev';
+ if (_mainDependencies.contains(package)) return _directMain;
+ if (_devDependencies.contains(package)) return _directDev;
// If a package appears in `dependency_overrides` and another dependency
// section, the main section it appears in takes precedence.
if (_overriddenDependencies.contains(package)) {
- return 'direct overridden';
+ return _directOverridden;
}
- return 'transitive';
+ return _transitive;
}
/// `true` if [other] has the same packages as `this` in the same versions
diff --git a/lib/src/pubspec.dart b/lib/src/pubspec.dart
index 0b6cd5b..6e1d566 100644
--- a/lib/src/pubspec.dart
+++ b/lib/src/pubspec.dart
@@ -133,7 +133,7 @@
if (pubspecOverridesFields != null) {
pubspecOverridesFields.nodes.forEach((key, _) {
if (!const {'dependency_overrides'}.contains(key.value)) {
- throw PubspecException(
+ throw SourceSpanApplicationException(
'pubspec_overrides.yaml only supports the `dependency_overrides` field.',
key.span,
);
@@ -293,7 +293,7 @@
/// Loads the pubspec for a package located in [packageDir].
///
/// If [expectedName] is passed and the pubspec doesn't have a matching name
- /// field, this will throw a [PubspecException].
+ /// field, this will throw a [SourceSpanApplicationException].
///
/// If [allowOverridesFile] is `true` [pubspecOverridesFilename] is loaded and
/// is allowed to override dependency_overrides from `pubspec.yaml`.
@@ -373,7 +373,7 @@
if (expectedName == null) return;
if (name == expectedName) return;
- throw PubspecException(
+ throw SourceSpanApplicationException(
'"name" field doesn\'t match expected name '
'"$expectedName".',
this.fields.nodes['name']!.span);
@@ -400,7 +400,7 @@
loadYamlNode(overridesFileContents, sourceUrl: overridesLocation));
}
} on YamlException catch (error) {
- throw PubspecException(error.message, error.span);
+ throw SourceSpanApplicationException(error.message, error.span);
}
return Pubspec.fromMap(pubspecMap, sources,
@@ -420,19 +420,20 @@
} else if (node is YamlMap) {
return node;
} else {
- throw PubspecException('The pubspec must be a YAML mapping.', node.span);
+ throw SourceSpanApplicationException(
+ 'The pubspec must be a YAML mapping.', node.span);
}
}
/// Returns a list of most errors in this pubspec.
///
/// This will return at most one error for each field.
- List<PubspecException> get allErrors {
- var errors = <PubspecException>[];
+ List<SourceSpanApplicationException> get allErrors {
+ var errors = <SourceSpanApplicationException>[];
void collectError(void Function() fn) {
try {
fn();
- } on PubspecException catch (e) {
+ } on SourceSpanApplicationException catch (e) {
errors.add(e);
}
}
@@ -591,7 +592,7 @@
}
/// Runs [fn] and wraps any [FormatException] it throws in a
-/// [PubspecException].
+/// [SourceSpanApplicationException].
///
/// [description] should be a noun phrase that describes whatever's being
/// parsed or processed by [fn]. [span] should be the location of whatever's
@@ -611,7 +612,7 @@
return fn();
} on FormatException catch (e) {
// If we already have a pub exception with a span, re-use that
- if (e is PubspecException) rethrow;
+ if (e is SourceSpanApplicationException) rethrow;
var msg = 'Invalid $description';
final typeName = _fileTypeName(fileType);
@@ -624,9 +625,9 @@
}
}
-/// Throws a [PubspecException] with the given message.
+/// Throws a [SourceSpanApplicationException] with the given message.
Never _error(String message, SourceSpan? span) {
- throw PubspecException(message, span);
+ throw SourceSpanApplicationException(message, span);
}
enum _FileType {
diff --git a/lib/src/pubspec_parse.dart b/lib/src/pubspec_parse.dart
index c86a706..c6babfb 100644
--- a/lib/src/pubspec_parse.dart
+++ b/lib/src/pubspec_parse.dart
@@ -7,7 +7,7 @@
import 'package:source_span/source_span.dart';
import 'package:yaml/yaml.dart';
-import 'exceptions.dart' show ApplicationException;
+import 'exceptions.dart';
import 'utils.dart' show identifierRegExp, reservedWords;
/// A regular expression matching allowed package names.
@@ -42,15 +42,18 @@
String _lookupName() {
final name = fields['name'];
if (name == null) {
- throw PubspecException('Missing the required "name" field.', fields.span);
+ throw SourceSpanApplicationException(
+ 'Missing the required "name" field.', fields.span);
} else if (name is! String) {
- throw PubspecException(
+ throw SourceSpanApplicationException(
'"name" field must be a string.', fields.nodes['name']?.span);
} else if (!packageNameRegExp.hasMatch(name)) {
- throw PubspecException('"name" field must be a valid Dart identifier.',
+ throw SourceSpanApplicationException(
+ '"name" field must be a valid Dart identifier.',
fields.nodes['name']?.span);
} else if (reservedWords.contains(name)) {
- throw PubspecException('"name" field may not be a Dart reserved word.',
+ throw SourceSpanApplicationException(
+ '"name" field may not be a Dart reserved word.',
fields.nodes['name']?.span);
}
@@ -223,7 +226,7 @@
bool get isPrivate => publishTo == 'none';
/// Runs [fn] and wraps any [FormatException] it throws in a
- /// [PubspecException].
+ /// [SourceSpanApplicationException].
///
/// [description] should be a noun phrase that describes whatever's being
/// parsed or processed by [fn]. [span] should be the location of whatever's
@@ -242,21 +245,13 @@
msg = '$msg in the "$name" pubspec on the "$targetPackage" dependency';
}
msg = '$msg: ${e.message}';
- throw PubspecException(msg, span);
+ throw SourceSpanApplicationException(msg, span);
}
}
- /// Throws a [PubspecException] with the given message.
+ /// Throws a [SourceSpanApplicationException] with the given message.
@alwaysThrows
void _error(String message, SourceSpan? span) {
- throw PubspecException(message, span);
+ throw SourceSpanApplicationException(message, span);
}
}
-
-/// An exception thrown when parsing a pubspec.
-///
-/// These exceptions are often thrown lazily while accessing pubspec properties.
-class PubspecException extends SourceSpanFormatException
- implements ApplicationException {
- PubspecException(String message, SourceSpan? span) : super(message, span);
-}
diff --git a/lib/src/solver/package_lister.dart b/lib/src/solver/package_lister.dart
index ce2b4c1..1597e73 100644
--- a/lib/src/solver/package_lister.dart
+++ b/lib/src/solver/package_lister.dart
@@ -190,7 +190,7 @@
try {
pubspec = await withDependencyType(
_dependencyType, () => _systemCache.describe(id));
- } on PubspecException catch (error) {
+ } on SourceSpanApplicationException catch (error) {
// The lockfile for the pubspec couldn't be parsed,
log.fine('Failed to parse pubspec for $id:\n$error');
_knownInvalidVersions = _knownInvalidVersions.union(id.version);
diff --git a/test/list_package_dirs/lockfile_error_test.dart b/test/list_package_dirs/lockfile_error_test.dart
index f4440ec..5c7bff9 100644
--- a/test/list_package_dirs/lockfile_error_test.dart
+++ b/test/list_package_dirs/lockfile_error_test.dart
@@ -19,7 +19,7 @@
'list-package-dirs',
'--format=json'
], outputJson: {
- 'error': contains('The lockfile must be a YAML mapping.'),
+ 'error': contains('Expected a YAML mapping.'),
'path': canonicalize(path.join(d.sandbox, appPath, 'pubspec.lock'))
}, exitCode: exit_codes.DATA);
});
diff --git a/test/pubspec_test.dart b/test/pubspec_test.dart
index 2ccb30f..1a9ac48 100644
--- a/test/pubspec_test.dart
+++ b/test/pubspec_test.dart
@@ -4,6 +4,7 @@
import 'dart:io';
+import 'package:pub/src/exceptions.dart';
import 'package:pub/src/pubspec.dart';
import 'package:pub/src/sdk.dart';
import 'package:pub/src/source/hosted.dart';
@@ -15,11 +16,12 @@
group('parse()', () {
final sources = SystemCache().sources;
- var throwsPubspecException = throwsA(const TypeMatcher<PubspecException>());
+ var throwsPubspecException =
+ throwsA(const TypeMatcher<SourceSpanApplicationException>());
void expectPubspecException(String contents, void Function(Pubspec) fn,
[String? expectedContains]) {
- var expectation = const TypeMatcher<PubspecException>();
+ var expectation = const TypeMatcher<SourceSpanApplicationException>();
if (expectedContains != null) {
expectation = expectation.having(
(error) => error.message, 'message', contains(expectedContains));
@@ -390,7 +392,7 @@
expect(
() => pubspec.dependencies,
throwsA(
- isA<PubspecException>()
+ isA<SourceSpanApplicationException>()
.having((e) => e.span!.text, 'span.text', 'invalid value'),
),
);
@@ -700,7 +702,7 @@
void Function(Pubspec) fn, [
String? expectedContains,
]) {
- var expectation = isA<PubspecException>();
+ var expectation = isA<SourceSpanApplicationException>();
if (expectedContains != null) {
expectation = expectation.having((error) => error.toString(),
'toString()', contains(expectedContains));
