proposal: unnecessary_final. Allow final local variables that shadow fields

[scheglov]

unnecessary_final

Description

Allow final local variables that shadow fields.

Details

Bad Examples

Same as now.

Good Examples

Allow

class A {
  int x = 0;
  void f() {
    final x = this.x;
  }
}

Discussion

This is the code style that the analyzer team follows.

See https://dart-review.googlesource.com/c/sdk/+/362901 for the change (will be removed in new updates) and the bulk updates to analysis_server.

[pq]

Should final y = this.x; be final x = this.x;?

Assuming so, I'm in favor.

@lrhn @munificent @natebosch @jakemac53 WDYT?

[lrhn]

Why?

Is it to prevent someone accidentally assigning to the local variable, when they intended to assign to the instance variable?
If so, maybe only allow it the instance variable is actually assignable.

Honestly, I'd prefer this as a separate opt-in exception/other lint. If I were to use a lint to enforce no local finals, I'd want it to actually do that.

[natebosch]

I think there has been some precedent for changing a lint behavior based on whether another lint is enabled. We could consider a only_shadow_with_final_locals or something along those lines, and when it is enabled that could be the signal to allow this case. Is there value in a lint that disallows assignment to a shadowing variable? Is that the intent of the analyzer team?

[bwilkerson]

Is it to prevent someone accidentally assigning to the local variable, when they intended to assign to the instance variable?

Yes, that's the reasoning behind our exception to the rule.

I'm not convinced that adding the exception to the lint is in the best interest of our users, and the ideas about creating a new lint, whether to replace or augment the existing lint, are interesting, so thanks for mentioning them.

[scheglov]

With https://dart-review.googlesource.com/c/sdk/+/366960 landed, we basically decided that we don't want this exception.