Migrator: improve pubspec parsing

* Prevent cast exception crash when pubspec.yaml is not a map
* Prevent cast exception crash when pubspec sdk is not a string
* Test error cases for parsing pubspec.

Change-Id: Id392815207dc52719cfec5038d6fa2d09f952f08
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/149602
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>

Author: srawlins

pkg/nnbd_migration/lib/src/front_end/non_nullable_fix.dart

@@ -122,7 +122,7 @@ class NonNullableFix {
 
   /// Update the pubspec.yaml file to specify a minimum Dart SDK version which
   /// enables the Null Safety feature.
-  Future<void> processPackage(Folder pkgFolder) async {
+  void processPackage(Folder pkgFolder) {
     if (!_packageIsNNBD) {
       return;
     }
@@ -137,14 +137,17 @@ class NonNullableFix {
 
     try {
       pubspecContent = pubspecFile.readAsStringSync();
+      var pubspecYaml = loadYaml(pubspecContent);
+      if (pubspecYaml is YamlMap) {
+        pubspecMap = pubspecYaml;
+      } else {
+        throw FormatException('pubspec.yaml file is not a YAML map');
+      }
     } on FileSystemException catch (e) {
-      processYamlException('read', pubspecFile.path, e);
+      _processPubspecException('read', pubspecFile.path, e);
       return;
-    }
-    try {
-      pubspecMap = loadYaml(pubspecContent) as YamlNode;
-    } on YamlException catch (e) {
-      processYamlException('parse', pubspecFile.path, e);
+    } on FormatException catch (e) {
+      _processPubspecException('parse', pubspecFile.path, e);
       return;
     }
 
@@ -202,17 +205,25 @@ environment:
   sdk: '$_intendedSdkVersionConstraint'""";
         insertAfterParent(environmentOptions.span, content);
       } else if (sdk is YamlScalar) {
-        var currentConstraint = VersionConstraint.parse(sdk.value as String);
-        var minimumVersion = Version.parse(_intendedMinimumSdkVersion);
-        if (currentConstraint is VersionRange &&
-            currentConstraint.min >= minimumVersion) {
-          // The current SDK version constraint already enables Null Safety.
-          return;
+        VersionConstraint currentConstraint;
+        if (sdk.value is String) {
+          currentConstraint = VersionConstraint.parse(sdk.value as String);
+          var minimumVersion = Version.parse(_intendedMinimumSdkVersion);
+          if (currentConstraint is VersionRange &&
+              currentConstraint.min >= minimumVersion) {
+            // The current SDK version constraint already enables Null Safety.
+            // Do not edit pubspec.yaml.
+            return;
+          } else {
+            // TODO(srawlins): This overwrites the current maximum version. In
+            // the uncommon situation that the maximum is not '<3.0.0', it
+            // should not.
+            replaceSpan(sdk.span, "'$_intendedSdkVersionConstraint'");
+          }
         } else {
-          // TODO(srawlins): This overwrites the current maximum version. In
-          // the uncommon situation that the maximum is not '<3.0.0', it should
-          // not.
-          replaceSpan(sdk.span, "'$_intendedSdkVersionConstraint'");
+          // Something is odd with the SDK constraint we've found in
+          // pubspec.yaml; Best to leave it alone.
+          return;
         }
       }
     }
@@ -237,20 +248,6 @@ environment:
     }
   }
 
-  void processYamlException(String action, String optionsFilePath, error) {
-    listener.addRecommendation('''Failed to $action options file
-  $optionsFilePath
-  $error
-
-  Manually update this file to enable the Null Safety language feature by
-  adding:
-
-    environment:
-      sdk: '$_intendedSdkVersionConstraint';
-''');
-    _packageIsNNBD = false;
-  }
-
   Future<MigrationState> rerun() async {
     reset();
     await rerunFunction();
@@ -275,6 +272,20 @@ environment:
     _server?.close();
   }
 
+  void _processPubspecException(String action, String pubspecPath, error) {
+    listener.addRecommendation('''Failed to $action pubspec file
+  $pubspecPath
+  $error
+
+  Manually update this file to enable the Null Safety language feature by
+  adding:
+
+    environment:
+      sdk: '$_intendedSdkVersionConstraint';
+''');
+    _packageIsNNBD = false;
+  }
+
   /// Allows unit tests to shut down any rogue servers that have been started,
   /// so that unit testing can complete.
   @visibleForTesting

pkg/nnbd_migration/test/migration_cli_test.dart

@@ -208,9 +208,11 @@ mixin _MigrationCliTestMethods on _MigrationCliTestBase {
     resourceProvider.newFolder(resourceProvider.pathContext.current);
   }
 
-  Map<String, String> simpleProject({bool migrated: false, String sourceText}) {
+  Map<String, String> simpleProject(
+      {bool migrated: false, String sourceText, String pubspecText}) {
     return {
-      'pubspec.yaml': '''
+      'pubspec.yaml': pubspecText ??
+          '''
 name: test
 environment:
   sdk: '${migrated ? '>=2.9.0 <2.10.0' : '>=2.6.0 <3.0.0'}'
@@ -1091,6 +1093,97 @@ int f() => null;
     expect(logger.stderrBuffer.toString(), startsWith('Warning:'));
   }
 
+  test_pubspec_does_not_exist() async {
+    var projectContents = simpleProject()..remove('pubspec.yaml');
+    var projectDir = await createProjectDir(projectContents);
+    var cli = _createCli();
+    await cli
+        .run(_parseArgs(['--no-web-preview', '--apply-changes', projectDir]));
+    // The Dart source code should still be migrated.
+    assertProjectContents(
+        projectDir, simpleProject(migrated: true)..remove('pubspec.yaml'));
+  }
+
+  test_pubspec_environment_is_missing_sdk() async {
+    var projectContents = simpleProject(pubspecText: '''
+name: test
+environment:
+  foo: 1
+''');
+    var projectDir = await createProjectDir(projectContents);
+    var cli = _createCli();
+    await cli
+        .run(_parseArgs(['--no-web-preview', '--apply-changes', projectDir]));
+    // The Dart source code should still be migrated.
+    assertProjectContents(
+        projectDir, simpleProject(migrated: true, pubspecText: '''
+name: test
+environment:
+  foo: 1
+  sdk: '>=2.9.0 <2.10.0'
+'''));
+  }
+
+  test_pubspec_environment_is_not_a_map() async {
+    var pubspecText = '''
+name: test
+environment: 1
+''';
+    var projectContents = simpleProject(pubspecText: pubspecText);
+    var projectDir = await createProjectDir(projectContents);
+    var cli = _createCli();
+    await cli
+        .run(_parseArgs(['--no-web-preview', '--apply-changes', projectDir]));
+    // The Dart source code should still be migrated.
+    assertProjectContents(
+        projectDir, simpleProject(migrated: true, pubspecText: pubspecText));
+  }
+
+  test_pubspec_environment_sdk_is_not_string() async {
+    var pubspecText = '''
+name: test
+environment:
+  sdk: 1
+''';
+    var projectContents = simpleProject(pubspecText: pubspecText);
+    var projectDir = await createProjectDir(projectContents);
+    var cli = _createCli();
+    await cli
+        .run(_parseArgs(['--no-web-preview', '--apply-changes', projectDir]));
+    // The Dart source code should still be migrated.
+    assertProjectContents(
+        projectDir, simpleProject(migrated: true, pubspecText: pubspecText));
+  }
+
+  test_pubspec_is_missing_environment() async {
+    var projectContents = simpleProject(pubspecText: '''
+name: test
+''');
+    var projectDir = await createProjectDir(projectContents);
+    var cli = _createCli();
+    await cli
+        .run(_parseArgs(['--no-web-preview', '--apply-changes', projectDir]));
+    // The Dart source code should still be migrated.
+    assertProjectContents(projectDir, simpleProject(migrated: true, pubspecText:
+        // This is strange-looking, but valid.
+        '''
+environment:
+  sdk: '>=2.9.0 <2.10.0'
+
+name: test
+'''));
+  }
+
+  test_pubspec_is_not_a_map() async {
+    var projectContents = simpleProject(pubspecText: 'not-a-map');
+    var projectDir = await createProjectDir(projectContents);
+    var cli = _createCli();
+    expect(
+        () async => await cli.run(
+            _parseArgs(['--no-web-preview', '--apply-changes', projectDir])),
+        throwsUnsupportedError);
+  }
+
   test_uses_physical_resource_provider_by_default() {
     var cli = MigrationCli(binaryName: 'nnbd_migration');
     expect(cli.resourceProvider, same(PhysicalResourceProvider.INSTANCE));