add required when generating methods with nnbd named arguments.

[bsutton]

If I use quickfix 'create method xxxx' and the method signature is:

final package = await DaoPackage().tryByMember(packageName: packageName);

The resulting generated signature is:

tryByMember({String packageName}) {}
`

The signature should be:
```dart
tryByMember({required String packageName}) {}

as nnbd named arguments require the 'required' keyword.

[DanTup]

I think it would also be valid to have String? packageName (for example if packageName in the call site was nullable), in which case required would not be required.

I thought this would be an easy fix, but it seems like in some situations the ? is not included so that should probably be fixed first (so we can conditionally add required). I've raised dart-lang/sdk#48086 about that and will look at this again after it's fixed.

[bsutton]

I'm only suggesting that we add the required for String not String?.

We don't have enough information to know the intent of String? and I would think not requiring a nullable type would be the most common use case.

[DanTup]

I'm only suggesting that we add the required for String not String?.

Yep, I understand. But in the case linked above, I'm not certain if we're always getting the type correctly (it seems to be non-nullable in cases where I expected it to be nullable). However, I added another comment and think maybe it's working as intended an dit's because my example was a literal string and the type was narrowed to just String. I'll wait for confirmation on that though, but I now think it's probably a non-issue :-)

[DanTup]

@bwilkerson interested in your opinion on this. Inserting required in the case above seems to make sense, because without it the code is invalid. However the code used to generate the parameters here is shared with some other fixes such as "add missing named parameter" and I think it's less clear what the behaviour should be there. For example, here's an existing test:

  Future<void> test_method_noParameters() async {
    await resolveTestCode('''
class A {
  test() {}

  main() {
    test(named: 42);
  }
}
''');
    await assertHasFix('''
class A {
  test({int named}) {}

  main() {
    test(named: 42);
  }
}
''');
  }

The generated code is also invalid, but I'm not sure if it's clear that it's better to insert required int versus int?. I feel like both are going to be wrong at some point. Should we just pick one, or is it better to generate the code with a warning here and force the user to pick? (or, should there be two options in the fixes?)

(Edit: After typing this, I'm actually wondering whether making the parameter nullable in the original case it also reasonable... just because you're supplying a non-null value here, doesn't necessarily mean that's how you want the new function to be?)

[bsutton]

With nnbd 90+% of my args are not null. So if they is uncertainty the nn should be selected.

…

On Tue, 11 Jan 2022, 12:04 am Danny Tuppeny, ***@***.***> wrote: @bwilkerson <https://github.com/bwilkerson> interested in your opinion on this. Inserting required in the case above seems to make sense, because without it the code is invalid. However the code used to generate the parameters here is shared with some other fixes such as "add missing named parameter" and I think it's less clear what the behaviour should be there. For example, here's an existing test: Future<void> test_method_noParameters() async { await resolveTestCode('''class A { test() {} main() { test(named: 42); }}'''); await assertHasFix('''class A { test({int named}) {} main() { test(named: 42); }}'''); } The generated code is also invalid, but I'm not sure if it's clear that it's better to insert required int versus int?. I feel like both are going to be wrong at some point. Should we just pick one, or is it better to generate the code with a warning here and force the user to pick? (or, should there be two options in the fixes?) — Reply to this email directly, view it on GitHub <#3770 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OBNEJDKLGVXU4U3IFDUVLKKJANCNFSM5LLRUE5Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[bwilkerson]

Inserting required in the case above seems to make sense, because without it the code is invalid.

I agree.

After typing this, I'm actually wondering whether making the parameter nullable in the original case it also reasonable... just because you're supplying a non-null value here, doesn't necessarily mean that's how you want the new function to be?

That's true, but I think in this case it's better to choose the most likely option (which hopefully for most cases will be to use non-nullable types) and let the user change the code manually if necessary. This case has the advantage that we don't have any other invocations of the method being added, which means choosing to make it required won't produce any follow-on errors.

However the code used to generate the parameters here is shared with some other fixes such as "add missing named parameter" and I think it's less clear what the behaviour should be there.
...
The generated code is also invalid, but I'm not sure if it's clear that it's better to insert required int versus int?. I feel like both are going to be wrong at some point.

Yeah, that case is a bit more complicated. There might be other invocation sites, so there might not be a single change that won't cause follow-on errors.

I'm not opposed to having two options for users, but I'm also not opposed to providing a single required int option now and seeing how often users want the other option before adding support for it.

[bsutton]

I'm not opposed to having two options for users, but I'm also not opposed to providing a single required int option now and seeing how often users want the other option before adding support for it.

agreed with single required int option for the moment.
Less work and if no one complains it was never needed.

We should be encouraging uses to use nnbd.
99% of my args are nnbd and I would imagine other developers are heading in the same direction as it's just easier to work with.

[DanTup]

Fixed by dart-lang/sdk@32b3e18. It's an SDK change so will show up in a future SDK release rather than a Dart-Code one.