Wire up argument_type_not_assignable for data driven fixes.

Fix nits in documentation.

Add support for data driven fix for change parameter to non null.


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

pkg/analysis_server/doc/implementation/data_driven_fixes.md

@@ -42,13 +42,13 @@ it's implemented in because we want these fixes to continue to work even after
 the element has been removed, at which point the reference to the element will
 be unresolved.
 
-We're making an assumption here that the file contains an import for a lilbrary
+We're making an assumption here that the file contains an import for a library
 in the element's defining package. That isn't necessarily true, and we might
 consider looking at all of the packages that are directly or indirectly depended
 on, but (a) that would have performance implications that haven't been assessed,
 and (b) we don't have any reports that this limitation is causing problems.
 
-For each imported package, it askes the `TransformSetManager` for the
+For each imported package, it asks the `TransformSetManager` for the
 `TransformSet` associated with the package. The transform manager is responsible
 for building a transform set from the data file(s) in the package. For the sake
 of performance, the transform sets are cached. The process of building a
@@ -76,7 +76,7 @@ you would instead modify something that already exists.
 ### Design the change
 
 The first step is to design the changes to the data-file format that will allow
-users to specify the change. Follow the design prinicples outlined in the
+users to specify the change. Follow the design principles outlined in the
 Overview in
 [Data-driven Fixes](https://github.com/flutter/flutter/wiki/Data-driven-Fixes).
 

pkg/analysis_server/lib/src/services/correction/fix/data_driven/code_template.dart

@@ -113,6 +113,11 @@ class TemplateContext {
         return parent;
       }
     }
+    var parent = node.parent;
+    if (parent is NamedExpression &&
+        parent.parent?.parent is InstanceCreationExpression) {
+      return parent.parent?.parent;
+    }
     return null;
   }
 }

pkg/analysis_server/lib/src/services/correction/fix/data_driven/element_matcher.dart

@@ -165,6 +165,9 @@ class _MatcherBuilder {
       _addMatcher(components: [node.name.lexeme], kinds: []);
     } else if (node is Literal) {
       var parent = node.parent;
+      if (parent is NamedExpression) {
+        parent = parent.parent;
+      }
       if (parent is ArgumentList) {
         _buildFromArgumentList(parent);
       }

pkg/analysis_server/lib/src/services/correction/fix/data_driven/modify_parameters.dart

@@ -8,6 +8,7 @@ import 'package:analysis_server/src/services/correction/fix/data_driven/code_tem
 import 'package:analysis_server/src/services/correction/fix/data_driven/parameter_reference.dart';
 import 'package:analysis_server/src/utilities/index_range.dart';
 import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/source/source_range.dart';
 import 'package:analyzer_plugin/utilities/change_builder/change_builder_dart.dart';
 import 'package:analyzer_plugin/utilities/range_factory.dart';
 
@@ -41,6 +42,28 @@ class AddParameter extends ParameterModification {
       : assert(index >= 0);
 }
 
+/// The type change of a parameter.
+class ChangeParameterType extends ParameterModification {
+  /// The name of the parameter that was changed, used with named parameters.
+  final String? name;
+
+  /// The index of the parameter to be changed, used with positional parameters.
+  final int? index;
+
+  /// The nullability of the parameter.
+  final String nullability;
+
+  /// The code template used to compute the value of the new argument in
+  /// invocations of the function, or `null` if the parameter is optional and no
+  /// argument needs to be added. The only time an argument needs to be added
+  /// for an optional parameter is if the parameter is positional and there are
+  /// preexisting optional positional parameters after the ones being added.
+  final CodeTemplate? argumentValue;
+
+  ChangeParameterType(
+      this.name, this.index, this.nullability, this.argumentValue);
+}
+
 /// The data related to an executable element whose parameters have been
 /// modified.
 class ModifyParameters extends Change<_Data> {
@@ -67,7 +90,7 @@ class ModifyParameters extends Change<_Data> {
     var argumentCount = arguments.length;
     var templateContext = TemplateContext(invocation, fix.utils);
 
-    var indexToNewArgumentMap = <int, AddParameter>{};
+    var indexToNewArgumentMap = <int, ParameterModification>{};
     var argumentsToInsert = <int>[];
     var argumentsToDelete = <int>[];
     var remainingArguments = [for (var i = 0; i < argumentCount; i++) i];
@@ -95,6 +118,32 @@ class ModifyParameters extends Change<_Data> {
           argumentsToDelete.add(index);
           remainingArguments.remove(index);
         }
+      } else if (modification is ChangeParameterType) {
+        var reference = modification.name == null
+            ? PositionalParameterReference(modification.index!)
+            : NamedParameterReference(modification.name!);
+        var argument = reference.argumentFrom(argumentList);
+        if (argument == null) {
+          // If there is no argument corresponding to the parameter then we assume
+          // that the parameter was absent.
+          var index = reference is PositionalParameterReference
+              ? reference.index
+              : remainingArguments.last + 1;
+          remainingArguments.add(index);
+          indexToNewArgumentMap[index] = modification;
+          argumentsToInsert.add(index);
+        } else {
+          // Check and replace null value arguments.
+          if (argument is NullLiteral) {
+            var argumentValue = modification.argumentValue;
+            if (argumentValue != null) {
+              builder.addReplacement(
+                  SourceRange(argument.offset, argument.length), (builder) {
+                argumentValue.writeOn(builder, templateContext);
+              });
+            }
+          }
+        }
       }
     }
     argumentsToInsert.sort();
@@ -113,6 +162,20 @@ class ModifyParameters extends Change<_Data> {
       }
     }
 
+    /// Write to the [builder] the change associated with a single
+    /// [parameter].
+    void writeChangeArgument(
+        DartEditBuilder builder, ChangeParameterType parameter) {
+      var argumentValue = parameter.argumentValue;
+      if (argumentValue != null) {
+        if (parameter.index == null) {
+          builder.write(parameter.name!);
+          builder.write(': ');
+        }
+        argumentValue.writeOn(builder, templateContext);
+      }
+    }
+
     var insertionRanges = IndexRange.contiguousSubRanges(argumentsToInsert);
     var deletionRanges = IndexRange.contiguousSubRanges(argumentsToDelete);
     if (insertionRanges.isNotEmpty) {
@@ -132,7 +195,12 @@ class ModifyParameters extends Change<_Data> {
           }
           var parameter = indexToNewArgumentMap[argumentIndex];
           if (parameter != null) {
-            writeArgument(builder, parameter);
+            switch (parameter) {
+              case AddParameter():
+                writeArgument(builder, parameter);
+              case ChangeParameterType():
+                writeChangeArgument(builder, parameter);
+            }
           }
         }
       }
@@ -197,9 +265,19 @@ class ModifyParameters extends Change<_Data> {
         var lower = insertionRange.lower;
         var upper = insertionRange.upper;
         var parameter = indexToNewArgumentMap[upper]!;
-        while (upper >= lower &&
-            (parameter.isPositional && !parameter.isRequired)) {
-          upper--;
+        switch (parameter) {
+          // Changing the type of parameter to non null indicates that a value
+          // must be passed in, regardless of whether is it positional or
+          // required.
+          case AddParameter():
+            while (upper >= lower &&
+                (parameter.isPositional && !parameter.isRequired)) {
+              upper--;
+            }
+          case ChangeParameterType():
+            while (upper >= lower) {
+              upper--;
+            }
         }
         if (upper >= lower) {
           builder.addInsertion(offset, (builder) {
@@ -257,6 +335,13 @@ class ModifyParameters extends Change<_Data> {
         greatGrandParent is InstanceCreationExpression) {
       var argumentList = greatGrandParent.argumentList;
       return _Data(argumentList);
+    } else if (parent is NamedExpression &&
+        greatGrandParent is InstanceCreationExpression) {
+      var argumentList = greatGrandParent.argumentList;
+      return _Data(argumentList);
+    } else if (grandParent is InstanceCreationExpression) {
+      var argumentList = grandParent.argumentList;
+      return _Data(argumentList);
     }
     return null;
   }

pkg/analysis_server/lib/src/services/correction/fix/data_driven/transform_set_parser.dart

@@ -67,6 +67,7 @@ class TransformSetParser {
   static const String _nameKey = 'name';
   static const String _newElementKey = 'newElement';
   static const String _newNameKey = 'newName';
+  static const String _nullabilityKey = 'nullability';
   static const String _oldNameKey = 'oldName';
   static const String _oneOfKey = 'oneOf';
   static const String _requiredIfKey = 'requiredIf';
@@ -95,6 +96,7 @@ class TransformSetParser {
 
   static const String _addParameterKind = 'addParameter';
   static const String _addTypeParameterKind = 'addTypeParameter';
+  static const String _changeParameterTypeKind = 'changeParameterType';
   static const String _fragmentKind = 'fragment';
   static const String _importKind = 'import';
   static const String _removeParameterKind = 'removeParameter';
@@ -110,6 +112,9 @@ class TransformSetParser {
     'required_positional'
   ];
 
+  /// The valid values for the [_nullabilityKey] in a [_changeParameterTypeKind] change.
+  static const List<String> validNullabilityChanges = ['non_null'];
+
   /// A table mapping the kinds of elements that can be replaced by a different
   /// element to a set of the kinds of elements with which they can be replaced.
   static final Map<ElementKind, Set<ElementKind>> compatibleReplacementTypes =
@@ -458,6 +463,9 @@ class TransformSetParser {
       } else if (kind == _removeParameterKind) {
         _translateRemoveParameterChange(node);
         return null;
+      } else if (kind == _changeParameterTypeKind) {
+        _translateChangeParameterTypeChange(node);
+        return null;
       } else if (kind == _renameKind) {
         return _translateRenameChange(node);
       } else if (kind == _renameParameterKind) {
@@ -478,6 +486,62 @@ class TransformSetParser {
     }
   }
 
+  void _translateChangeParameterTypeChange(YamlMap node) {
+    _reportUnsupportedKeys(node, const {
+      _argumentValueKey,
+      _indexKey,
+      _kindKey,
+      _nameKey,
+      _nullabilityKey
+    });
+
+    var parameterSpecKey = _singleKey(node, const [_nameKey, _indexKey], node);
+    if (parameterSpecKey == null) {
+      // The error has already been reported.
+      return;
+    }
+
+    int? index;
+    String? name;
+    if (parameterSpecKey == _indexKey) {
+      index = _translateInteger(node.valueAt(_indexKey),
+          ErrorContext(key: _indexKey, parentNode: node));
+      if (index == null) {
+        // The error has already been reported.
+        return;
+      }
+    } else {
+      name = _translateString(node.valueAt(_nameKey),
+          ErrorContext(key: _nameKey, parentNode: node));
+      if (name == null) {
+        // The error has already been reported.
+        return;
+      }
+    }
+
+    var nullabilityNode = node.valueAt(_nullabilityKey);
+    var nullability = _translateString(
+        nullabilityNode, ErrorContext(key: _nullabilityKey, parentNode: node));
+    if (nullability == null) {
+      // The error has already been reported.
+      return;
+    }
+    if (!validNullabilityChanges.contains(nullability)) {
+      var nullabilityChangeList =
+          validNullabilityChanges.map((value) => value).join(', ');
+      _reportError(TransformSetErrorCode.invalidValueOneOf, nullabilityNode!,
+          [_nullabilityKey, nullabilityChangeList]);
+      return;
+    }
+    var argumentValueNode = node.valueAt(_argumentValueKey);
+    var argumentValue = _translateCodeTemplate(argumentValueNode,
+        ErrorContext(key: _argumentValueKey, parentNode: node),
+        canBeConditionallyRequired: false);
+    _parameterModifications ??= [];
+    _parameterModifications!
+        .add(ChangeParameterType(name, index, nullability, argumentValue));
+  }
+
   /// Translate the [node] into a value generator. Return the resulting
   /// generator, or `null` if the [node] does not represent a valid value
   /// extractor.

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

@@ -781,6 +781,9 @@ class FixProcessor extends BaseProcessor {
     CompileTimeErrorCode.AMBIGUOUS_EXTENSION_MEMBER_ACCESS: [
       AddExtensionOverride.new,
     ],
+    CompileTimeErrorCode.ARGUMENT_TYPE_NOT_ASSIGNABLE: [
+      DataDriven.new,
+    ],
     CompileTimeErrorCode.CAST_TO_NON_TYPE: [
       ImportLibrary.forType,
     ],

pkg/analysis_server/test/src/services/correction/fix/data_driven/flutter_use_case_test.dart

@@ -2274,6 +2274,141 @@ void f() {
 ''');
   }
 
+  Future<void> test_services_ClipboardData_changeParameterNonNull() async {
+    setPackageContent('''
+class ClipboardData {
+  const ClipboardData({required String this.text});
+
+  final String? text;
+}
+''');
+
+    addPackageDataFile('''
+version: 1
+transforms:
+  - title: "Migrate to empty 'text' string"
+    date: 2023-04-19
+    element:
+      uris: ['$importUri']
+      constructor: ''
+      inClass: 'ClipboardData'
+    changes:
+      - kind: 'changeParameterType'
+        name: 'text'
+        nullability: non_null
+        argumentValue:
+          expression: "''"
+''');
+
+    await resolveTestCode('''
+import '$importUri';
+
+void f() {
+  var c = ClipboardData(text: null);
+  print(c);
+}
+''');
+    await assertHasFix('''
+import '$importUri';
+
+void f() {
+  var c = ClipboardData(text: '');
+  print(c);
+}
+''');
+  }
+
+  Future<void>
+      test_services_ClipboardData_changeParameterNonNullAbsent() async {
+    setPackageContent('''
+class ClipboardData {
+  const ClipboardData({required String this.text, String? this.p});
+
+  final String? text;
+  final String? p;
+}
+''');
+
+    addPackageDataFile('''
+version: 1
+transforms:
+  - title: "Migrate to empty 'text' string"
+    date: 2023-04-19
+    element:
+      uris: ['$importUri']
+      constructor: ''
+      inClass: 'ClipboardData'
+    changes:
+      - kind: 'changeParameterType'
+        name: 'text'
+        nullability: non_null
+        argumentValue:
+          expression: "''"
+''');
+
+    await resolveTestCode('''
+import '$importUri';
+
+void f() {
+  var c = ClipboardData(p: 'hello', text: null);
+  print(c);
+}
+''');
+    await assertHasFix('''
+import '$importUri';
+
+void f() {
+  var c = ClipboardData(p: 'hello', text: '');
+  print(c);
+}
+''');
+  }
+
+  Future<void> test_services_ClipboardData_changePositionalParameter() async {
+    setPackageContent('''
+class ClipboardData {
+  const ClipboardData(this.text, this.p);
+
+  final String text;
+  final String p;
+}
+''');
+
+    addPackageDataFile('''
+version: 1
+transforms:
+  - title: "Migrate to empty 'text' string"
+    date: 2023-04-19
+    element:
+      uris: ['$importUri']
+      constructor: ''
+      inClass: 'ClipboardData'
+    changes:
+      - kind: 'changeParameterType'
+        index: 1
+        nullability: non_null
+        argumentValue:
+          expression: "''"
+''');
+
+    await resolveTestCode('''
+import '$importUri';
+
+void f() {
+  var c = ClipboardData('hello', null);
+  print(c);
+}
+''');
+    await assertHasFix('''
+import '$importUri';
+
+void f() {
+  var c = ClipboardData('hello', '');
+  print(c);
+}
+''');
+  }
+
   Future<void>
       test_widgets_BuildContext_ancestorInheritedElementForWidgetOfExactType_deprecated() async {
     setPackageContent('''

pkg/analysis_server/test/src/services/correction/fix/data_driven/transform_set_parser_test.dart

@@ -426,6 +426,112 @@ transforms:
     expect(_changes(transform), isEmpty);
   }
 
+  void test_changeParameterType_named_nullability() {
+    assertNoErrors('''
+version: 1
+transforms:
+- title: 'Change parameter type'
+  date: 2023-05-02
+  element:
+    uris: ['test.dart']
+    function: 'f'
+  changes:
+    - kind: 'changeParameterType'
+      name: 'p'
+      nullability: 'non_null'
+      argumentValue:
+        expression: "'newValue'"
+''');
+    var transforms = _transforms('f');
+    expect(transforms, hasLength(1));
+    var transform = transforms[0];
+    expect(transform.title, 'Change parameter type');
+    var changes = _changes(transform);
+    expect(changes, hasLength(1));
+    var change = changes[0] as ModifyParameters;
+    var modifications = change.modifications;
+    expect(modifications, hasLength(1));
+    var modification = modifications[0] as ChangeParameterType;
+    expect(modification.name, 'p');
+    var components = modification.argumentValue!.components;
+    expect(components, hasLength(1));
+    var value = (components[0] as TemplateText).text;
+    expect(value, "'newValue'");
+  }
+
+  void test_changeParameterType_nullabilityKey_absent() {
+    assertErrors('''
+version: 1
+transforms:
+- title: 'Change parameter type'
+  date: 2023-05-02
+  element:
+    uris: ['test.dart']
+    function: 'f'
+  changes:
+    - kind: 'changeParameterType'
+      name: 'p'
+      argumentValue:
+        expression: "'newValue'"
+''', [
+      error(TransformSetErrorCode.missingKey, 145, 98),
+    ]);
+  }
+
+  void test_changeParameterType_oneOf_named_index_keys() {
+    assertErrors('''
+version: 1
+transforms:
+- title: 'Change parameter type'
+  date: 2023-05-02
+  element:
+    uris: ['test.dart']
+    function: 'f'
+  changes:
+    - kind: 'changeParameterType'
+      name: 'p'
+      index: 0
+      nullability: 'non_null'
+      argumentValue:
+        expression: "'newValue'"
+''', [
+      error(TransformSetErrorCode.conflictingKey, 195, 5),
+    ]);
+  }
+
+  void test_changeParameterType_positional_nullability() {
+    assertNoErrors('''
+version: 1
+transforms:
+- title: 'Change parameter type'
+  date: 2023-05-02
+  element:
+    uris: ['test.dart']
+    function: 'f'
+  changes:
+    - kind: 'changeParameterType'
+      index: 0
+      nullability: 'non_null'
+      argumentValue:
+        expression: "'newValue'"
+''');
+    var transforms = _transforms('f');
+    expect(transforms, hasLength(1));
+    var transform = transforms[0];
+    expect(transform.title, 'Change parameter type');
+    var changes = _changes(transform);
+    expect(changes, hasLength(1));
+    var change = changes[0] as ModifyParameters;
+    var modifications = change.modifications;
+    expect(modifications, hasLength(1));
+    var modification = modifications[0] as ChangeParameterType;
+    expect(modification.index, 0);
+    var components = modification.argumentValue!.components;
+    expect(components, hasLength(1));
+    var value = (components[0] as TemplateText).text;
+    expect(value, "'newValue'");
+  }
+
   void test_correctOffsetForPlainStrings() {
     assertErrors('''
 version: 1