Skip to content

Decide what advice and hints/lints we want around late final fields #1239

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

Open
natebosch opened this issue Sep 25, 2020 · 10 comments
Open

Decide what advice and hints/lints we want around late final fields #1239

natebosch opened this issue Sep 25, 2020 · 10 comments

Comments

@natebosch
Copy link
Member

As far as I can tell a late final field with no initializer is not something anyone should ever write, but it feels very natural for some migrations where fields are initialized in a constructor. The better migration is generally to use late final with an initializer (though there are some caveats there as well if it matters when the initializing expression is called).

If we think there should be no false positives we should write a hint for this. If we think there do exist valid use cases for this we could make it a lint. Either way we should mention this in the migration guide. cc @munificent

See, for example, dart-archive/async#139

@leafpetersen
Copy link
Member

The better migration is generally to use late final with an initializer

The point of the feature is that this is sometimes not possible.

@leafpetersen
Copy link
Member

If we think there should be no false positives we should write a hint for this.

There are 100% false positives, in the sense of code for which the only valid alternative is to use a non-final late field (or a normal nullable non-final field). If we really feel that we just don't want to support this, we should just remove the feature.

@natebosch
Copy link
Member Author

natebosch commented Sep 25, 2020

The point of the feature is that this is sometimes not possible.

My expectation is that the "obvious" migration from fields that are set in a constructor body will be late final, but still set in the constructor body. When I say "generally" I mean that most of the time people reach for this pattern, they can probably use late final with an initializer instead of setting it in the constructor body.

There are 100% false positives, in the sense of code for which the only valid alternative is to use a non-final late field (or a normal nullable non-final field). If we really feel that we just don't want to support this, we should just remove the feature.

I would like to explore when this might come up, and whether there are other better patterns for it. Introducing an unusable setter is probably not worth it compared to the old patterns of private field, public getter. I'm aware of at least one place we used this incorrectly, I'm not yet aware of any places where public late final field with no initializer was good fit for a problem.

@bwilkerson
Copy link
Member

... most of the time people reach for this pattern, they can probably use late final with an initializer instead of setting it in the constructor body.

What pattern(s) would we look for after the fact to know that late final is being used incorrectly? (I'm basically trying to get enough information to be able to assess how many false positives there are likely to be.)

@natebosch
Copy link
Member Author

natebosch commented Sep 25, 2020

What pattern(s) would we look for after the fact to know that late final is being used incorrectly? (I'm basically trying to get enough information to be able to assess how many false positives there are likely to be.)

After some more discussion I don't think we can tell for sure if it was intentional or a mistake. People do write code with a private field, public getter, and public setter that throws if it is called more than once.

If we were to add a hint about public late final fields, or even disallow them entirely at the language level, there would still be a workaround.

late final Foo _foo;
Foo get foo => _foo;
set foo(Foo foo) => _foo = foo;

The question is whether we think this pattern is unusual enough, and the mistake is likely enough, that it's worth disallowing it and forcing this extra boilerplate. We already have some boilerplate if you want to use the feature without the public setter:

late final Foo _foo;
Foo get foo => _foo;

My opinion is that this is enough of a footgun to just disallow it and impose the boilerplate for the usual cases too.

@natebosch
Copy link
Member Author

natebosch commented Sep 25, 2020

One thing that could help here, at least some places where this is done could be avoiding if we have #622

That is the case with dart-archive/collection#156

@nielsenko
Copy link

Have you considered making the setter of a public late final field without initializer private? That way the developer adding the field has more control over its usage.

@munificent
Copy link
Member

Have you considered making the setter of a public late final field without initializer private? That way the developer adding the field has more control over its usage.

We have somewhat, yes. So far, the general feeling on the language team is that automatically adding or removing _ to an identifier (in this case implicitly adding _ for the setter) is a little too unintuitive and magical, so we've avoided going down this path.

@nielsenko
Copy link

You could argue that it is also a bit "magical" that a final field has a public setter. I think, I prefer the magical "_". Have you done any experiments to see how much code would break?

@Levi-Lesches
Copy link

@nielsenko, yep, this strange behavior of public final fields having setters is a bit of a known problem. See the issue I opened, #1390, where it seems to have been decided that a lint rule should be enough to catch certain error-prone cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants