changes to prefer_final_locals breaking flutter analyze trybot

[pq]

https://ci.chromium.org/p/dart/builders/luci.dart.ci.sandbox/flutter-analyze/107

841 issues like this:

   info • Prefer final for variable declaration if reference is not reassigned • dev/benchmarks/microbenchmarks/lib/common.dart:54:27 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/benchmarks/microbenchmarks/lib/common.dart:62:27 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/benchmarks/microbenchmarks/lib/gestures/velocity_tracker_bench.dart:18:23 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:148:17 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:171:21 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:255:15 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:268:19 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:379:18 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:382:18 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:519:17 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:734:18 • prefer_final_locals
   info • Prefer final for variable declaration if reference is not reassigned • dev/bots/analyze-sample-code.dart:794:17 • prefer_final_locals

...

Addressing these warnings blocks integration of 0.1.77. A few options:

1. fix flutter code to produce warnings
2. disable the lint in flutter
3. revert the change to the rule and re-publish

Regardless of what we decide we should probably start w/

0. review the new warnings to see if they're adding value (or possibly false positives)

/cc @srawlins

[srawlins]

I think that whatever value is provided by prefer_final_locals is also provided when applying it to for-each loop variables. It's definitely not in most people's muscle memory, but if a team wants the benefits that this rule provides, I think it should be applied to for-each loop variables as well.

[srawlins]

I glanced at a few of these:

https://github.com/flutter/flutter/blob/e4515824a1aa9504fb9cab788afe5e05190aec52/dev/benchmarks/microbenchmarks/lib/common.dart#L54

https://github.com/flutter/flutter/blob/e4515824a1aa9504fb9cab788afe5e05190aec52/dev/benchmarks/microbenchmarks/lib/common.dart#L62

https://github.com/flutter/flutter/blob/e4515824a1aa9504fb9cab788afe5e05190aec52/dev/benchmarks/microbenchmarks/lib/gestures/velocity_tracker_bench.dart#L18

I don't see any that are false positives.

[pq]

Thanks! I guess the question now becomes whether these new semantics line up with what Flutter wants from the rule.

@Hixie, curious how you feel. With Sam's fix, the follow code now lints:

  for (var i in [1, 2, 3]) { // LINT
    print(i);
  }

and should read:

  for (final i in [1, 2, 3]) { // OK
    print(i);
  }

If you're OK with that change, we can put together a PR to fix up the Flutter codebase.

[srawlins]

I think the more egregious / non-muscle-memory changes are when an explicit type is provided, like:

for (final _BenchmarkResult result in _results) {
  results[result.name] = result.value;
}

[Hixie]

Yeah, as much as I like final, I'd really rather we just change the language to (as a file-level opt-in, maybe?) default everything to final unless you explicitly say var, than having final in for loop declarations and argument declarations and everything. It's pretty verbose already as it is.

[pq]

I'd really rather we just change the language

I agree with this. As a general rule I think mutability is the degenerate case and not the other way around.

That said, we do have a decision to make here. I think our choices are probably down to:

1. lumping it and verbosifying flutter code to remove warnings
2. disable the lint in flutter
3. revert the change to the rule and re-publish

If we opt for 3, it may make sense to add a new rule (sigh) for this case specifically (something like prefer_final_iterators... 😬 ).

/cc @bwilkerson FYI

[Hixie]

I definitely would go for 3b (have separate lints for variable declarations, parameters, for loop variables, etc).

[pq]

I agree w/ @Hixie. Lets roll this back and break it out into a separate lint as proposed by @srawlins in #1346.