dart-lang / linter

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

avoid_unnecessary_containers does not flag `Container()` #4778

Open navaronbracke opened 1 year ago

navaronbracke commented 1 year ago

Describe the issue The lint avoid_unnecessary_containers does not flag an empty container widget.

To Reproduce

import 'package:flutter/widgets.dart';

class Foo extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Column(
      children: [
        Container(),
        const Text('Bar'),
      ],
    );
  }
}

Expected behavior The lint avoid_unnecessary_containers should flag the empty container.

TBD: if wrapping said container in a SizedBox / ConstrainedBox / DecoratedBox should be flagged, as all these force styling or min/max sizes, which can probably be avoided by rewriting code anyway, because the empty container does not add value while being the child.

Additional context

[✓] Flutter (Channel stable, 3.13.6, on macOS 13.5.2 22G91 darwin-x64, locale
    en-BE)
    • Flutter version 3.13.6 on channel stable at
      /Users/navaronbracke/Documents/flutter
    • Upstream repository git@github.com:navaronbracke/flutter.git
    • FLUTTER_GIT_URL = git@github.com:navaronbracke/flutter.git
    • Framework revision ead455963c (2 weeks ago), 2023-09-26 18:28:17 -0700
    • Engine revision a794cf2681
    • Dart version 3.1.3
    • DevTools version 2.25.0

[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
    • Android SDK at /Users/navaronbracke/Library/Android/sdk
    • Platform android-34, build-tools 34.0.0
    • ANDROID_HOME = /Users/navaronbracke/Library/Android/sdk
    • Java binary at: /Applications/Android
      Studio.app/Contents/jbr/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build
      17.0.6+0-17.0.6b802.4-9586694)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 15.0)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 15A240d
    • CocoaPods version 1.13.0

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2022.2)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build
      17.0.6+0-17.0.6b802.4-9586694)

[✓] VS Code (version 1.83.0)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.74.0

[✓] Connected device (2 available)
    • macOS (desktop) • macos  • darwin-x64     • macOS 13.5.2 22G91 darwin-x64
    • Chrome (web)    • chrome • web-javascript • Google Chrome 117.0.5938.149

[✓] Network resources
    • All expected network resources are available.

• No issues found!
srawlins commented 1 year ago

Does this sound right, @goderbauer ?

goderbauer commented 1 year ago

Good question. In the example given the container is definitely unnecessary and should be removed, but I am not sure if that's true for all empty containers. They could be used to fulfill the responsibilities of a SizedBox.shrink() or SizedBox.expand(), in which case they are not really unnecessary (although they could be replaced with something more optimal).

I get the sense that the spirit of the avoid_unnecessary_containers lint is to flag containers that can be removed without replacement. I don't think that is true for all instances of empty Container()s. Some of these may have to be replaced with a SizedBox and I am not sure if we can detect automatically detect which category a container falls into (I couldn't even describe it verbally).

With that, I am not sure if flagging all empty containers is in the spirit of this lint?

navaronbracke commented 1 year ago

That is indeed an open question I had too (hence the TBD)

I often see empty containers that don't have such a parent, though. I.e. as a child of a Column or inside a Padding, as the result of a ternary expression. (these are just two examples)

I often have to bring this up during code reviews, hence why I filed this (rather optimistic) issue.