Fix dependency_overrides validator in workspaces (#4564)

Author: sigurdm

lib/src/package_graph.dart

@@ -37,15 +37,14 @@ class PackageGraph {
     SolveResult result,
   ) {
     final packages = {
-      for (final id in result.packages)
-        id.name:
-            id.isRoot
-                ? entrypoint.workspaceRoot
-                : Package(
-                  result.pubspecs[id.name]!,
-                  entrypoint.cache.getDirectory(id),
-                  [],
-                ),
+      for (final package in entrypoint.workspaceRoot.transitiveWorkspace)
+        package.name: package,
+      for (final id in result.packages.where((p) => !p.isRoot))
+        id.name: Package(
+          result.pubspecs[id.name]!,
+          entrypoint.cache.getDirectory(id),
+          [],
+        ),
     };
 
     return PackageGraph(entrypoint, packages);

lib/src/validator/dependency_override.dart

@@ -4,26 +4,33 @@
 
 import 'dart:async';
 
-import 'package:collection/collection.dart';
-
 import '../validator.dart';
 
-/// A validator that validates a package's dependencies overrides (or the
-/// absence thereof).
+/// Complains (with a hint) if any of the transitive dependencies of a package's
+/// non-dev dependencies are overridden anywhere in the workspace.
 class DependencyOverrideValidator extends Validator {
   @override
   Future<void> validate() async {
-    final overridden = MapKeySet(
-      context.entrypoint.workspaceRoot.allOverridesInWorkspace,
-    );
-    final dev = MapKeySet(package.devDependencies);
-    if (overridden.difference(dev).isNotEmpty) {
-      final overridesFile =
-          package.pubspec.dependencyOverridesFromOverridesFile
-              ? package.pubspecOverridesPath
-              : package.pubspecPath;
+    final graph = await context.entrypoint.packageGraph;
+    final transitiveNonDevDependencies = <String>{};
+    final toVisit = [package.name];
+    while (toVisit.isNotEmpty) {
+      final next = toVisit.removeLast();
+      if (transitiveNonDevDependencies.add(next)) {
+        toVisit.addAll(graph.packages[next]!.dependencies.keys);
+      }
+    }
 
-      hints.add('''
+    for (final workspacePackage
+        in context.entrypoint.workspaceRoot.transitiveWorkspace) {
+      for (final override
+          in workspacePackage.pubspec.dependencyOverrides.keys) {
+        if (transitiveNonDevDependencies.contains(override)) {
+          final overridesFile =
+              workspacePackage.pubspec.dependencyOverridesFromOverridesFile
+                  ? workspacePackage.pubspecOverridesPath
+                  : workspacePackage.pubspecPath;
+          hints.add('''
 Non-dev dependencies are overridden in $overridesFile.
 
 This indicates you are not testing your package against the same versions of its
@@ -32,6 +39,8 @@ dependencies that users will have when they use it.
 This might be necessary for packages with cyclic dependencies.
 
 Please be extra careful when publishing.''');
+        }
+      }
     }
   }
 }

pubspec.lock

@@ -5,23 +5,18 @@ packages:
     dependency: transitive
     description:
       name: _fe_analyzer_shared
-      sha256: "88399e291da5f7e889359681a8f64b18c5123e03576b01f32a6a276611e511c3"
+      sha256: dc27559385e905ad30838356c5f5d574014ba39872d732111cd07ac0beff4c57
       url: "https://pub.dev"
     source: hosted
-    version: "78.0.0"
-  _macros:
-    dependency: transitive
-    description: dart
-    source: sdk
-    version: "0.3.3"
+    version: "80.0.0"
   analyzer:
     dependency: "direct main"
     description:
       name: analyzer
-      sha256: "62899ef43d0b962b056ed2ebac6b47ec76ffd003d5f7c4e4dc870afe63188e33"
+      sha256: "192d1c5b944e7e53b24b5586db760db934b177d4147c42fbca8c8c5f1eb8d11e"
       url: "https://pub.dev"
     source: hosted
-    version: "7.1.0"
+    version: "7.3.0"
   args:
     dependency: "direct main"
     description:
@@ -190,14 +185,6 @@ packages:
       url: "https://pub.dev"
     source: hosted
     version: "1.3.0"
-  macros:
-    dependency: transitive
-    description:
-      name: macros
-      sha256: "1d9e801cd66f7ea3663c45fc708450db1fa57f988142c64289142c9b7ee80656"
-      url: "https://pub.dev"
-    source: hosted
-    version: "0.1.3-main.0"
   matcher:
     dependency: transitive
     description:

pubspec.yaml

@@ -4,7 +4,7 @@ environment:
   sdk: ^3.7.0
 
 dependencies:
-  analyzer: 7.1.0
+  analyzer: 7.3.0
   args: ^2.7.0
   async: ^2.11.0
   cli_util: ^0.4.1

test/validator/dependency_override_test.dart

@@ -2,14 +2,15 @@
 // 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 'package:path/path.dart' as p;
 import 'package:test/test.dart';
 
 import '../descriptor.dart' as d;
 import '../test_pub.dart';
 import 'utils.dart';
 
 void main() {
-  test('should consider a package valid if it has dev dependency '
+  test('should consider a package valid if it overrides dev dependency '
       'overrides', () async {
     final server = await servePackages();
     server.serve('foo', '3.0.0');
@@ -27,8 +28,28 @@ void main() {
     await expectValidation();
   });
 
-  group('should consider a package invalid if', () {
-    test('it has only non-dev dependency overrides', () async {
+  test('should consider a package valid '
+      'if it has any dependency overrides on non-dependency', () async {
+    final server = await servePackages();
+    server.serve('foo', '3.0.0');
+    server.serve('bar', '3.0.0');
+
+    await d.validPackage().create();
+    await d.dir(appPath, [
+      d.validPubspec(
+        extras: {
+          'dev_dependencies': {'foo': '^1.0.0'},
+          'dependency_overrides': {'foo': '^3.0.0', 'bar': '^3.0.0'},
+        },
+      ),
+    ]).create();
+
+    await expectValidation();
+  });
+
+  test(
+    'should consider a package invalid if it has override of direct dependency',
+    () async {
       final server = await servePackages();
       server.serve('foo', '3.0.0');
       await d.validPackage().create();
@@ -45,46 +66,84 @@ void main() {
       await expectValidationHint(
         'Non-dev dependencies are overridden in pubspec.yaml.',
       );
-    });
-    test('it has a pubspec_overrides.yaml', () async {
-      final server = await servePackages();
-      server.serve('foo', '3.0.0');
-      await d.validPackage().create();
+    },
+  );
 
-      await d.dir(appPath, [
-        d.validPubspec(
-          extras: {
-            'dependencies': {'foo': '^1.0.0'},
-          },
-        ),
-        d.pubspecOverrides({
-          'dependency_overrides': {'foo': '3.0.0'},
-        }),
-      ]).create();
+  test('should consider a package invalid if it '
+      'has override of transitive dependency', () async {
+    final server = await servePackages();
+    server.serve('foo', '1.0.0', deps: {'bar': '^3.0.0'});
+    server.serve('bar', '3.0.0');
 
-      await expectValidationHint(
-        'Non-dev dependencies are overridden in pubspec_overrides.yaml.',
-      );
-    });
+    await d.validPackage().create();
 
-    test('it has any non-dev dependency overrides', () async {
-      final server = await servePackages();
-      server.serve('foo', '3.0.0');
-      server.serve('bar', '3.0.0');
+    await d.dir(appPath, [
+      d.validPubspec(
+        extras: {
+          'dependencies': {'foo': '^1.0.0'},
+          'dependency_overrides': {'bar': '^3.0.0'},
+        },
+      ),
+    ]).create();
 
-      await d.validPackage().create();
-      await d.dir(appPath, [
+    await expectValidationHint(
+      'Non-dev dependencies are overridden in pubspec.yaml.',
+    );
+  });
+
+  test('reports correctly about a pubspec_overrides.yaml', () async {
+    final server = await servePackages();
+    server.serve('foo', '3.0.0');
+    await d.validPackage().create();
+
+    await d.dir(appPath, [
+      d.validPubspec(
+        extras: {
+          'dependencies': {'foo': '^1.0.0'},
+        },
+      ),
+      d.pubspecOverrides({
+        'dependency_overrides': {'foo': '3.0.0'},
+      }),
+    ]).create();
+
+    await expectValidationHint(
+      'Non-dev dependencies are overridden in pubspec_overrides.yaml.',
+    );
+  });
+
+  test('Detects overrides from outside work-package', () async {
+    final server = await servePackages();
+    server.serve('foo', '3.0.0');
+    await d.validPackage().create();
+
+    await d.dir(appPath, [
+      d.libPubspec(
+        'workspace',
+        '1.2.3',
+        extras: {
+          'workspace': ['a'],
+          'dependency_overrides': {'foo': '^3.0.0'},
+        },
+        sdk: '^3.5.0',
+      ),
+      d.dir('a', [
+        ...d.validPackage().contents,
         d.validPubspec(
           extras: {
-            'dev_dependencies': {'foo': '^1.0.0'},
-            'dependency_overrides': {'foo': '^3.0.0', 'bar': '^3.0.0'},
+            'environment': {'sdk': '^3.5.0'},
+            'resolution': 'workspace',
+            'dependencies': {'foo': '^1.0.0'},
           },
         ),
-      ]).create();
+      ]),
+    ]).create();
 
-      await expectValidationHint(
-        'Non-dev dependencies are overridden in pubspec.yaml.',
-      );
-    });
+    final s = p.separator;
+    await expectValidationHint(
+      'Non-dev dependencies are overridden in ..${s}pubspec.yaml.',
+      workingDirectory: p.join(d.sandbox, appPath, 'a'),
+      environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'},
+    );
   });
 }

test/validator/utils.dart

@@ -58,11 +58,13 @@ Future<void> expectValidationHint(
   String hint, {
   int count = 1,
   Map<String, String> environment = const {},
+  String? workingDirectory,
 }) async {
   final s = count == 1 ? '' : 's';
   await expectValidation(
     message: allOf([contains(hint), contains('and $count hint$s')]),
     environment: environment,
+    workingDirectory: workingDirectory,
   );
 }