[dart fix] Support fixing pubspecs in a monorepo - multiple packages …

…in one workspace.

Change-Id: Idd1c74d8e107fc1a770878161877574d29e21a99
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362442
Commit-Queue: Keerti Parthasarathy <keertip@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

pkg/analysis_server/lib/src/services/correction/bulk_fix_processor.dart

@@ -42,8 +42,7 @@ import 'package:analyzer/src/util/performance/operation_performance.dart';
 import 'package:analyzer/src/utilities/cancellation.dart';
 import 'package:analyzer/src/utilities/extensions/analysis_session.dart';
 import 'package:analyzer/src/utilities/extensions/string.dart';
-import 'package:analyzer/src/workspace/blaze.dart';
-import 'package:analyzer/src/workspace/gn.dart';
+import 'package:analyzer/src/workspace/pub.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart'
     show SourceFileEdit;
 import 'package:analyzer_plugin/src/utilities/change_builder/change_builder_core.dart';
@@ -355,37 +354,40 @@ class BulkFixProcessor {
     var details = <BulkFix>[];
     for (var context in contexts) {
       var workspace = context.contextRoot.workspace;
-      if (workspace is GnWorkspace || workspace is BlazeWorkspace) {
+      if (workspace is! PackageConfigWorkspace) {
         continue;
       }
-      // Find the pubspec file
-      var rootFolder = context.contextRoot.root;
-      var pubspecFile = rootFolder.getChild('pubspec.yaml') as File;
-      if (!pubspecFile.exists) {
-        continue;
-      }
-      var packages = <String>{};
-      var devPackages = <String>{};
-
       var pathContext = context.contextRoot.resourceProvider.pathContext;
-      final libPath = rootFolder.getChild('lib').path;
-      final binPath = rootFolder.getChild('bin').path;
-
-      bool isPublic(String path) {
-        if (path.startsWith(libPath) || path.startsWith(binPath)) {
-          return true;
-        }
-        return false;
-      }
+      var resourceProvider = workspace.provider;
+      var packageToDeps = <PubPackage, _PubspecDeps>{};
 
       for (var path in context.contextRoot.analyzedFiles()) {
         if (!file_paths.isDart(pathContext, path) ||
             file_paths.isGenerated(path) ||
             file_paths.isMacroGenerated(path)) {
           continue;
         }
-        // Get the list of imports used in the files.
+        var package = workspace.findPackageFor(path);
+        if (package is! PubPackage) {
+          continue;
+        }
 
+        final libPath =
+            resourceProvider.getFolder(package.root).getChild('lib').path;
+        final binPath =
+            resourceProvider.getFolder(package.root).getChild('bin').path;
+
+        bool isPublic(String path, PubPackage package) {
+          if (path.startsWith(libPath) || path.startsWith(binPath)) {
+            return true;
+          }
+          return false;
+        }
+
+        var pubspecDeps =
+            packageToDeps.putIfAbsent(package, () => _PubspecDeps());
+
+        // Get the list of imports used in the files.
         var result = context.currentSession.getParsedLibrary(path);
         if (result is! ParsedLibraryResult) {
           return PubspecFixRequestResult(fixes, details);
@@ -398,28 +400,32 @@ class BulkFixProcessor {
                 (directive is ImportDirective) ? directive.uri.stringValue : '';
             if (uri!.startsWith('package:')) {
               final name = Uri.parse(uri).pathSegments.first;
-              if (isPublic(path)) {
-                packages.add(name);
+              if (isPublic(path, package)) {
+                pubspecDeps.packages.add(name);
               } else {
-                devPackages.add(name);
+                pubspecDeps.devPackages.add(name);
               }
             }
           }
         }
       }
 
-      // Compute changes to pubspec.
-      var result = await _runPubspecValidatorAndFixGenerator(
-          FileSource(pubspecFile),
-          packages,
-          devPackages,
-          context.contextRoot.resourceProvider);
-      if (result.isNotEmpty) {
-        for (var fix in result) {
-          fixes.addAll(fix.change.edits);
+      // Iterate over packages in the workspace, compute changes to pubspec.
+      for (var package in packageToDeps.keys) {
+        var pubspecDeps = packageToDeps[package]!;
+        var pubspecFile = package.pubspecFile;
+        var result = await _runPubspecValidatorAndFixGenerator(
+            FileSource(pubspecFile),
+            pubspecDeps.packages,
+            pubspecDeps.devPackages,
+            context.contextRoot.resourceProvider);
+        if (result.isNotEmpty) {
+          for (var fix in result) {
+            fixes.addAll(fix.change.edits);
+          }
+          details.add(BulkFix(pubspecFile.path,
+              [BulkFixDetail(PubspecWarningCode.MISSING_DEPENDENCY.name, 1)]));
         }
-        details.add(BulkFix(pubspecFile.path,
-            [BulkFixDetail(PubspecWarningCode.MISSING_DEPENDENCY.name, 1)]));
       }
     }
     return PubspecFixRequestResult(fixes, details);
@@ -1064,6 +1070,11 @@ class PubspecFixRequestResult {
   PubspecFixRequestResult(this.edits, this.details);
 }
 
+class _PubspecDeps {
+  final Set<String> packages = <String>{};
+  final Set<String> devPackages = <String>{};
+}
+
 extension on int {
   String get isAre => this == 1 ? 'is' : 'are';
 }

pkg/analysis_server/test/src/services/correction/fix/bulk_fix_processor_test.dart

@@ -149,6 +149,53 @@ void bad() {
 
 @reflectiveTest
 class PubspecFixTest extends BulkFixProcessorTest {
+  @FailingTest(
+      reason: 'TODO(keertip: Fix issue with deletes where the entry to'
+          'be removed is in between entries.')
+  Future<void> test_delete_change() async {
+    var content = '''
+name: test
+dependencies:
+  a: any
+dev_dependencies:
+  b: any
+  c: any
+  d: any
+''';
+    var expected = '''
+name: test
+dependencies:
+  a: any
+  c: any
+dev_dependencies:
+  b: any
+  d: any
+''';
+    updateTestPubspecFile(content);
+
+    newFile('$testPackageLibPath/lib.dart', '''
+import 'package:c/c.dart';
+
+void bad() {
+  try {
+  } on Error catch (e) {
+    print(e);
+  }
+}
+''');
+    var testFile = newFile('$testPackageTestPath/test.dart', '''
+import 'package:b/b.dart';
+import 'package:c/c.dart';
+import 'package:d/d.dart';
+import 'package:test/lib.dart';
+void f() {
+  print(C());
+}
+''');
+    await getResolvedUnit(testFile);
+    await assertFixPubspec(content, expected);
+  }
+
   Future<void> test_fix() async {
     var content = '''
 name: test
@@ -203,6 +250,69 @@ void bad() {
     await assertFixPubspec(content, expected);
   }
 
+  Future<void> test_multiple_pubspec_change() async {
+    var content = '''
+name: test
+dependencies:
+  a: any
+dev_dependencies:
+  b: any
+  d: any
+  c: any
+''';
+    var expected = '''
+name: test
+dependencies:
+  a: any
+  c: any
+  test2: any
+dev_dependencies:
+  b: any
+  d: any
+''';
+    updateTestPubspecFile(content);
+
+    newFile('$testPackageLibPath/lib.dart', '''
+import 'package:c/c.dart';
+import 'package:test2/lib.dart';
+import 'package:flutter_gen/gen.dart';
+
+void bad() {
+  try {
+  } on Error catch (e) {
+    print(e);
+  }
+}
+''');
+    var testFile = newFile('$testPackageTestPath/test.dart', '''
+import 'package:b/b.dart';
+import 'package:c/c.dart';
+import 'package:d/d.dart';
+import 'package:test/lib.dart';
+void f() {
+  print(C());
+}
+''');
+
+    newFile('$workspaceRootPath/test2/lib.dart', '''
+import 'package:d/d.dart';
+import 'package:flutter_gen/gen.dart';
+
+class A{}
+''');
+    var test2PubspecContent = '''
+name: test2
+deps:
+  d: any
+''';
+    var test2Pubspec =
+        newFile('$workspaceRootPath/test2/pubspec.yaml', test2PubspecContent);
+    await getResolvedUnit(testFile);
+    await assertFixPubspec(content, expected);
+    await assertFixPubspec(test2PubspecContent, test2PubspecContent,
+        file: test2Pubspec);
+  }
+
   Future<void> test_no_fix() async {
     var content = '''
 name: test

pkg/analysis_server/test/src/services/correction/fix/fix_processor.dart

@@ -7,6 +7,7 @@ import 'package:analysis_server/src/services/correction/change_workspace.dart';
 import 'package:analysis_server/src/services/correction/fix.dart';
 import 'package:analysis_server/src/services/correction/fix_internal.dart';
 import 'package:analyzer/error/error.dart';
+import 'package:analyzer/file_system/file_system.dart';
 import 'package:analyzer/src/dart/analysis/byte_store.dart';
 import 'package:analyzer/src/dart/error/lint_codes.dart';
 import 'package:analyzer/src/services/available_declarations.dart';
@@ -99,9 +100,10 @@ abstract class BulkFixProcessorTest extends AbstractSingleUnitTest {
   }
 
   /// Computes fixes for the pubspecs in the given contexts.
-  Future<void> assertFixPubspec(String original, String expected) async {
+  Future<void> assertFixPubspec(String original, String expected,
+      {File? file}) async {
     var tracker = DeclarationsTracker(MemoryByteStore(), resourceProvider);
-    var analysisContext = contextFor(testFile);
+    var analysisContext = contextFor(file ?? testFile);
     tracker.addContext(analysisContext);
     var processor =
         BulkFixProcessor(TestInstrumentationService(), await workspace);

pkg/analyzer/lib/src/pubspec/validators/missing_dependency_validator.dart

@@ -38,6 +38,12 @@ class MissingDependencyValidator {
   /// The listener to record the errors.
   final RecordingErrorListener recorder;
 
+  /// A set of names of special packages that should not be added as
+  /// dependencies in the `pubspec.yaml` file. For example, the flutter_gen
+  /// codegen package is specified in a special `flutter` section of the
+  /// `pubspec.yaml` file and not as part of the `dependencies` section.
+  final Set noDepsPackages = <String>{'flutter_gen'};
+
   MissingDependencyValidator(this.contents, this.source, this.provider)
       : recorder = RecordingErrorListener() {
     reporter = ErrorReporter(recorder, source);
@@ -79,6 +85,10 @@ class MissingDependencyValidator {
     // Ensure that the package itself is not listed as a dependency.
     usedDeps.remove(packageName);
     usedDevDeps.remove(packageName);
+    for (var package in noDepsPackages) {
+      usedDeps.remove(package);
+      usedDevDeps.remove(package);
+    }
 
     var availableDeps = [
       if (dependencies.isNotEmpty)

pkg/analyzer/lib/src/workspace/pub.dart

@@ -393,9 +393,11 @@ class PubPackage extends WorkspacePackage {
 
   final String? _name;
 
-  final String? _pubspecContent;
+  final String? pubspecContent;
 
-  final Pubspec? _pubspec;
+  final Pubspec? pubspec;
+
+  final File pubspecFile;
 
   VersionConstraint? _sdkVersionConstraint;
 
@@ -410,23 +412,20 @@ class PubPackage extends WorkspacePackage {
     var pubspecContent = pubspecFile.readAsStringSync();
     var pubspec = Pubspec.parse(pubspecContent);
     var packageName = pubspec.name?.value.text;
-    return PubPackage._(root, workspace, pubspecContent, pubspec, packageName);
+    return PubPackage._(
+        root, workspace, pubspecContent, pubspecFile, pubspec, packageName);
   }
 
-  PubPackage._(this.root, this.workspace, this._pubspecContent, this._pubspec,
-      this._name);
-
-  Pubspec? get pubspec => _pubspec;
-
-  String? get pubspecContent => _pubspecContent;
+  PubPackage._(this.root, this.workspace, this.pubspecContent, this.pubspecFile,
+      this.pubspec, this._name);
 
   /// The version range for the SDK specified for this package , or `null` if
   /// it is ill-formatted or not set.
   VersionConstraint? get sdkVersionConstraint {
     if (!_parsedSdkConstraint) {
       _parsedSdkConstraint = true;
 
-      var sdkValue = _pubspec?.environment?.sdk?.value.text;
+      var sdkValue = pubspec?.environment?.sdk?.value.text;
       if (sdkValue != null) {
         try {
           _sdkVersionConstraint = VersionConstraint.parse(sdkValue);