Dart fix for unused_element removes parameters when it shouldn't

[gspencergoog]

When applying dart fix --apply to the following code:

void main() {
  final _Foo foo = _Foo();
  print('$foo');
}

class _Foo {
  _Foo({this.bar}); // The "bar" parameter is never given a value, but it's nullable.

  final String? bar;
}

The bar parameter is never given a value, so the unused_element lint shows up for that line:

   info • bug.dart:7:14 • A value for optional parameter 'bar' isn't ever given. Try removing the unused parameter. • unused_element

Running dart fix --apply to the code yields:

void main() {
  final _Foo foo = _Foo();
  print('$foo');
}

class _Foo {
  _Foo();

  final String? bar;
}

Which yields the following error:

  error • bug.dart:7:3 • All final variables must be initialized, but 'bar' isn't. Try adding an initializer for the field. • final_not_initialized_constructor

The Dart Fix for unused_element shouldn't remove parameters that are nullable, since they have implicit default values. Alternatively, it could also remove the uninitialized variable, but I suspect that has tons of corner cases.

[gspencergoog]

cc @goderbauer

[bwilkerson]

I would go one step farther. I don't think that unused_element should flag that parameter because it's being used to initialize the field, even when no argument is passed in.

@srawlins

[srawlins]

I believe the fix for an unused field-formal parameter (this.) should remove the parameter, and add bar=null to the initializer list.

I would go one step farther. I don't think that unused_element should flag that parameter because it's being used to initialize the field, even when no argument is passed in.

I don't think that's a reason to pollute the parameter list of a constructor. The situation should still be reported, and the fix can be to move the initialization to the constructor initializer list. The goal is to indicate unnecessary complexity, perhaps unnecessary fields (such as the final field which is only ever initialized with a value of null).

[gspencergoog]

I assume this is what the end result above would be:

void main() {
  final _Foo foo = _Foo();
  print('$foo');
}

class _Foo {
  _Foo() : bar = null;

  final String? bar; // Would this line get a lint warning that says it's always null?
}

So, in that case, would there also now be a lint warning for the final field that is always null? Presumably there's no dart fix for that because of the possible complexity involved in removing the member and all of its uses.

[srawlins]

We actually don't produce any warnings on that last code. We also don't produce a warning if you have a final field with an initializer like final foo = 'hello';.

[gspencergoog]

Oh, sure, I guess it could easily be the case that someone wants a base class to have a final field that is always null that they then override the getter for in a subclass.