Add fix for missing @required params.

Add fix for missing @required params.

Groundwork for: https://github.com/flutter/flutter-intellij/issues/553

BUG=
R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/e2808dff233a0c910cd8526b54138e371fef1259

[pquitslund, 2017-02-14 21:51:06]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[scheglov, 2017-02-14 22:06:07]

LGTM in general, but might be useful to add more tests.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/services/correction/fix.dart (right):

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix.dart:127:
'ADD_MISSING_REQUIRED_PARAMETER', 30, "Add required parameter");
We're not adding parameters, we're adding arguments for required parameters.
"Add required arguments" might be better.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/services/correction/fix_internal.dart (right):

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:556: if (node
is SimpleIdentifier && node.parent is MethodInvocation) {
Do we need this functionality for constructors?

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:557:
MethodInvocation invocation = node.parent;
I prefer speculatively name "AstNode invocation = node?.parent;" before "if" so
that after "is MethodInvocation" we know that it  actually is. But this might be
a recent change, so it's up to you. We might have many instances of doing just
like you did here, e.g. because this code was translated from Java :-)

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:567:
.where((p) => p.isRequired && !namedArgs.contains(p.name));
Maybe materialize using toList().

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:575:
NodeList<Expression> args = invocation.argumentList.arguments;
You already have "arguments" above.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:581: sb = new
SourceBuilder(file, args.last.endToken.offset + 1);
I think this would work only for single character expressions.
Maybe args.last.end is better?

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:588:
_addFix(DartFixKind.ADD_MISSING_REQUIRED_PARAMETER, []);
Should we use different kinds for adding one, and adding multiple required
parameters?

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/services/correction/fix_test.dart (right):

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/correction/fix_test.dart:299: void
_addMetaPackageSource() {
Please format and sort and files that you change in analyzer and analysis
server.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/correction/fix_test.dart:335:
test_addMissingRequiredParam_multiple() async {
This looks like "_hasOtherArgument" test.
But yes, adding multiple required arguments should be nice.

[Brian Wilkerson, 2017-02-14 22:23:12]

lgtm, modulo Konstantin's comments

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/services/correction/fix_internal.dart (right):

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:270: if
(errorCode == HintCode.MISSING_REQUIRED_PARAM) {
We might want to fix the hint code too.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:588:
_addFix(DartFixKind.ADD_MISSING_REQUIRED_PARAMETER, []);
> Should we use different kinds for adding one, and adding multiple required
> parameters?

Is there any reason not to always add all arguments? We don't have different
fixes for missing methods...

[pquitslund, 2017-02-14 23:41:38]

Thanks for the feedback!

I think I've got your concerns addressed...  PTAL.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/services/correction/fix.dart (right):

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix.dart:127:
'ADD_MISSING_REQUIRED_PARAMETER', 30, "Add required parameter");
On 2017/02/14 22:06:07, scheglov wrote:
> We're not adding parameters, we're adding arguments for required parameters.
> "Add required arguments" might be better.

Done.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/services/correction/fix_internal.dart (right):

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:270: if
(errorCode == HintCode.MISSING_REQUIRED_PARAM) {
On 2017/02/14 22:23:12, Brian Wilkerson wrote:
> We might want to fix the hint code too.

Probably.  Unfortunately, this will break options files that refer to this code.
 :/ Let's discuss a follow-up.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:556: if (node
is SimpleIdentifier && node.parent is MethodInvocation) {
On 2017/02/14 22:06:07, scheglov wrote:
> Do we need this functionality for constructors?

Yes!  Case added and a test.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:557:
MethodInvocation invocation = node.parent;
On 2017/02/14 22:06:07, scheglov wrote:
> I prefer speculatively name "AstNode invocation = node?.parent;" before "if"
so
> that after "is MethodInvocation" we know that it  actually is. But this might
be
> a recent change, so it's up to you. We might have many instances of doing just
> like you did here, e.g. because this code was translated from Java :-)

Done.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:567:
.where((p) => p.isRequired && !namedArgs.contains(p.name));
On 2017/02/14 22:06:07, scheglov wrote:
> Maybe materialize using toList().

Done.

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:575:
NodeList<Expression> args = invocation.argumentList.arguments;
On 2017/02/14 22:06:07, scheglov wrote:
> You already have "arguments" above.

Ah. Good catch!

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:581: sb = new
SourceBuilder(file, args.last.endToken.offset + 1);
On 2017/02/14 22:06:07, scheglov wrote:
> I think this would work only for single character expressions.
> Maybe args.last.end is better?

Good catch.  Fixed!

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:588:
_addFix(DartFixKind.ADD_MISSING_REQUIRED_PARAMETER, []);
On 2017/02/14 22:23:12, Brian Wilkerson wrote:
> > Should we use different kinds for adding one, and adding multiple required
> > parameters?
> 
> Is there any reason not to always add all arguments? We don't have different
> fixes for missing methods...

My thinking was to always just add them all.  If that's annoying in practice, we
can refine...

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/services/correction/fix_test.dart (right):

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/correction/fix_test.dart:299: void
_addMetaPackageSource() {
On 2017/02/14 22:06:07, scheglov wrote:
> Please format and sort and files that you change in analyzer and analysis
> server.

Whoops!  I got confused by the "Rearrange Code" action which I *thought* was
doing the right thing.  (PS: it's annoying that it doesn't!)

https://codereview.chromium.org/2692983003/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/correction/fix_test.dart:335:
test_addMissingRequiredParam_multiple() async {
On 2017/02/14 22:06:07, scheglov wrote:
> This looks like "_hasOtherArgument" test.
> But yes, adding multiple required arguments should be nice.

Acknowledged.

[Brian Wilkerson, 2017-02-14 23:58:20]

lgtm

https://codereview.chromium.org/2692983003/diff/40001/pkg/analysis_server/tes...
File pkg/analysis_server/test/services/correction/fix_test.dart (right):

https://codereview.chromium.org/2692983003/diff/40001/pkg/analysis_server/tes...
pkg/analysis_server/test/services/correction/fix_test.dart:501: }
Possibly add a test case where there is a normal (not named) parameter and
argument.

[scheglov, 2017-02-15 00:03:21]

lgtm

[pquitslund, 2017-02-15 00:13:44]

On 2017/02/14 23:58:20, Brian Wilkerson wrote:
> lgtm
> 
>
https://codereview.chromium.org/2692983003/diff/40001/pkg/analysis_server/tes...
> File pkg/analysis_server/test/services/correction/fix_test.dart (right):
> 
>
https://codereview.chromium.org/2692983003/diff/40001/pkg/analysis_server/tes...
> pkg/analysis_server/test/services/correction/fix_test.dart:501: }
> Possibly add a test case where there is a normal (not named) parameter and
> argument.

Not a bad idea.  Done.

Thank you!

[pquitslund, 2017-02-15 17:02:51]

Description was changed from

==========
Add fix for missing @required params.

Groundwork for: https://github.com/flutter/flutter-intellij/issues/553

BUG=
==========

to

==========
Add fix for missing @required params.

Groundwork for: https://github.com/flutter/flutter-intellij/issues/553

BUG=
R=brianwilkerson@google.com, scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/e2808dff233a0c910cd8526b54138e371fef1259
==========

[pquitslund, 2017-02-15 17:02:53]

Committed patchset #4 (id:60001) manually as
e2808dff233a0c910cd8526b54138e371fef1259 (presubmit successful).