Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(aft): Constraints edge cases #3732

Merged
merged 1 commit into from
Sep 12, 2023
Merged

fix(aft): Constraints edge cases #3732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
215 changes: 151 additions & 64 deletions packages/aft/lib/src/constraints_checker.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
import 'package:aft/aft.dart';
import 'package:aws_common/aws_common.dart';
import 'package:collection/collection.dart';
import 'package:path/path.dart' as p;
import 'package:pub_semver/pub_semver.dart';
import 'package:pubspec_parse/pubspec_parse.dart';
import 'package:yaml/yaml.dart';

typedef MismatchedDependency = ({
PackageInfo package,
Expand All @@ -26,7 +28,7 @@ sealed class ConstraintsChecker {
bool checkConstraints(PackageInfo package);

/// Populated by [checkConstraints] if a constraint check fails.
List<MismatchedDependency> get mismatchedDependencies;
final Set<MismatchedDependency> mismatchedDependencies = {};

/// Processes the [expectedConstraint] for [dependencyPath] in [package].
///
Expand All @@ -36,7 +38,7 @@ sealed class ConstraintsChecker {
bool _processConstraint({
required PackageInfo package,
required List<String> dependencyPath,
required VersionConstraint expectedConstraint,
required Dependency expectedConstraint,
required String errorMessage,
}) {
switch (action) {
Expand All @@ -53,7 +55,7 @@ sealed class ConstraintsChecker {
case ConstraintsAction.update:
package.pubspecInfo.pubspecYamlEditor.update(
dependencyPath,
expectedConstraint.toString(),
expectedConstraint.toYaml(),
);
return true;
}
Expand All @@ -70,9 +72,6 @@ final class GlobalConstraintChecker extends ConstraintsChecker {
final Map<String, VersionConstraint> globalConstraints;
final Environment globalEnvironment;

@override
final List<MismatchedDependency> mismatchedDependencies = [];

/// Checks the package's dependency constraints against the global config.
bool _checkDependency(
PackageInfo package,
Expand All @@ -99,7 +98,7 @@ final class GlobalConstraintChecker extends ConstraintsChecker {
return _processConstraint(
package: package,
dependencyPath: [dependencyType.key, dependencyName],
expectedConstraint: globalConstraint,
expectedConstraint: HostedDependency(version: globalConstraint),
errorMessage: 'Expected $globalConstraint\n'
'Found $currentConstraint',
);
Expand All @@ -126,7 +125,7 @@ final class GlobalConstraintChecker extends ConstraintsChecker {
_processConstraint(
package: package,
dependencyPath: ['environment', 'sdk'],
expectedConstraint: globalSdkConstraint,
expectedConstraint: HostedDependency(version: globalSdkConstraint),
errorMessage: 'Expected $globalSdkConstraint\n'
'Found $localSdkConstraint',
);
Expand All @@ -143,7 +142,9 @@ final class GlobalConstraintChecker extends ConstraintsChecker {
_processConstraint(
package: package,
dependencyPath: ['environment', 'flutter'],
expectedConstraint: globalFlutterConstraint,
expectedConstraint: HostedDependency(
version: globalFlutterConstraint,
),
errorMessage: 'Expected $globalFlutterConstraint\n'
'Found $localFlutterConstraint',
);
Expand Down Expand Up @@ -210,9 +211,6 @@ final class PublishConstraintsChecker extends ConstraintsChecker {

final Map<PackageInfo, List<PackageInfo>> repoGraph;

@override
final List<MismatchedDependency> mismatchedDependencies = [];

/// Returns the intersection of all [constraints].
VersionConstraint _intersection(Iterable<VersionConstraint> constraints) {
var constraint = VersionConstraint.any;
Expand All @@ -222,6 +220,112 @@ final class PublishConstraintsChecker extends ConstraintsChecker {
return constraint;
}

/// Checks the [constraint] on [dependency] in [package] given its
/// [dependencyType].
void _checkConstraint(
PackageInfo package,
PackageInfo dependency,
DependencyType dependencyType,
Dependency constraint,
_DependencyConstraintMap allConstraints,
) {
switch ((
packageIsPublished: package.isPublishable,
dependencyIsPublished: dependency.isPublishable,
type: dependencyType,
)) {
/// Publishable packages listed in the `dependencies` block must have a hosted constraint.
case (
dependencyIsPublished: true,
packageIsPublished: _,
type: DependencyType.dependency,
) ||

/// Publishable packages must list other publishable packages with a hosted constraint.
(
dependencyIsPublished: true,
packageIsPublished: true,
type: _,
):
switch (constraint) {
case HostedDependency(:final version):
allConstraints.recordConstraint(
dependency: dependency,
package: package,
constraint: version,
);
default:
_processConstraint(
package: package,
dependencyPath: [dependencyType.key, dependency.name],
expectedConstraint: HostedDependency(
version: package.isPublishable
? dependency.currentConstraint
: VersionConstraint.any,
),
errorMessage:
'Invalid constraint type: ${constraint.runtimeType}. '
'A hosted dependency is required when listing any publishable '
'package in the `dependencies` block.',
);
}

/// Published packages must list unpublished packages with a path constraint.
case (
packageIsPublished: true,
dependencyIsPublished: false,
type:
DependencyType.devDependency || DependencyType.dependencyOverride,
):
switch (constraint) {
case PathDependency _:
return;
default:
_processConstraint(
package: package,
dependencyPath: [dependencyType.key, dependency.name],
expectedConstraint: PathDependency(
p.relative(dependency.path, from: package.path),
),
errorMessage: 'Invalid constraint on unpublished package. '
'A path dependency is required when listing any unpublished '
'package in the `dev_dependencies` block of a published package.',
);
}

/// A published package cannot take a dependency on an unpublished package
/// anywhere but the `dev_dependencies` block.
case (
packageIsPublished: true,
dependencyIsPublished: false,
type: DependencyType.dependency,
):
throw AssertionError(
'Non-publishable package (${dependency.name}) found in '
'the `dependencies` block of ${package.name}.',
);

/// Unpublished packages can depend on other unpublished packages in
/// any way they like without it affecting the pub validator.
case (
packageIsPublished: false,
dependencyIsPublished: false,
type: _,
):
return;

/// Unpublished packages' `dev_dependencies` and `dependency_overrides`
/// blocks are not checked by the pub validator.
case (
dependencyIsPublished: true,
packageIsPublished: false,
type:
DependencyType.devDependency || DependencyType.dependencyOverride,
):
return;
}
}

@override
bool checkConstraints(PackageInfo package) {
if (!package.isPublishable) {
Expand Down Expand Up @@ -249,56 +353,21 @@ final class PublishConstraintsChecker extends ConstraintsChecker {
};
for (final MapEntry(
key: dependencyName,
value: (dependencyType, dependency)
value: (dependencyType, constraint)
) in dependencies.entries) {
final repoDependency = repoGraph.keys.singleWhereOrNull(
final dependency = repoGraph.keys.singleWhereOrNull(
(pkg) => pkg.name == dependencyName,
);
if (repoDependency == null) {
if (dependency == null) {
continue;
}
switch (dependency) {
case HostedDependency(version: final constraint):
allConstraints.recordConstraint(
repoDependency: repoDependency,
inPackage: package,
constraint: constraint,
);

// Do not verify the constraints in the `dev_dependencies`
// block since this only affects the `pub` algorithm transitively.
case _ when dependencyType == DependencyType.devDependency:
return;

// Do not verify the constraint of non-publishable packages listed
// in the `dependencies` block of other non-publishable packages.
// We only care about publishable dependencies listed in the `dependencies`
// block of non-publishable packages.
case _ when !repoDependency.isPublishable:
if (package.isPublishable) {
throw AssertionError(
'Non-publishable package ($dependencyName) found in '
'the `dependencies` block of ${package.name}.',
);
}
return;

// Otherwise, we have a constraint which might cause an error. This
// is most often caused by path dependency on a publishable package
// in a test package's `dependencies` block.
case _:
_processConstraint(
package: package,
dependencyPath: [dependencyType.key, dependencyName],
expectedConstraint: package.isPublishable
? repoDependency.currentConstraint
: VersionConstraint.any,
errorMessage:
'Invalid constraint type: ${dependency.runtimeType}. '
'A hosted dependency is required when listing any publishable '
'package in the `dependencies` block.',
);
}
_checkConstraint(
package,
dependency,
dependencyType,
constraint,
allConstraints,
);
}
},
);
Expand All @@ -320,9 +389,11 @@ final class PublishConstraintsChecker extends ConstraintsChecker {
package.dependencyType(repoDependency)!.key,
repoDependency.name,
],
expectedConstraint: package.isPublishable
? repoDependency.currentConstraint
: VersionConstraint.any,
expectedConstraint: HostedDependency(
version: package.isPublishable
? repoDependency.currentConstraint
: VersionConstraint.any,
),
errorMessage:
'Constraint for dependency causes an empty intersection '
'for ${rootPackage.name}: $constraint',
Expand All @@ -339,13 +410,13 @@ final class _DependencyConstraintMap
with AWSDebuggable, AWSSerializable<Map<String, Object?>> {
_DependencyConstraintMap() : super({});

/// Records the [constraint] for [repoDependency] found in [inPackage].
/// Records the [constraint] for [dependency] found in [package].
void recordConstraint({
required PackageInfo repoDependency,
required PackageInfo inPackage,
required PackageInfo package,
required PackageInfo dependency,
required VersionConstraint constraint,
}) {
(this[repoDependency] ??= _ConstraintMap())[inPackage] = constraint;
(this[dependency] ??= _ConstraintMap())[package] = constraint;
}

@override
Expand Down Expand Up @@ -379,3 +450,19 @@ extension on PackageInfo {
max: version.nextMinor,
);
}

extension DependencyToYaml on Dependency {
YamlNode toYaml() => switch (this) {
HostedDependency(:final version) => YamlScalar.wrap(version.toString()),
PathDependency(:final path) => YamlMap.wrap({'path': path}),
SdkDependency(:final sdk) => YamlMap.wrap({'sdk': sdk}),
GitDependency(:final url, :final ref, :final path) => YamlMap.wrap({
'git': {
'url': url.toString(),
if (ref != null) 'ref': ref,
if (path != null) 'path': path,
},
}),
_ => throw StateError('Invalid dependency: $this'),
};
}
Loading
Loading