Linter: Add lints to help users avoid common Widget anti-patterns.

[jacob314]

Motivation from a Flutter user group post: "I've been seeing this a lot with new devs. Don't needlessly use Containers. Wrapping a widget in Container and setting no other parameters does absolutely nothing but make your code more complicated."
These lints could help Flutter examples on the web be more idiomatic with less unneeded boilerplate.

Initial list of lints with obvious quick fixes we could add:

• [x] 1. Container with no parameters other than child --> Remove the container.
• [ ] 2. Center() widget as a leaf node.
• [ ] 3. Wrapping a widget in a Padding even though the widget itself has padding settings.
• [ ] 4. Using multiple widgets to do something Container can do.
• [ ] 5. Using Align when Center will do (https://github.com/dart-lang/linter/issues/2157)
• [x] 6. No state in constructor https://github.com/dart-lang/linter/issues/1865
• [ ] 7. Incomplete or missing didUpdateWidget method https://github.com/dart-lang/linter/issues/2485
• [ ] 8. Failure to manage listener lifecycles correctly https://github.com/dart-lang/linter/issues/2486
• [ ] 9. Guidance around "implicit" setState (https://github.com/flutter/devtools/pull/1336#discussion_r346598595)
• [ ] 10. Your idea here??

[pq]

These ideas are great. Do you imagine folks would want to opt in to some but not all of these advices? To what extent can/should we bundle them in a single rule? 1-3 seem like a natural bundle... 4 might deserve it's own. Thoughts on names?

Also, motivating examples (ideally from the wild) would be welcome for documentation too.

[bwilkerson]

I could imagine cases when teams might define conventions that would negate some of 1-3 but not others, so I'd leave them as separate lints.

Perhaps it's just my unfamiliarity with flutter, but I need more concrete information to understand 4.

[jacob314]

4 is the most complex one. Container is basically a de-sugaring for a number of simple combinations of nested padding, alignment, and decoration widgets. Think of it like "div" in CSS.
Its build method shows what that desugaring is.
For example

Align(
  alignment: alignment
  child : Padding(
    padding: padding,
    child: widget
  ),
)

is identical to

Container(alignment: alignment, padding: padding, child: widget)

Here is the Build method that converts the parameters of a container into the desugaring. To be safe, you could only suggest the lints if the underlying widgets are exactly in the same order they are listed in the build method.

  Widget build(BuildContext context) {
    Widget current = child;

    if (child == null && (constraints == null || !constraints.isTight)) {
      current = LimitedBox(
        maxWidth: 0.0,
        maxHeight: 0.0,
        child: ConstrainedBox(constraints: const BoxConstraints.expand()),
      );
    }

    if (alignment != null)
      current = Align(alignment: alignment, child: current);

    final EdgeInsetsGeometry effectivePadding = _paddingIncludingDecoration;
    if (effectivePadding != null)
      current = Padding(padding: effectivePadding, child: current);

    if (decoration != null)
      current = DecoratedBox(decoration: decoration, child: current);

    if (foregroundDecoration != null) {
      current = DecoratedBox(
        decoration: foregroundDecoration,
        position: DecorationPosition.foreground,
        child: current,
      );
    }

    if (constraints != null)
      current = ConstrainedBox(constraints: constraints, child: current);

    if (margin != null)
      current = Padding(padding: margin, child: current);

    if (transform != null)
      current = Transform(transform: transform, child: current);

    return current;
  }

[jacob314]

https://github.com/flutter/flutter/issues/1220
has some issues that match this list of feature request as well as some other great ideas for Flutter specific lints. We should file bugs in the linter package for each one of the unsupported issues.

[a14n]

Regarding 4 (_Using multiple widgets to do something Container can do_) there shouldn't be a lint if the expression is _const_ because Container does not have a const constructor.

[pq]

Spinning up on analyses but would appreciate input on naming. Needless to say more ideas welcome too!

1. Container with no parameters other than child => avoid_unnecessary_containers
2. Center() widget as a leaf node. => avoid_center_leaf_nodes
3. Wrapping a widget in a Padding even though the widget itself has padding settings. prefer_inset_padding (???)

[pq]

Is it idiomatic Flutter to prefer Center when using an Align that is centered? From the docs:

///  * [Center], which is the same as [Align] but with the [alignment] always
///    set to [Alignment.center].

/cc @HansMuller @daveshuckerow

[pq]

1. Center() widget as a leaf node. => avoid_center_leaf_nodes

Is this generalizable at all? For Align widgets generally? More broadly? It seems like there might be many other "unnecessary" leaf nodes that we might flag. Or?

/cc @InMatrix

[bwilkerson]

I agree: it seems like 2 and 3 could be generalized.

1. Center() widget as a leaf node.

Is there a term for widgets that control the display (or behavior) of child widgets but are not useful when there are no child widgets? Align, Padding and Transform all appear to be similar in that sense. Seems like we'd want to flag using any of these as leaves.

1. Wrapping a widget in a Padding even though the widget itself has padding settings.

Again, seems like there are lots of cases like this that might want to be caught by the same lint. For example, if there were a Center whose child was an Align (or vice versa) we'd want to flag that because the parent and child are redundant. (avoid_nesting_redundant_widgets?)

[Levi-Lesches]

3 would need a lot of special-casing, as well as Transitions. Like, should we flag an Align around an AlignTransition? Basically, you would need to consider every Widget that has a certain property. Container is an obvious one, but you would probably need to dive deeper.

[pq]

Thanks @Levi-Lesches. It would be ideal if this generalizes but it may just be a bag of special cases.

@Hixie: any thoughts?

[Levi-Lesches]

I think it's worth pointing out that even if we don't flag every single anti-pattern, special casing may be enough -- especially since these are just lints and not a part of the language itself. The most common anti-patterns I've seen are empty Containers (ie, just child) and using padding with each element of a row and/or column instead of padding the whole row/column or using Spacer or SizedBox in between the elements. That especially leads to a layout breakdown. Most extreme case I saw was someone who tested on an iPhone, and then due to this, on Android the layout fell apart (getting landscape to work was a disaster).

[pq]

@Levi-Lesches: any concrete examples you can share would be greatly appreciated. Thanks a million for the input!

[Levi-Lesches]

Sure, although this isn't the exact code. It went something like this:

@override
Widget build(BuildContext context) {
  // I think that with list comprehension, we really don't need functions like this. 
  // If you're not using a cached widget, just return a widget tree already. 
  // I would strongly advocate for that as a lint. 
  final Widget firstWidget = Padding(/* stuff */);  // this is going in a padded column
  // above, plus align shouldn't be used in a row when you can use mainAxisAlignment. See below
  final Widget secondWidget = Padding(child: Align(/* stuff */));
  final Widget simpleWidget = Container(child: Placeholder());  // no need for container
  // really should be in its own widget, but that's subjective
  final Widget veryComplicatedWidget = Placeholder();

  return Column(
    // notice how vertical padding in widgets should be replaced with SizedBox here.
    // keeps the padding simple and visible
    children: [
      Row(  
        // should use alignment parameters instead of manual aligns and pads
        children: [firstWidget, secondWidget], 
      ),
      simpleWidget,
      veryComplicatedWidget,
    ]
  );
  // again, all we did here was define widgets and return them. No await, no adding to lists, nothing.
  // easily could just be a widget tree with fat arrow. Most of Flutter team code is like this too.
}

As I said, not only is this horrible to look at, but it can actually lead to un-responsive design by misleading the developer (and especially anyone else working on the code).

[Hixie]

1 would be interesting if we solved it with an annotation in some sort of general sense — "this constructor is a no-op if called with only this argument", or "this constructor is a no-op if none of these arguments are set" or something.

2 we can solve with @required if we think it's a good idea.

3 is sketchy. Usually padding on a widget is different than padding around a widget, and we can't easily know which is right. That might be a better feature for an IDE tool like the devtools inspector, some sort of "possible problem auditor".

4 is not something I would recommend. Generally, it's an iota more efficient to use the widgets directly rather than Container, so I'm reluctant to encourage it. It's a taste thing at best.

5 we have a whole list of these in the flutter issue tracker somewhere.

[Levi-Lesches]

2 we can solve with @required if we think it's a good idea.

Just curious, how would you enforce Center as a leaf widget using required?

[jacob314]

Solving #1 with annotations sounds great. Lets get a quick design doc together on what annotation(s) we should add to handle the Widget use case.
#2
With @required you could still write
Center(child: null)
but you would be reminded not to write
Center()
which is likely the common case we are concerned about.
We could also add
assert(child != null) within the constructor of Center if you we really wanted to make the child parameter mandatory but that would be quite a breaking change.
Here are some details on the upcoming dart NNBD work.

[Levi-Lesches]

but you would be reminded not to write Center()

Wait so people are just putting blank Centers in their widget trees? That's... interesting.

Lets get a quick design doc together

I like this idea a lot. How about something like:

const Center({@noopWithOnly this.padding});

or

const Center({@noopWithout this.child});

Of course, we can change "noop" to something more beginner-friendly, like "useless" or something. But I think in the parameter declaration, like required is a nice place to put them.

[jacob314]

I've seen examples of Center() in the wild but nowhere near as many as I have seen references to Container() in the wild.
For example: there are even cases that slipped in to non-test code in the Flutter framework. Using a Container here is actually a little bit slower than the alternative due to the extra logic in the Container build method.
https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/cupertino/picker.dart#L322

[pq]

An "unneeded container" example in the wild:

https://github.com/flutter/devtools/pull/1508#discussion_r363383521

[NatoBoram]

I'd personally like to see this one : https://github.com/dart-lang/linter/issues/2410