dart-lang / linter

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

proposal: `no_write_only_initialization` #3100

Open dnfield opened 2 years ago

dnfield commented 2 years ago

no_write_only_initialization

Description

A write only field may be tree-shaken away, but if it is not late its initialization code will be inlined into the constructor. This can negatively impact performance.

Kind

Performance

Good Examples

class Foo {
  // Will only run this initializer if asserts are enabled.
  late final Map<String, Object> _debugMapOfThings = {};

  void bar(String baz) {
    assert(() {
      _debugMapOfThings[baz] = ...;
      return true;
    }());
    ...
  }
}

Bad Examples

class Foo {
  // Will cause the map to be initialized whenever a new Foo is created, even if asserts are disabled.
  final Map<String, Object> _debugMapOfThings = {};

  void bar(String baz) {
    assert(() {
      _debugMapOfThings[baz] = ...;
      return true;
    }());
    ...
  }
}

Discussion

See https://github.com/flutter/flutter/pull/94955 and https://github.com/flutter/flutter/pull/94911

Discussion checklist

dnfield commented 2 years ago

It would, however, be fine for this lint to ignore something like bool _debugIsSet = true - i.e. any primitive types that are cheap to set.

Hixie commented 2 years ago

cc @a14n :-)

srawlins commented 1 year ago

Is the request here to ban any late final instance field with an initializer?

lrhn commented 1 year ago

If a field is tree-shaken completely, why is the initialization of it not also tree-shaken (if it can be proven to be side-effect free). That sounds like a compile issue, not something we should ask people to code around.

The example using debug-only code makes sense, but should the lint recognize that the field is only used from debug code, and only trigger in that case?

Because I definitely do not want people to use late for fields in general. That's a potential overhead on every field access. And if a field is otherwise tree-shakable, then it's because it's unused, and that's a different kind of warning.