From 7d84814918ef5b14c773027365467c79b56d898a Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Thu, 22 Feb 2024 14:58:32 +0000 Subject: [PATCH 1/2] Delay error reporting for dependencies --- lib/src/pubspec.dart | 84 ++++++++++++++++++------------ lib/src/solver/package_lister.dart | 15 ++++-- lib/src/system_cache.dart | 5 ++ test/dependency_override_test.dart | 19 +++++++ test/pubspec_test.dart | 28 ++++++---- 5 files changed, 106 insertions(+), 45 deletions(-) diff --git a/lib/src/pubspec.dart b/lib/src/pubspec.dart index 2bf9250d85..ab314cd502 100644 --- a/lib/src/pubspec.dart +++ b/lib/src/pubspec.dart @@ -14,6 +14,7 @@ import 'language_version.dart'; import 'package_name.dart'; import 'pubspec_parse.dart'; import 'sdk.dart'; +import 'source/error.dart'; import 'system_cache.dart'; export 'pubspec_parse.dart' hide PubspecBase; @@ -442,17 +443,28 @@ class Pubspec extends PubspecBase { /// Returns a list of most errors in this pubspec. /// /// This will return at most one error for each field. - List get allErrors => _collectErrorsFor([ - () => name, - () => version, - () => dependencies, - () => devDependencies, - () => publishTo, - () => executables, - () => falseSecrets, - () => sdkConstraints, - () => ignoredAdvisories, - ]); + List get allErrors { + void errorsOf(Map m) { + for (final PackageRange(description: description) in m.values) { + if (description is ErrorDescription) { + throw description.exception; + } + } + } + + return _collectErrorsFor([ + () => name, + () => version, + () => errorsOf(dependencies), + () => errorsOf(devDependencies), + () => errorsOf(dependencyOverrides), + () => publishTo, + () => executables, + () => falseSecrets, + () => sdkConstraints, + () => ignoredAdvisories, + ]); + } /// Returns the type of dependency from this package onto [name]. DependencyType dependencyType(String? name) { @@ -562,28 +574,34 @@ Map _parseDependencies( specNode.span, ); } - - // Let the source validate the description. - var ref = _wrapFormatException( - 'description', - descriptionNode?.span, - () { - String? pubspecDir; - if (location != null && _isFileUri(location)) { - pubspecDir = path.dirname(path.fromUri(location)); - } - - return sources(sourceName).parseRef( - name, - descriptionNode?.value, - containingDir: pubspecDir, - languageVersion: languageVersion, - ); - }, - packageName, - fileType, - targetPackage: name, - ); + String? pubspecDir; + if (location != null && _isFileUri(location)) { + pubspecDir = path.dirname(path.fromUri(location)); + } + PackageRef ref; + try { + // Let the source validate the description. + ref = sources(sourceName).parseRef( + name, + descriptionNode?.value, + containingDir: pubspecDir, + languageVersion: languageVersion, + ); + } on FormatException catch (e) { + // If we already have a pub exception with a span, re-use that + if (e is SourceSpanApplicationException) rethrow; + final typeName = _fileTypeName(fileType); + + final msg = + 'Invalid description in the "$packageName" $typeName on the "$name" ' + 'dependency: ${e.message}'; + ref = PackageRef( + name, + ErrorDescription( + SourceSpanApplicationException(msg, descriptionNode?.span), + ), + ); + } dependencies[name] = ref.withConstraint(versionConstraint); }, diff --git a/lib/src/solver/package_lister.dart b/lib/src/solver/package_lister.dart index bce663b4c4..1c9d064545 100644 --- a/lib/src/solver/package_lister.dart +++ b/lib/src/solver/package_lister.dart @@ -15,6 +15,7 @@ import '../package.dart'; import '../package_name.dart'; import '../pubspec.dart'; import '../sdk.dart'; +import '../source/error.dart'; import '../system_cache.dart'; import '../utils.dart'; import 'incompatibility.dart'; @@ -253,7 +254,11 @@ class PackageLister { .where((range) => !_overriddenPackages.contains(range.name)), if (id.isRoot) ...pubspec.dependencyOverrides.values, ]; - + for (final PackageRange(description: description) in entries) { + if (description is ErrorDescription) { + throw description.exception; + } + } return entries.map((range) => _dependency(depender, range)).toList(); } @@ -274,12 +279,16 @@ class PackageLister { // Don't recompute dependencies that have already been emitted. var dependencies = Map.from(pubspec.dependencies); - for (var package in dependencies.keys.toList()) { + for (final MapEntry(key: package, value: range) + in Map.from(dependencies).entries) { if (_overriddenPackages.contains(package)) { dependencies.remove(package); continue; } - + final description = range.description; + if (description is ErrorDescription) { + throw description.exception; + } var constraint = _alreadyListedDependencies[package]; if (constraint != null && constraint.allows(id.version)) { dependencies.remove(package); diff --git a/lib/src/system_cache.dart b/lib/src/system_cache.dart index f8204b0e87..5a39da5e09 100644 --- a/lib/src/system_cache.dart +++ b/lib/src/system_cache.dart @@ -17,6 +17,7 @@ import 'package_name.dart'; import 'pubspec.dart'; import 'source.dart'; import 'source/cached.dart'; +import 'source/error.dart'; import 'source/git.dart'; import 'source/hosted.dart'; import 'source/path.dart'; @@ -185,6 +186,10 @@ Consider setting the `PUB_CACHE` variable manually. Duration? maxAge, Version? allowedRetractedVersion, }) async { + final description = ref.description; + if (description is ErrorDescription) { + throw description.exception; + } if (ref.isRoot) { throw ArgumentError('Cannot get versions for the root package.'); } diff --git a/test/dependency_override_test.dart b/test/dependency_override_test.dart index 2898e80200..6ae1bed7a1 100644 --- a/test/dependency_override_test.dart +++ b/test/dependency_override_test.dart @@ -133,4 +133,23 @@ void main() { ); }); }); + + test('can override package with faulty description', () async { + final server = await servePackages(); + server.serve( + 'foo', + '1.0.0', + deps: { + 'bar': {'path': '../abc'}, + }, + ); + server.serve('bar', '1.0.0'); + await d.appDir( + dependencies: {'foo': '^1.0.0'}, + pubspec: { + 'dependency_overrides': {'bar': '1.0.0'}, + }, + ).create(); + await pubGet(); + }); } diff --git a/test/pubspec_test.dart b/test/pubspec_test.dart index a5456277e7..5bed321820 100644 --- a/test/pubspec_test.dart +++ b/test/pubspec_test.dart @@ -5,7 +5,9 @@ import 'dart:io'; import 'package:pub/src/exceptions.dart'; +import 'package:pub/src/package_name.dart'; import 'package:pub/src/pubspec.dart'; +import 'package:pub/src/source/error.dart'; import 'package:pub/src/source/hosted.dart'; import 'package:pub/src/system_cache.dart'; import 'package:pub_semver/pub_semver.dart'; @@ -246,7 +248,7 @@ dependencies: name: foo url: '::' ''', - (pubspec) => pubspec.dependencies, + (pubspec) => errorsOf(pubspec.dependencies), ); }); @@ -428,7 +430,7 @@ name: pkg dependencies: from_path: {path: non_local_path} ''', - (pubspec) => pubspec.dependencies, + (pubspec) => errorsOf(pubspec.dependencies), 'Invalid description in the "pkg" pubspec on the "from_path" ' 'dependency: "non_local_path" is a relative path, but this isn\'t a ' 'local pubspec.'); @@ -603,7 +605,7 @@ dependencies: hosted: url: https://example.org/pub/ ''', - (pubspec) => pubspec.dependencies, + (pubspec) => errorsOf(pubspec.dependencies), "The 'name' key must have a string value without a minimum Dart " 'SDK constraint of 2.15.'); }); @@ -618,7 +620,7 @@ dependencies: foo: hosted: http://pub.example.org ''', - (pubspec) => pubspec.dependencies, + (pubspec) => errorsOf(pubspec.dependencies), 'Using `hosted: ` is only supported with a minimum SDK constraint of 2.15.', ); }, @@ -636,7 +638,7 @@ dependencies: url: git://github.com/dart-lang/foo path: 12 ''', - (pubspec) => pubspec.dependencies, + (pubspec) => errorsOf(pubspec.dependencies), ); }); @@ -649,7 +651,7 @@ dependencies: url: git://github.com/dart-lang/foo path: git://github.com/dart-lang/foo/bar ''', - (pubspec) => pubspec.dependencies, + (pubspec) => errorsOf(pubspec.dependencies), ); expectPubspecException( @@ -660,7 +662,7 @@ dependencies: url: git://github.com/dart-lang/foo path: /foo ''', - (pubspec) => pubspec.dependencies, + (pubspec) => errorsOf(pubspec.dependencies), ); }); @@ -673,7 +675,7 @@ dependencies: url: git://github.com/dart-lang/foo path: foo/../../bar ''', - (pubspec) => pubspec.dependencies, + (pubspec) => errorsOf(pubspec.dependencies), ); }); }); @@ -1005,7 +1007,7 @@ dependency_overrides: name: foo url: '::' ''', - (pubspecOverrides) => pubspecOverrides.dependencyOverrides, + (pubspecOverrides) => errorsOf(pubspecOverrides.dependencyOverrides), 'Error on line 4, column 7 of ${Platform.pathSeparator}pubspec_overrides.yaml', ); }); @@ -1030,3 +1032,11 @@ name: 'foo' }); }); } + +void errorsOf(Map m) { + for (final PackageRange(description: description) in m.values) { + if (description is ErrorDescription) { + throw description.exception; + } + } +} From 85c8e74a79986ebe36f153ba9d00e7dcf3720192 Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Thu, 22 Feb 2024 15:35:23 +0000 Subject: [PATCH 2/2] Missing file --- lib/src/source/error.dart | 40 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 lib/src/source/error.dart diff --git a/lib/src/source/error.dart b/lib/src/source/error.dart new file mode 100644 index 0000000000..06ce5ebf94 --- /dev/null +++ b/lib/src/source/error.dart @@ -0,0 +1,40 @@ +// Copyright (c) 2024, 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 '../language_version.dart'; +import '../source.dart'; +import 'unknown.dart'; + +/// Represents a bad dependency description. +/// +/// Allows for postponing the error until the dependency is actually read. +class ErrorDescription extends Description { + final Exception exception; + + ErrorDescription(this.exception); + + @override + String format() { + throw UnimplementedError(); + } + + @override + bool operator ==(Object other) { + return identical(other, this); + } + + @override + int get hashCode => identityHashCode(this); + + @override + Object? serializeForPubspec({ + required String? containingDir, + required LanguageVersion languageVersion, + }) { + throw UnimplementedError(); + } + + @override + Source get source => UnknownSource('Error'); +}