@lsaudon thank you for this massive contribution!

Unfortunately, you have merge conflicts on this PR. But, given the size, I think it might be infeasible to code review, and to keep up to date with upstream changes. The only way to land this kind of a migration might be to do it in smaller chunks before the lint is enabled. WDYT @goderbauer ?

I knew it was too big.
Indeed I can make by smaller part.

I just wanted to say I think I like the move to prefer_final_parameters. I was wondering why we didn't follow that.

I agree with @christopherfujino that you should split this up, maybe be library or something, and merge those PRs individually. Then enable the lint parameter at the end.

I just wanted to say I think I like the move to prefer_final_parameters. I was wondering why we didn't follow that.

I'm not opposed to this change, but it's unfortunate it adds so much verbosity, alas...

Yeah that is a good point. I think it's worth it even though I have to type final a lot more, but others may disagree.

I do think this will be a worthwhile change. Landing it may be difficult given how many files this affects, which is one reason why we haven't done this earlier. Doing it in smaller chunks is a good idea. Another idea was to land something like this on a major holiday where traffic in the repo is low to avoid merge conflicts...

Looks like you're already opening up smaller PRs. I am going to close this one then.

but it's unfortunate it adds so much verbosity, alas...

I kinda wish Dart would have picked 'final' as the default and you had to specify something if you didn't want final since that seems to be less common... Oh well...

This seems to be adding a lot of verbosity to flutter tool code. What sort of errors does this catch? Do we make those mistakes in practice? I'm not convinced this is a positive change.

I agree, this is a lot of churn that makes the resulting code substantially more verbose. I don't think the benefits outweigh the costs here, as I cannot recall any instances of accidentally assigning to a parameter

I use this in my personal projects (largely because I turn on all the lints by default and then only opt-out the ones that are just obviously wrong or mutually-exclusive) and I must admit I don't think this lint has ever really done me any good, and as others have said, it's pretty verbose. I think the cost/benefit on this one is pretty questionable. I never put the final in there until I try to compile, and then I just have to add "final"s everywhere, which is very tedious.

That said I'm happy to defer to the relevant code owners (@christopherfujino for the tool, @goderbauer for the framework, @stuartmorgan for the packages repo) in terms of whether we should actually do this. I just ask that we be consistent throughout the repo (and dart:ui, and ideally flutter/packages) about it.

Maybe what we really want is a lint that you should never re-assign an argument.

That said I'm happy to defer to the relevant code owners (@christopherfujino for the tool, @goderbauer for the framework, @stuartmorgan for the packages repo) in terms of whether we should actually do this. I just ask that we be consistent throughout the repo (and dart:ui, and ideally flutter/packages) about it.

My vote would be weakly against because of the cost to line lengths and readability.

I'm relatively neutral on this; I don't love the verbosity, but I'm generally willing to trade verbosity for safety.

Do we make those mistakes in practice?

This seems like a key question. I haven't read through the 3k files changed here: @lsaudon How many instances were there (roughly) where you had to make a code change as a result of adding final to parameters?

I just ask that we be consistent throughout the repo (and dart:ui, and ideally flutter/packages) about it.

My policy for flutter/packages is that we deviate from flutter/flutter analysis options only where there is a compelling argument for why it should be different (e.g., sort_pub_dependencies is off for a specific reason in flutter/flutter that doesn't apply at all to flutter/packages, and we still have unnecessary_null_comparison: ignore in flutter/packages because packages don't all require Dart 3 so can still run in mixed mode). There's no good argument for a different choice here, so it'll stay consistent with flutter/flutter whichever way it goes.

Given the feedback, I am rather neutral on this one as well now. In the past, we have chosen a pretty verbose style for Flutter and this follows that style. We also require final for local variables, so also doing it for parameters seems consistent.

However, I can't recall any instances where this would have prevented a bug. So, if the general consensus is that it is too verbose let's not enforce it. We should update the comment in the analysis_options.yaml file to record our decision:

Maybe what we really want is a lint that you should never re-assign an argument.

That lint exists: https://dart-lang.github.io/linter/lints/parameter_assignments.html However, I believe that one is too strong because in many instances we do actually assign values to parameters on purpose.