Don't insert a trailing comma after a missing required argument, when there is on already.

R=brianwilkerson@google.com

Change-Id: I47819c9d2698880f793de2e3c056e1112b1b8f57
Reviewed-on: https://dart-review.googlesource.com/40402
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

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

@@ -602,6 +602,7 @@ class FixProcessor {
   }
 
   Future<Null> _addFix_addMissingRequiredArgument() async {
+    InstanceCreationExpression creation;
     Element targetElement;
     ArgumentList argumentList;
 
@@ -611,50 +612,59 @@ class FixProcessor {
         targetElement = invocation.methodName.bestElement;
         argumentList = invocation.argumentList;
       } else {
-        AstNode ancestor =
+        creation =
             invocation.getAncestor((p) => p is InstanceCreationExpression);
-        if (ancestor is InstanceCreationExpression) {
-          targetElement = ancestor.staticElement;
-          argumentList = ancestor.argumentList;
+        if (creation != null) {
+          targetElement = creation.staticElement;
+          argumentList = creation.argumentList;
         }
       }
     }
 
     if (targetElement is ExecutableElement) {
       // Format: "Missing required argument 'foo"
-      List<String> parts = error.message.split("'");
-      if (parts.length < 2) {
+      List<String> messageParts = error.message.split("'");
+      if (messageParts.length < 2) {
+        return;
+      }
+      String missingParameterName = messageParts[1];
+
+      ParameterElement missingParameter = targetElement.parameters.firstWhere(
+          (p) => p.name == missingParameterName,
+          orElse: () => null);
+      if (missingParameter == null) {
         return;
       }
 
-      // add proposal
-      String paramName = parts[1];
-      final List<Expression> args = argumentList.arguments;
-      int offset =
-          args.isEmpty ? argumentList.leftParenthesis.end : args.last.end;
+      int offset;
+      bool hasTrailingComma = false;
+      List<Expression> arguments = argumentList.arguments;
+      if (arguments.isEmpty) {
+        offset = argumentList.leftParenthesis.end;
+      } else {
+        Expression lastArgument = arguments.last;
+        offset = lastArgument.end;
+        hasTrailingComma = lastArgument.endToken.next.type == TokenType.COMMA;
+      }
+
       DartChangeBuilder changeBuilder = new DartChangeBuilder(session);
       await changeBuilder.addFileEdit(file, (DartFileEditBuilder builder) {
         builder.addInsertion(offset, (DartEditBuilder builder) {
-          if (args.isNotEmpty) {
+          if (arguments.isNotEmpty) {
             builder.write(', ');
           }
-          List<ParameterElement> parameters =
-              (targetElement as ExecutableElement).parameters;
-          ParameterElement element = parameters
-              .firstWhere((p) => p.name == paramName, orElse: () => null);
-          String defaultValue = getDefaultStringParameterValue(element);
-          builder.write('$paramName: $defaultValue');
+          String defaultValue =
+              getDefaultStringParameterValue(missingParameter);
+          builder.write('$missingParameterName: $defaultValue');
           // Insert a trailing comma after Flutter instance creation params.
-          InstanceCreationExpression newExpr =
-              flutter.identifyNewExpression(node);
-          if (newExpr != null && flutter.isWidgetCreation(newExpr)) {
+          if (!hasTrailingComma && flutter.isWidgetExpression(creation)) {
             builder.write(',');
           }
         });
       });
       _addFixFromBuilder(
           changeBuilder, DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT,
-          args: [paramName]);
+          args: [missingParameterName]);
     }
   }
 

pkg/analysis_server/lib/src/utilities/flutter.dart

@@ -300,7 +300,7 @@ bool isWidget(ClassElement element) {
  * class that has the Flutter class `Widget` as a superclass.
  */
 bool isWidgetCreation(InstanceCreationExpression expr) {
-  ClassElement element = expr.staticElement?.enclosingElement;
+  ClassElement element = expr?.staticElement?.enclosingElement;
   return isWidget(element);
 }
 

pkg/analysis_server/test/services/correction/fix_test.dart

@@ -614,9 +614,7 @@ build() {
 }
 ''');
 
-    await assertHasFix(
-        DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT,
-        '''
+    await assertHasFix(DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT, '''
 import 'package:flutter/widgets.dart';
 import 'package:meta/meta.dart';
 
@@ -627,8 +625,38 @@ class MyWidget extends Widget {
 build() {
   return new MyWidget(children: <Widget>[],);
 }
-''',
-        target: '/test.dart');
+''');
+  }
+
+  test_addMissingRequiredArg_cons_flutter_hasTrailingComma() async {
+    addFlutterPackage();
+    _addMetaPackageSource();
+
+    await resolveTestUnit('''
+import 'package:flutter/widgets.dart';
+import 'package:meta/meta.dart';
+
+class MyWidget extends Widget {
+  MyWidget({@required int a, @required int b});
+}
+
+build() {
+  return new MyWidget(a: 1,);
+}
+''');
+
+    await assertHasFix(DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT, '''
+import 'package:flutter/widgets.dart';
+import 'package:meta/meta.dart';
+
+class MyWidget extends Widget {
+  MyWidget({@required int a, @required int b});
+}
+
+build() {
+  return new MyWidget(a: 1, b: null,);
+}
+''');
   }
 
   test_addMissingRequiredArg_cons_single() async {
@@ -649,16 +677,13 @@ main() {
   A a = new A();
 }
 ''');
-    await assertHasFix(
-        DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT,
-        '''
+    await assertHasFix(DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT, '''
 import 'libA.dart';
 
 main() {
   A a = new A(a: null);
 }
-''',
-        target: '/test.dart');
+''');
   }
 
   test_addMissingRequiredArg_cons_single_closure() async {
@@ -682,16 +707,13 @@ main() {
   A a = new A();
 }
 ''');
-    await assertHasFix(
-        DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT,
-        '''
+    await assertHasFix(DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT, '''
 import 'libA.dart';
 
 main() {
   A a = new A(onPressed: () {});
 }
-''',
-        target: '/test.dart');
+''');
   }
 
   test_addMissingRequiredArg_cons_single_closure_2() async {
@@ -715,16 +737,13 @@ main() {
   A a = new A();
 }
 ''');
-    await assertHasFix(
-        DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT,
-        '''
+    await assertHasFix(DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT, '''
 import 'libA.dart';
 
 main() {
   A a = new A(callback: (e) {});
 }
-''',
-        target: '/test.dart');
+''');
   }
 
   test_addMissingRequiredArg_cons_single_closure_3() async {
@@ -748,16 +767,13 @@ main() {
   A a = new A();
 }
 ''');
-    await assertHasFix(
-        DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT,
-        '''
+    await assertHasFix(DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT, '''
 import 'libA.dart';
 
 main() {
   A a = new A(callback: (a, b, c) {});
 }
-''',
-        target: '/test.dart');
+''');
   }
 
   test_addMissingRequiredArg_cons_single_closure_4() async {
@@ -781,16 +797,13 @@ main() {
   A a = new A();
 }
 ''');
-    await assertHasFix(
-        DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT,
-        '''
+    await assertHasFix(DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT, '''
 import 'libA.dart';
 
 main() {
   A a = new A(callback: (int a, String b, c) {});
 }
-''',
-        target: '/test.dart');
+''');
   }
 
   test_addMissingRequiredArg_cons_single_list() async {
@@ -812,16 +825,13 @@ main() {
   A a = new A();
 }
 ''');
-    await assertHasFix(
-        DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT,
-        '''
+    await assertHasFix(DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT, '''
 import 'libA.dart';
 
 main() {
   A a = new A(names: <String>[]);
 }
-''',
-        target: '/test.dart');
+''');
   }
 
   test_addMissingRequiredArg_multiple() async {

pkg/analysis_server/test/src/utilities/flutter_test.dart

@@ -258,6 +258,8 @@ import 'package:flutter/widgets.dart';
 var a = new Object();
 var b = new Text('bbb');
 ''');
+    expect(isWidgetCreation(null), isFalse);
+
     InstanceCreationExpression a = _getTopVariableCreation('a');
     expect(isWidgetCreation(a), isFalse);