Linter: meta issue tracking lints with issues when using optional new/const

[devoncarew]

Follow up from https://github.com/flutter/flutter/issues/17429; we need to ensure that the lints (esp. the ones in use by Flutter) work when using optional new and const. I think this means:

• [ ] removing new and const from the flutter codebase (@munificent has a handy version of dart_style that does this)
• [ ] running flutter analyze --flutter-repo and identifying problematic lints (for example, Prefer const with constant constructors)
• [ ] addressing the problematic lints (and getting the optional new/const version of them under test)

@pq, can you take this on?

• [ ] prefer_const_literals_to_create_immutables
• [x] prefer_const_constructors #995

[a14n]

Removing optional new/const on flutter examples I only see issues with prefer_const_literals_to_create_immutables and prefer_const_constructors.

[a14n]

Related to #995

[pq]

Meta-meta question: do we even want these lints in the context of Dart2? I'm of two minds. In particular, I worry that their behavior when correct might nonetheless be surprising. As a general rule, I think the semantics of lints should be straight-forward (and not a place for nuance). But that's just me...

@hixie: care to chime in? Starting w/ Dart2, how do you feel about the value of these lints?

[Hixie]

For Flutter I'm hoping we can have the following lints:

• avoid using "new"
• avoid using redundant "const"
• use "const" if the entire subexpression could be const

[Hixie]

and i would enable the last one for users by default

[bwilkerson]

I worry that their behavior when correct might nonetheless be surprising.

That's a valid concern. The question is whether we can remove the surprise factor by the way the lint is reported (the message). Can we, for example, tell users something like

The keyword const is not needed because \

And would that be enough to help users understand why the lint was produced?

[pq]

A message like:

The keyword const is not needed because \

feels like a big step in the right direction. Great suggestion.

In general my concern is just that with new/const (as w/ types in the face of inference), it's often not obvious to a reader whether an explicit new/const is gratuitous or actually communicating something. But this is a meta (if not meta-meta) point.

Back to this point and @Hixie's asks, I wonder whether it makes sense to muscle these existing lints into compliance or to just deprecate them and start fresh.

[Hixie]

Incidentally, one thing I dislike about the current "hey, you should use const" lint, is that in a situation like:

new Foo(new Bar(new Baz()))

...it'll first tell me to make Baz const, then if I run it again tell me to make Bar const, then if I run it again tell me to make Foo const. I wish it would just immediately tell me to make Foo const. Hence the phrasing of my suggestion above ("use 'const' if the entire subexpression could be const").

[a14n]

For Flutter I'm hoping we can have the following lints:

• avoid using "new"
• avoid using redundant "const"

I started with a single lint avoid_keyword_to_create_instances in #936 when there were a notion of _magic const_ . If you all think 2 lints are better I have no problem with that. Just let me know and I will split the rule.

Regarding the concerns about incremental iterations for new Foo(new Bar(new Baz())) there will be less noice with the lint to remove new/const. To fix that we would have to merge 2 lints (prefer_const_literals_to_create_immutables and prefer_const_constructors). However it's just a UX improvment we could track in an other issue.

[Hixie]

I don't mind if it's two, three, or four lints. I would like the "use const" lint(s) to be separately enabled from the "skip these keywords" lint(s), because not everyone wants to skip the keywords, but everyone should want to use const where possible as that becomes a perf issue.

[munificent]

For Flutter I'm hoping we can have the following lints:

avoid using "new"
avoid using redundant "const"

+1. As soon as we're ready for users to start relying on optional new/const, these two rules are exactly what "Effective Dart" will say.

use "const" if the entire subexpression could be const

I believe this one is a little trickier because there are some weird corner cases, like:

1. If a constructor could possibly throw, then changing it to const changes the semantics of the program.

2. If you do rely on identity and want a unique instance, then changing it to const changes the behavior.

I think (1) is why the "auto-const" part of optional new/const got cut for Dart 2, though there is probably some more subtlety than what I'm describing here.

it's often not obvious to a reader whether an explicit new/const is gratuitous or actually communicating something.

Explicit new never communicates anything as far as the language semantics are concerned. A program with all new removed behaves identically to the original program. It might make a difference to the human reader, but I think most of us assume it's not a useful thing to make visible in the program. "Effective Dart" will tell people to drop all the new keywords from their code.

[Hixie]

want a unique instance

For cases where that makes sense, in Flutter we'll probably provide an explicitly non-const constructor.

[munificent]

Yeah, in practice it's unlikely to be a problem (though new Object() is sitting there as a pathological example), but it could be, which makes it hard for a linter to not have false positives.

[a14n]

An other snippet in #1013.

All the issues look related to ConstantVerifier (that is used in the 2 problematic lints). @bwilkerson identified the issue but I don't think he had time to work on a fix.

[zoechi]

where Border.all is a factory constructor with a body. Is this also related to ConstantVerifier?

[pq]

@zoechi: I think so? @bwilkerson: can you confirm?

[bwilkerson]

Is this also related to ConstantVerifier?

I'm not at a place where I can confirm it, but it seems very likely.

[pq]

@a14n are we good w/ prefer_const_literals_to_create_immutables now or is there more still to do?

[a14n]

Trying on Flutter codebase I discover a bug in linter fixed by #1045

[a14n]

There's also dart-lang/sdk#33647 that need to be fix (but there only one case in Flutter and a workaround is to keep new).

AFAICT those are the only remaining issues.

[dgrove]

Is there anything remaining here?

[a14n]

Is there anything remaining here?

Nothing I am aware of. So this issue can be closed imo.