Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it easier to add new final fields to existing constructors #53707

Open
Tracked by #55825
DanTup opened this issue Oct 9, 2023 · 7 comments
Open
Tracked by #55825

Make it easier to add new final fields to existing constructors #53707

DanTup opened this issue Oct 9, 2023 · 7 comments
Labels
analyzer-quick-fix analyzer-refactoring area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@DanTup
Copy link
Collaborator

DanTup commented Oct 9, 2023

This code:

class A {
  final String a;
  final String b;
}

produces diagnostics for field_not_initialized and has a convenient fix to add a constructor:

image

If I use that fix and later add a new field c:

class A {
  final String a;
  final String b;
  final String c;

  A({required this.a, required this.b});
}

There is no diagnostic on c, nor are there any code actions to add C to the constructor (from the declaration of c, where I was typing). Instead, there is a fix on the constructor:

image

This is less convenient because it requires moving to the constructor and there's no option to add it as a named field (you have to use "Add final field formal parameters" and then "Convert selected formal parameter(s) to named" or "Convert all formal parameters to name" (although that one doesn't move it to the end where you might prefer it)).

It would be useful if a field that is not assigned in a constructor (causing final_not_initialized_constructor) provided a fix/assist at the location of the field (with both named and positional options).

(@bwilkerson one thing I thought about here was having context messages on final_not_initialized_constructor that point at the field, and showing fixes against the locations of context messages in addition to where the diagnostic is rendered... although it may fall down when there are two constructors with this diagnostic pointing at the same field as that would result in two fixes on the field declaration)

@DanTup DanTup added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-refactoring analyzer-quick-fix labels Oct 9, 2023
@DanTup
Copy link
Collaborator Author

DanTup commented Oct 9, 2023

(unsure if this is really a request for a refactoring or a quick-fix)

@bwilkerson
Copy link
Member

It would be useful if a field that is not assigned in a constructor (causing final_not_initialized_constructor) provided a fix/assist at the location of the field (with both named and positional options).

I can see how that would be useful.

The reason we put the diagnostic on the constructors rather than the field is that every constructor needs to be looked at to decide the right way to add the initialization. I think the same reasoning applies to a fix.

Maybe it's the case in practice that either (a) there is typically a single constructor, so the options could be presented at the field declaration, or (b) one of the fixes, such as adding a parameter, is common enough that having a way to apply it to all constructors would be useful. I don't know whether either of those is the case, but we could certainly consider adding this kind of support for those special cases; I don't think we'd want to add every combination of edit x constructor.

unsure if this is really a request for a refactoring or a quick-fix

The same consideration would apply for an "add field" refactoring.

@DanTup
Copy link
Collaborator Author

DanTup commented Oct 10, 2023

or (b) one of the fixes, such as adding a parameter, is common enough that having a way to apply it to all constructors would be useful. I don't know whether either of those is the case, but we could certainly consider adding this kind of support for those special cases; I don't think we'd want to add every combination of edit x constructor.

My thinking was that the same two options from the first screenshot would be useful, and they're added to all constructors that have a diagnostic because this field is missing:

  • add as positional parameter to all constructors that have final_not_initialized_constructor because of this field
  • add as [required if non-nullable] named parameter to all constructors that have final_not_initialized_constructor because of this field

@srawlins srawlins added P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug labels Oct 17, 2023
@FMorschel
Copy link
Contributor

Just to point out the similar interest with #45821

@eernstg
Copy link
Member

eernstg commented Sep 5, 2024

Here is a thing which is somewhat relevant: If we add support for primary constructors (something like this proposal) and inferred required) then the change would be as follows:

// First version.
class const A({String a, String b});

// Second version.
class const A({String a, String b, String c});

Of course, this isn't a solution to the original request because it wouldn't do anything to fix all the other constructors (if any). However, it would make the transition cost lower in some cases that might be common.

It might even be useful to use this together with a refactoring that transforms a regular constructor to a primary constructor, or vice versa.

@FMorschel
Copy link
Contributor

I also think this could this be the same as #53301.

@FMorschel
Copy link
Contributor

Just a full review for us to reference. Coming from #54156 (comment):

I've tested some combinations of fixes.

Things I noticed:

class C {
  final int a;

  C({required this.a});
}

class D extends C {
  final int b;
}
  1. When using the fix on D the generated constructor ignores b.
    When doing this, on the generated constructor D({required super.a}); there is a fix on D with one option to add "final parameters", this is the output: D(this.b, {required super.a});
  2. When using the fix on b "required named" all works fine
  3. When using the fix on b for "final fields" (no required named) it doesn't add a.
    When doing this, on the generated constructor D(this.b); there is a fix on D there are two options:
    a. "Add super constructor invocation" which could be what you meant
    b. "Add required parameter" which produces invalid code D(, {required super.a}this.b); (latest master)

Note: If a was nullable or had a default value making it not required it doesn't show the option to fix (no need) when the constructor is created with the third option.


Some things do change with widgets:

class MyWidget extends StatelessWidget {
  final String text;

  @override
  Widget build(BuildContext context) => const SizedBox();
}
  1. If you fix with "required named" on text you get const MyWidget({super.key, required this.text}); which could trigger always_put_required_named_parameters_first if it's on.
  2. By using "add key to constructors on MyWidget the generated one is const MyWidget({super.key}); which has a new fix to "Add final formal parameters" that results finally in the same as above: const MyWidget({super.key, required this.text});.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-quick-fix analyzer-refactoring area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants