Adding lints related to the primary_constructors feature

[bwilkerson]

There are several lints related to the primary_constructors feature that the Developer Experience team has implemented or are considering implementing. The ones we have already implemented were requested in order to better test the feature by maximally converting existing code to use the new feature. All of the implemented lints have fixes associated with them.

We are now considering which of these lints we will ship with the feature. One of the signals we use is the likelihood that we would include the lint in either the core, recommended, or flutter lint sets. This issue is a request for feedback on the suitability of these lints for inclusion. This isn't the only factor we'll consider, but it is an important one. And if we do decide not to ship some of the lints, we might very well ship the existing fix as an assist.

(If you would rather have separate issues for each lint, let me know. I thought it might be useful to consider them together because they are related to the same language feature.)

When considering this question, keep in mind that these lints were designed for the maximal conversion use case. Feedback of the form "we would include this if the lint were changed in this way" would be at least as valuable as a simple 'yes' or 'no'.

The lints being considered are as follows:

• emptyContainerBodies

Flags 'containers' (classes, mixins, etc.) whose body is empty (that is, where there is only non-comment whitespace between the curly braces). It recommends replacing the body with a semicolon.

• initializeInFieldDeclaration

Flags field initializers in a primary constructor body where the initializer could be removed and the right-hand side moved to the field's initializer. It recommends removing the initialization.

• unnecessaryConstInEnumConstructor

Flags primary and secondary constructors in an enum declaration that have the const modifier. Recommends removing the modifier.

• unnecessaryPrimaryConstructorBody

Flags primary constructor bodies with no initializers, an empty body, and no metadata or doc comment. It recommends removing the primary constructor body.

• unnecessaryTypeNameInConstructor

Flags secondary constructors that include the name of the enclosing container. It recommends removing the type name or replacing the type name with new, as appropriate.

• useDeclaringParameters

Flags non-declaring parameters in a primary constructor declaration that could be replaced by a declaring parameter. Recommends converting to a declaring parameter and removing the field declaration.

• usePrimaryConstructors

Flags any situation in which a secondary constructor could be converted to be a primary constructor. This includes flagging default constructors. Recommends converting the flagged constructor into a primary constructor.

[bwilkerson]

@keertip @srawlins

[jakemac53]

• usePrimaryConstructors

Flags any situation in which a secondary constructor could be converted to be a primary constructor. This includes flagging default constructors. Recommends converting the flagged constructor into a primary constructor.

This one is probably worth elaborating on and potentially splitting into several different lints.

Specifically I think there should be a lint in the recommended or core sets, which flags "trivial" secondary constructors which should definitely be primary constructors.

The guiding principle for "trivial" would be that the entire constructor can be completely moved into the class header, and you wouldn't have to still have a constructor declaration in the class body to complete it.

[srawlins]

• emptyContainerBodies - soft +1; I think I essentially never see these in the wild, so probably not big impact, but we do have plenty of lint rules around "being concise."
• initializeInFieldDeclaration - if I understand this right, +1
• unnecessaryConstInEnumConstructor - +1; be concise; don't be redundant.
• unnecessaryPrimaryConstructorBody - +1; I think this could help users who might see this code and think it does something, when it does not.
• unnecessaryTypeNameInConstructor - +1
• useDeclaringParameters - +1
• usePrimaryConstructors - complicated. 😁 I think I generally support a very broad lint rule out the gate, and we can trim the violations later if we want. If we ship a more narrow rule, it would likely be very difficult to make it more broad in the future.

Specifically I think there should be a lint in the recommended or core sets, which flags "trivial" secondary constructors which should definitely be primary constructors.

The guiding principle for "trivial" would be that the entire constructor can be completely moved into the class header, and you wouldn't have to still have a constructor declaration in the class body to complete it.

What benefit is there to making a secondary constructor like this into a primary constructor? I think a secondary constructor is much cleaner, having a separate declaration, than squeezing it between a class's name and it's extends/implements/with clauses. Unless it also had declaring parameters.

[jakemac53]

What benefit is there to making a secondary constructor like this into a primary constructor? I think a secondary constructor is much cleaner, having a separate declaration, than squeezing it between a class's name and it's extends/implements/with clauses. Unless it also had declaring parameters.

Isn't this an argument that all we should have done is add a primary keyword to constructors with the current logic for avoiding duplicating field declarations?

I certainly hear the argument, but its not what we chose, and I would personally find value in having a consistent style across projects. That consistent style would presumably have to settle on primary constructor style since secondary constructors are fundamentally more limited in functionality.

Certainly this is a large change that will take some getting used to, but if we chose as a language to design primary constructors in the way we did, then we must believe it is better? If not, then we made a mistake.

[jakemac53]

Fwiw, one advantage of primary constructors is they are SUPER easy to find. And they are arguably the most important part of most classes. No more arguing about whether constructors should go after field declarations or before, etc.

[srawlins]

Isn't this an argument that all we should have done is add a primary keyword to constructors with the current logic for avoiding duplicating field declarations?

😬

Seriously though, I don't think so. I think it's orthogonal. I think this is probably the case with most features:

• adding new syntax is a negative; it's more complexity and cognitive overhead
• enabling a new feature is a positive, and in cases where that positive (and other pros) outweigh the syntactic cost (and other cons), yay we've done well.

So for a constructor that doesn't declare fields, in my opinion, it is a negative to move it to be a primary constructor; it adds syntactic complexity with not much gain (it is easier to find; that's a win). I definitely don't think the feature would carry its weight if it didn't come with declaring parameters.

[jakemac53]

You do also avoid duplicating the class name in the secondary constructor.

Mostly though, I think having two equally supported ways of doing the same thing is a net negative, in pretty much all cases. I would like to see us push a little more strongly towards one specific style in general. People can always disable lints if they really don't like a certain style that we promote.

We could decide that constructors with no field declaring parameters should not be primary constructors, and lint for that. Or the other way around, I don't particularly care, but I don't ever want to get the code review comment "move this constructor to be a primary constructor" and then the next week have a different reviewer tell me the opposite. Especially when its something we could just enforce via lints.

[bwilkerson]

Being unfamiliar with this process, how do we move toward a decision? Or even a partial decision? If there is one or more of these lints that there's consensus about, knowing that earlier would be better than waiting until a consensus has been reached on all of them.

For example, are there additional people that need to be involved in the discussion?

[bwilkerson]

Pinging a few people in hopes of making progress.

@munificent @lrhn @eernstg

[eernstg]

I suspect that usePrimaryConstructors has a too complex target (lots of different constructor declarations might be somehow "better" if rewritten as primary constructors), but it might be useful even if it's just covering a handful of special cases.

All the other lints aimed at introducing primary constructors and/or expressing them concisely seem quite useful to me, and unlikely to have false positives.

[bwilkerson]

Thanks for the discussion.

I think the answer to my original question is that most of the proposed lints are worth having, but that we aren't going to add any of them to the 'recommended' set as part of shipping the feature. We might add some in the future, based on community usage (of the language, not the lints).

I'll leave it to the owners of this repo to decide whether to close this issue as completed or leave it open as a reminder to revisit this question at some point.