From f4001df1188e0da170f97c46f5051b8509b0ba15 Mon Sep 17 00:00:00 2001
From: Paul Berry <paulberry@google.com>
Date: Mon, 9 Nov 2020 13:16:34 +0000
Subject: [PATCH] Migration: include exports when checking that imports are
 migrated.

When I first implemented this I thought that checking exports wasn't
needed, since it's not allowed for a migrated library to re-export
symbols defined in an unmigtated library.  But it turns out we still
do need to include exports in the check, since there might be migrated
libraries that are only reachable in the transitive dependency graph
via an export, and we need to check that *those* libraries don't
import unmigrated code.

Bug: https://github.com/dart-lang/sdk/issues/44061
Change-Id: I280b4117373a3320fc1efe65abe24e344199754c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170840
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
---
 .../lib/src/nullability_migration_impl.dart   | 11 +++--
 .../test/migration_cli_test.dart              | 44 +++++++++++++++++++
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/pkg/nnbd_migration/lib/src/nullability_migration_impl.dart b/pkg/nnbd_migration/lib/src/nullability_migration_impl.dart
index f80f4c4028e..81a3079c508 100644
--- a/pkg/nnbd_migration/lib/src/nullability_migration_impl.dart
+++ b/pkg/nnbd_migration/lib/src/nullability_migration_impl.dart
@@ -217,7 +217,10 @@ class NullabilityMigrationImpl implements NullabilityMigration {
       return;
     }
     ExperimentStatusException.sanityCheck(result);
-    _recordTransitiveImportOptInStatus(result.libraryElement.importedLibraries);
+    _recordTransitiveImportExportOptInStatus(
+        result.libraryElement.importedLibraries);
+    _recordTransitiveImportExportOptInStatus(
+        result.libraryElement.exportedLibraries);
     if (_variables == null) {
       _variables = Variables(_graph, result.typeProvider, _getLineInfo,
           instrumentation: _instrumentation,
@@ -270,14 +273,16 @@ class NullabilityMigrationImpl implements NullabilityMigration {
   }
 
   /// Records the opt in/out status of all libraries in [libraries], and any
-  /// libraries they transitively import, in [_libraryOptInStatus].
-  void _recordTransitiveImportOptInStatus(Iterable<LibraryElement> libraries) {
+  /// libraries they transitively import or export, in [_libraryOptInStatus].
+  void _recordTransitiveImportExportOptInStatus(
+      Iterable<LibraryElement> libraries) {
     var librariesToCheck = libraries.toList();
     while (librariesToCheck.isNotEmpty) {
       var library = librariesToCheck.removeLast();
       if (_libraryOptInStatus.containsKey(library.source)) continue;
       _libraryOptInStatus[library.source] = library.isNonNullableByDefault;
       librariesToCheck.addAll(library.importedLibraries);
+      librariesToCheck.addAll(library.exportedLibraries);
     }
   }
 
diff --git a/pkg/nnbd_migration/test/migration_cli_test.dart b/pkg/nnbd_migration/test/migration_cli_test.dart
index c581b6b6b19..ce0ec972349 100644
--- a/pkg/nnbd_migration/test/migration_cli_test.dart
+++ b/pkg/nnbd_migration/test/migration_cli_test.dart
@@ -762,6 +762,50 @@ int? f() => null
     });
   }
 
+  test_lifecycle_import_check_via_export() async {
+    // If the user's code exports a library that imports a non-migrated library,
+    // that's a problem too.
+    var projectContents = simpleProject(
+        sourceText: "export 'package:foo/foo.dart';",
+        packageConfigText: _getPackageConfigText(
+            migrated: false, packagesMigrated: {'foo': true, 'bar': false}));
+    var projectDir = createProjectDir(projectContents);
+    resourceProvider.newFile(
+        packagePath('foo/lib/foo.dart'), "import 'package:bar/bar.dart';");
+    resourceProvider.newFile(packagePath('bar/lib/bar.dart'), '');
+    await assertRunFailure([projectDir], expectedExitCode: 1);
+    var output = logger.stdoutBuffer.toString();
+    expect(output, contains('Error: package has unmigrated dependencies'));
+    // Output should mention bar.dart, since it's unmigrated
+    expect(output, contains('package:bar/bar.dart'));
+    // But it should not mention foo.dart, which is migrated
+    expect(output, isNot(contains('package:foo/foo.dart')));
+  }
+
+  test_lifecycle_import_check_via_indirect_export() async {
+    // If the user's code imports a library that exports a library that imports
+    // a non-migrated library, that's a problem too.
+    var projectContents = simpleProject(
+        sourceText: "import 'package:foo/foo.dart';",
+        packageConfigText: _getPackageConfigText(
+            migrated: false,
+            packagesMigrated: {'foo': true, 'bar': true, 'baz': false}));
+    var projectDir = createProjectDir(projectContents);
+    resourceProvider.newFile(
+        packagePath('foo/lib/foo.dart'), "export 'package:bar/bar.dart';");
+    resourceProvider.newFile(
+        packagePath('bar/lib/bar.dart'), "import 'package:baz/baz.dart';");
+    resourceProvider.newFile(packagePath('baz/lib/baz.dart'), '');
+    await assertRunFailure([projectDir], expectedExitCode: 1);
+    var output = logger.stdoutBuffer.toString();
+    expect(output, contains('Error: package has unmigrated dependencies'));
+    // Output should mention baz.dart, since it's unmigrated
+    expect(output, contains('package:baz/baz.dart'));
+    // But it should not mention foo.dart or bar.dart, which are migrated
+    expect(output, isNot(contains('package:foo/foo.dart')));
+    expect(output, isNot(contains('package:bar/bar.dart')));
+  }
+
   test_lifecycle_no_preview() async {
     var projectContents = simpleProject();
     var projectDir = createProjectDir(projectContents);