dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
628 stars 172 forks source link

Consider whether `prefer_const_constructor` should always apply to extension types. #4780

Open lrhn opened 9 months ago

lrhn commented 9 months ago

If I have a (semi-silly) type like:

extension type const Maybe<T extends Object>._(T? _value) {
  const Maybe.yes(T value) : this._(value);
  const Maybe.no() : this._(null);  
  bool get isIt => _value != null;
  T get value => _value ?? (throw StateError('Not'));
}
void main() {
  var maybe = Maybe.no();
  if (maybe.isIt) print(maybe.value);
}

then doing Maybe.no() gives the warning

info line 8 • Use 'const' with the constructor to improve performance. (view docs) Try adding the 'const' keyword to the constructor invocation.

The issue is that it won't improve performance, so the message is misleading. We may want to recommend using const here anyway, for consistency, but using const is not (always) for performance, and (IMO) it undermines the lint's message to say so in cases where there is no performance change at all.

The argument for giving the lint anyway would be that it's non-trivial to know whether an extension type constructor has any runtime overhead. These do not. If inlined (which they should be by any semi-competent compiler), the constructors will go away entirely.

If the constructor is only slightly more complicated, that may no longer be true, and giving a warning to clients that depends on the implementation, rather than the declaration, risks giving flaky warnings that change between minor versions of the same library.

If an extension type constructor can be const, then it means that it's initializing the representation object using a potentially constant expression, which will be constant if inlined. A non-const invocation of that constructor with constant arguments, which is what is needed to trigger this lint, will most likely be creating an object which is indistinguishable from a constant object. The only difference should be whether any asserts are evaluated at compile-time or not.

Which means it should be fairly safe to not recommend const at all.

So: It's not clear whether the lint should always apply to extension types, but it's also not clear how to decide whether it should or not. And the workaround is to add const, which isn't wrong. It's just unnecessarily verbose, in a situation where the user may know that they're really just casting an existing constant to a new static type, and they don't need the result to be a constant expression.

bwilkerson commented 9 months ago

The issue is that it won't improve performance, so the message is misleading.

I don't have an answer for the issue's question either, nor do I disagree with you that the message in this case is misleading. As background information, this lint was written to help Flutter users on the rails when creating widget trees where using const does improve performance. Hence the wording of the message.

It's possible that the lint should be restricted to only flagging the use of const with constructors on subclasses of Widget, or classes marked with a preferConst annotation, or some other criteria. I suspect that extension types are not the only place where this lint flags code unnecessarily.