dart-lang / linter

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

Lint if debug fields are used outside of a class annotated as debug #1802

Open jacob314 opened 4 years ago

jacob314 commented 4 years ago

Describe the rule you'd like to see implemented

Lint that warns if debug only members/classes as defined by members/classes that start with the name debug or are annotated with @debug are used outside of a debug build context.

Debug contexts are contexts marked as @debug or the body of an assert

Examples

class Element {
  Element debugCreationContext;
  Element debugNextElementSameLocation;
  Element debugNextElementSameLocation;
  int someRealField;
  void attachElement() {
    WidgetInspector.instance.attachElement(this); // Error as this is not a debug context.
    assert(() {
      WidgetInspector.instance.attachElement(this); // Fine as this is a debug context.
    }
  }
}

@debug
class WidgetInspector {
}

Additional context This will allow the Flutter Framework to eliminate the long tail of existing issues where debug fields and objects are accessed in non-debug contexts. This should later allow enhanced AOT treeshaking or a custom kernel transformer to eliminate all definitions of debug fields and classes from AOT builds. This will reduce Flutter code size potentially significantly as there are debug only fields on classes such as Element in package:flutter (_debugLifecycleState)

jacob314 commented 4 years ago

@goderbauer This is the lint we were just talking about.

jacob314 commented 4 years ago

@pq another interesting Flutter lint. This time a lint that could help reduce code size and memory usage of Flutter.

jacob314 commented 4 years ago

A few more examples of debug only fields from RenderObject. Would be nice to ensure these are never used outside of debug code so they can be tree-shaken including removing the fields. These fields show 3 wasted slots on each RenderObject.

  /// Whether [performResize] for this render object is currently running.
  ///
  /// Only valid when asserts are enabled. In release builds, always returns
  /// false.
  bool get debugDoingThisResize => _debugDoingThisResize;
  bool _debugDoingThisResize = false;

  /// Whether [performLayout] for this render object is currently running.
  ///
  /// Only valid when asserts are enabled. In release builds, always returns
  /// false.
  bool get debugDoingThisLayout => _debugDoingThisLayout;
  bool _debugDoingThisLayout = false;

  /// The render object that is actively computing layout.
  ///
  /// Only valid when asserts are enabled. In release builds, always returns
  /// null.
  static RenderObject get debugActiveLayout => _debugActiveLayout;
  static RenderObject _debugActiveLayout;
pq commented 4 years ago

@jacob314: do we imagine this being generally useful (to, e.g., library authors) or more specifically about the flutter framework?

jacob314 commented 4 years ago

Anyone using the flutter framework should also have these lints on. Using the debug methods in classes in users code is equally bad. It can result in code that works in debug and fails in release as well as having code size and memory usage issues.

rrousselGit commented 4 years ago

As a package author, I'd want this too. I use the same convention as Flutter for debug-only variables and have quite a few of these.

jacob314 commented 4 years ago

Fyi @ferhatb this could also be relevant for code size compiling with dart2js.

bwilkerson commented 4 years ago

I'm guessing that the @debug annotation is defined in the flutter package. Should we consider moving it into the meta package, or is this only usable with Flutter?

goderbauer commented 4 years ago

We currently don’t have a debug annotation in flutter. It’s just a convention for fields/methods whose name start with “debug” or “_debug”.

bwilkerson commented 4 years ago

Ok, thanks. The OP said "or are annotated with @debug", so I assumed there was an annotation.

If it would be useful, we could potentially add an annotation rather than relying on a naming convention. Not sure it's worthwhile.