[nnbd_migration] Print warning when SDK_PATH is set and things go wrong.

MichaelRFairhurst · commit-bot@chromium.org · commit 6b40b26044a6 · 2020-06-11T17:19:51.000Z
Also clean up sanity checking code a little bit. Change-Id: I4274dad42687a4dc419090d656ec56168c982c29 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150661 Commit-Queue: Mike Fairhurst <mfairhurst@google.com> Reviewed-by: Paul Berry <paulberry@google.com>
diff --git a/pkg/nnbd_migration/lib/migration_cli.dart b/pkg/nnbd_migration/lib/migration_cli.dart
@@ -25,9 +25,11 @@ import 'package:args/command_runner.dart';
 import 'package:cli_util/cli_logging.dart';
 import 'package:meta/meta.dart';
 import 'package:nnbd_migration/src/edit_plan.dart';
+import 'package:nnbd_migration/src/exceptions.dart';
 import 'package:nnbd_migration/src/front_end/dartfix_listener.dart';
 import 'package:nnbd_migration/src/front_end/driver_provider_impl.dart';
 import 'package:nnbd_migration/src/front_end/non_nullable_fix.dart';
+import 'package:nnbd_migration/src/messages.dart';
 import 'package:nnbd_migration/src/utilities/json.dart' as json;
 import 'package:nnbd_migration/src/utilities/source_edit_diff_formatter.dart';
 import 'package:path/path.dart' show Context;
@@ -236,6 +238,10 @@ class MigrationCli {
 
   final Map<String, LineInfo> lineInfo = {};
 
+  /// The environment variables, tracked to help users debug if SDK_PATH was
+  /// specified and that resulted in any [ExperimentStatusException]s.
+  final Map<String, String> _environmentVariables;
+
   DartFixListener _dartFixListener;
 
   _FixCodeProcessor _fixCodeProcessor;
@@ -246,15 +252,17 @@ class MigrationCli {
 
   bool _hasAnalysisErrors = false;
 
-  MigrationCli(
-      {@required this.binaryName,
-      @visibleForTesting this.loggerFactory = _defaultLoggerFactory,
-      @visibleForTesting this.defaultSdkPathOverride,
-      @visibleForTesting ResourceProvider resourceProvider,
-      @visibleForTesting this.processManager = const ProcessManager.system()})
-      : logger = loggerFactory(false),
+  MigrationCli({
+    @required this.binaryName,
+    @visibleForTesting this.loggerFactory = _defaultLoggerFactory,
+    @visibleForTesting this.defaultSdkPathOverride,
+    @visibleForTesting ResourceProvider resourceProvider,
+    @visibleForTesting this.processManager = const ProcessManager.system(),
+    @visibleForTesting Map<String, String> environmentVariables,
+  })  : logger = loggerFactory(false),
         resourceProvider =
-            resourceProvider ?? PhysicalResourceProvider.INSTANCE;
+            resourceProvider ?? PhysicalResourceProvider.INSTANCE,
+        _environmentVariables = environmentVariables ?? Platform.environment;
 
   @visibleForTesting
   DriverBasedAnalysisContext get analysisContext {
@@ -437,8 +445,12 @@ class MigrationCli {
         await _fixCodeProcessor.runFirstPhase();
         _fixCodeProcessor._progressBar.clear();
         _checkForErrors();
-      } on StateError catch (e) {
+      } on ExperimentStatusException catch (e) {
         logger.stdout(e.toString());
+        final sdkPathVar = _environmentVariables['SDK_PATH'];
+        if (sdkPathVar != null) {
+          logger.stdout('$sdkPathEnvironmentVariableSet: $sdkPathVar');
+        }
         exitCode = 1;
       }
       if (exitCode != null) return;
diff --git a/pkg/nnbd_migration/lib/src/exceptions.dart b/pkg/nnbd_migration/lib/src/exceptions.dart
@@ -0,0 +1,39 @@
+import 'package:analyzer/dart/analysis/features.dart';
+import 'package:analyzer/dart/analysis/results.dart';
+import 'package:nnbd_migration/src/messages.dart';
+
+/// A [StateError] specific to the ways that the NNBD experiment can be
+/// misconfigured which may prevent the tool from working.
+class ExperimentStatusException extends StateError {
+  /// A file included in the migration dir has already been migrated.
+  ExperimentStatusException.migratedAlready(String path)
+      : super('$migratedAlready: $path');
+
+  /// The SDK was analyzed without NNBD semantics.
+  ExperimentStatusException.sdkExperimentDisabled() : super(nnbdExperimentOff);
+
+  /// The SDK does not contain the NNBD sources, it is the pre-unfork copy.
+  ExperimentStatusException.sdkPreforkSources() : super(sdkNnbdOff);
+
+  /// Throw an [ExperimentStatusException] if the [result] seems to have
+  /// incorrectly configured experiment flags/nnbd sources.
+  static void sanityCheck(ResolvedUnitResult result) {
+    final equalsParamType = result.typeProvider.objectType
+        .getMethod('==')
+        .parameters[0]
+        .type
+        .getDisplayString(withNullability: true);
+    if (equalsParamType == 'Object*') {
+      throw ExperimentStatusException.sdkExperimentDisabled();
+    }
+
+    if (equalsParamType != 'Object') {
+      throw ExperimentStatusException.sdkPreforkSources();
+    }
+
+    if (result.unit.featureSet.isEnabled(Feature.non_nullable)) {
+      // TODO(mfairhurst): Allow for skipping already migrated compilation units.
+      throw ExperimentStatusException.migratedAlready(result.path);
+    }
+  }
+}
diff --git a/pkg/nnbd_migration/lib/src/messages.dart b/pkg/nnbd_migration/lib/src/messages.dart
@@ -7,3 +7,6 @@ const String migratedAlready =
 const String nnbdExperimentOff =
     'Analyzer seems to need the nnbd experiment on in the SDK.';
 const String sdkNnbdOff = 'Analysis seems to have an SDK without NNBD enabled.';
+const String sdkPathEnvironmentVariableSet =
+    r'Note: $SDK_PATH environment variable is set and may point to outdated '
+    'dart:core sources';
diff --git a/pkg/nnbd_migration/lib/src/nullability_migration_impl.dart b/pkg/nnbd_migration/lib/src/nullability_migration_impl.dart
@@ -17,7 +17,7 @@ import 'package:nnbd_migration/src/edge_builder.dart';
 import 'package:nnbd_migration/src/edit_plan.dart';
 import 'package:nnbd_migration/src/fix_aggregator.dart';
 import 'package:nnbd_migration/src/fix_builder.dart';
-import 'package:nnbd_migration/src/messages.dart';
+import 'package:nnbd_migration/src/exceptions.dart';
 import 'package:nnbd_migration/src/node_builder.dart';
 import 'package:nnbd_migration/src/nullability_node.dart';
 import 'package:nnbd_migration/src/postmortem_file.dart';
@@ -104,7 +104,7 @@ class NullabilityMigrationImpl implements NullabilityMigration {
 
   @override
   void finalizeInput(ResolvedUnitResult result) {
-    _sanityCheck(result);
+    ExperimentStatusException.sanityCheck(result);
     if (!_propagated) {
       _propagated = true;
       _graph.propagate(_postmortemFileWriter);
@@ -160,7 +160,7 @@ class NullabilityMigrationImpl implements NullabilityMigration {
   }
 
   void prepareInput(ResolvedUnitResult result) {
-    _sanityCheck(result);
+    ExperimentStatusException.sanityCheck(result);
     if (_variables == null) {
       _variables = Variables(_graph, result.typeProvider, _getLineInfo,
           instrumentation: _instrumentation,
@@ -184,7 +184,7 @@ class NullabilityMigrationImpl implements NullabilityMigration {
   }
 
   void processInput(ResolvedUnitResult result) {
-    _sanityCheck(result);
+    ExperimentStatusException.sanityCheck(result);
     var unit = result.unit;
     try {
       DecoratedTypeParameterBounds.current = _decoratedTypeParameterBounds;
@@ -207,26 +207,6 @@ class NullabilityMigrationImpl implements NullabilityMigration {
     _graph.update(_postmortemFileWriter);
   }
 
-  void _sanityCheck(ResolvedUnitResult result) {
-    final equalsParamType = result.typeProvider.objectType
-        .getMethod('==')
-        .parameters[0]
-        .type
-        .getDisplayString(withNullability: true);
-    if (equalsParamType == 'Object*') {
-      throw StateError(nnbdExperimentOff);
-    }
-
-    if (equalsParamType != 'Object') {
-      throw StateError(sdkNnbdOff);
-    }
-
-    if (result.unit.featureSet.isEnabled(Feature.non_nullable)) {
-      // TODO(jcollins-g): Allow for skipping already migrated compilation units.
-      throw StateError('$migratedAlready: ${result.path}');
-    }
-  }
-
   static Location _computeLocation(
       LineInfo lineInfo, SourceEdit edit, Source source) {
     final locationInfo = lineInfo.getLocation(edit.offset);
diff --git a/pkg/nnbd_migration/test/migration_cli_test.dart b/pkg/nnbd_migration/test/migration_cli_test.dart
@@ -25,6 +25,7 @@ import 'package:nnbd_migration/src/front_end/non_nullable_fix.dart';
 import 'package:nnbd_migration/src/front_end/web/edit_details.dart';
 import 'package:nnbd_migration/src/front_end/web/file_details.dart';
 import 'package:nnbd_migration/src/front_end/web/navigation_tree.dart';
+import 'package:nnbd_migration/src/messages.dart' as messages;
 import 'package:path/path.dart' as path;
 import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
@@ -81,14 +82,16 @@ class _MigrationCli extends MigrationCli {
   Future<void> Function() _runWhilePreviewServerActive;
 
   _MigrationCli(_MigrationCliTestBase test,
-      {this.injectArtificialException = false})
+      {this.injectArtificialException = false,
+      Map<String, String> environmentVariables})
       : super(
             binaryName: 'nnbd_migration',
             loggerFactory: (isVerbose) => test.logger = _TestLogger(isVerbose),
             defaultSdkPathOverride:
                 test.resourceProvider.convertPath(mock_sdk.sdkRoot),
             resourceProvider: test.resourceProvider,
-            processManager: test.processManager);
+            processManager: test.processManager,
+            environmentVariables: environmentVariables);
 
   @override
   Future<void> blockUntilSignalInterrupt() async {
@@ -141,6 +144,8 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
   @override
   /*late*/ _TestLogger logger;
 
+  Map<String, String> environmentVariables = {};
+
   final hasVerboseHelpMessage = contains('for verbose help output');
 
   final hasUsageText = contains('Usage: nnbd_migration');
@@ -281,6 +286,7 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
 
   void setUp() {
     resourceProvider.newFolder(resourceProvider.pathContext.current);
+    environmentVariables.clear();
   }
 
   Map<String, String> simpleProject(
@@ -343,10 +349,30 @@ int${migrated ? '?' : ''} f() => null;
     await cli.run(MigrationCli.createParser().parse([projectDir]));
     assertErrorExit(cli, withUsage: false);
     var output = logger.stdoutBuffer.toString();
-    expect(
-        output,
-        contains(
-            'Bad state: Analysis seems to have an SDK without NNBD enabled'));
+    expect(output, contains(messages.sdkNnbdOff));
+  }
+
+  test_detect_old_sdk_environment_variable() async {
+    environmentVariables['SDK_PATH'] = '/fake-old-sdk-path';
+    var cli = _createCli();
+    // Alter the mock SDK, changing the signature of Object.operator== to match
+    // the signature that was present prior to NNBD.  (This is what the
+    // migration tool uses to detect an old SDK).
+    var coreLib = resourceProvider.getFile(
+        resourceProvider.convertPath('${mock_sdk.sdkRoot}/lib/core/core.dart'));
+    var oldCoreLibText = coreLib.readAsStringSync();
+    var newCoreLibText = oldCoreLibText.replaceAll(
+        'external bool operator ==(Object other)',
+        'external bool operator ==(dynamic other)');
+    expect(newCoreLibText, isNot(oldCoreLibText));
+    coreLib.writeAsStringSync(newCoreLibText);
+    var projectDir = await createProjectDir(simpleProject());
+    await cli.run(MigrationCli.createParser().parse([projectDir]));
+    assertErrorExit(cli, withUsage: false);
+    var output = logger.stdoutBuffer.toString();
+    expect(output, contains(messages.sdkNnbdOff));
+    expect(output, contains(messages.sdkPathEnvironmentVariableSet));
+    expect(output, contains(environmentVariables['SDK_PATH']));
   }
 
   test_flag_apply_changes_default() {
@@ -1547,7 +1573,8 @@ name: test
   _MigrationCli _createCli({bool injectArtificialException = false}) {
     mock_sdk.MockSdk(resourceProvider: resourceProvider);
     return _MigrationCli(this,
-        injectArtificialException: injectArtificialException);
+        injectArtificialException: injectArtificialException,
+        environmentVariables: environmentVariables);
   }
 
   Future<String> _getHelpText({@required bool verbose}) async {
