dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.27k stars 1.58k forks source link

autofix for dead_null_aware_expression #47159

Open Hixie opened 3 years ago

Hixie commented 3 years ago

It would be great if dart fix could fix dead_null_aware_expression.

In particular, consider an API like this:

int? foo() => 0;

void main() {
  int x = foo() ?? 0;
}

If I change the foo method to be more precise in its return type by making it non-nullable, I get a dead_null_aware_expression info in the main function:

int foo() => 0;

void main() {
  int x = foo() ?? 0; // dead_null_aware_expression
}

It would be great if dart fix could just remove the ?? operator (and the right operand) for me.

https://github.com/flutter/flutter/pull/89451 is an example of an API change that triggered this quite a bit.

asashour commented 3 years ago

https://dart-review.googlesource.com/c/sdk/+/212824

jcollins-g commented 2 years ago

It looks like the fix in https://dart-review.googlesource.com/c/sdk/+/212824 has some issues and was abandoned. I will take a look.

jcollins-g commented 2 years ago

I don't think it would be safe to always remove dead null aware expressions in all codebases. There's currently no way to specify that it is safe in a particular codebase or for a particular run (#49224). Expanding this fix so it can fix all instances in a file (although not through dart fix) would help reduce developer effort in the given case, though, I'll give that a try.

Hixie commented 2 years ago

what's an example where it's not safe? I guess when you need to be compatible with two versions of the API, one of which is nullable?

bwilkerson commented 2 years ago

Another example, which is currently being discussed elsewhere, is when you've run the null-safety migration tool and it's made some suboptimal choices. If we were to bulk apply this fix then the information about the code that users need to validate would be lost.

It's possible that over time, as more of the community is migrated, that the number of cases where such a fix might be detrimental will decrease to the point where we can safely enable it.

Hixie commented 2 years ago

It would make sense for cases like this for dart fix --apply to require another argument, like --include-unsafe-fixes= dead_null_aware_expression (maybe in the normal mode it could include a message saying something like "Detected 3 cases of dead_null_aware_expression that were not fixed because they could not be guaranteed to be correct. Use --include-unsafe-fixes to force fixes for these cases as well. See https://dart.dev/... for details." or something).

bwilkerson commented 2 years ago

We have definitely talked a few times about adding support for specifying a set of diagnostics to fix and/or a set of fixes to apply. We just haven't gotten to the point of starting a design doc where we can discuss the various use cases and how best to structure the UI.

jcollins-g commented 2 years ago

@Hixie I filed https://github.com/dart-lang/sdk/issues/49224 last week when I bumped into this, good to know we're on the same page.