Lint request : avoid_returning_widgets

[NatoBoram]

Describe the rule you'd like to see implemented

Include as much detail as you can.

This should include a description of who would use the lint (such as if it's
specific to a framework like Flutter), what conditions the rule should look
for, and what conditions the rule should ignore.

Disallow functions from returning a Widget except in a few exceptions.

• @override is used
• It's a type, not an implementation
• It's an anonymous function

There are many discussions about Functional Widgets vs Stateless Widgets and the consensus is that classes are generally better than functions. I think it would be better to get in the habit of writing widgets instead of functions early in the learning process and a linting rule can help avoid mistakes.

Alternative names : avoid_returning_widgets, avoid_functional_widgets, prefer_widget_classes.

Examples

Ideally, provide motivating examples w/ rationale.

• Small stateless widgets have a good chance of being able to use a const constructor, which would avoid unnecessary rebuilds.
• Large widgets are easier to maintain when they are in their own file rather than mixed in with another larger widget.
• Functional widgets don't show up in the Widget inspector

BAD

class Bad extends StatelessWidget {
  const Bad({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) => _getExample();

  /// Disallowed.
  Widget _getExample() => const Example();
}

class BadBuilder extends StatelessWidget {
  const BadBuilder({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) => Builder(builder: _getExample);

  /// Disallowed.
  Widget _getExample(BuildContext context) => const Example();
}

GOOD

class Good extends StatelessWidget {
  const Good({Key key}) : super(key: key);

  /// Allowed because of `@override`.
  @override
  Widget build(BuildContext context) => const Example();
}

class GoodBuilder extends StatelessWidget {
  const GoodBuilder({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) => Builder(
        builder:

            /// Allowed because it's anonymous.
            (context) => const Example(),
      );
}

/// Allowed because it's not implemented.
typedef WidgetBuilder = Widget Function(BuildContext context);

class Builder extends StatelessWidget {
  const Builder({
    Key key,
    @required this.builder,
  })  : assert(builder != null),
        super(key: key);

  /// Allowed because it's not implemented.
  final WidgetBuilder builder;

  /// Allowed because of `@override`.
  @override
  Widget build(BuildContext context) => builder(context);
}

Additional context

Add any other considerations or context here.

• Why should I ever use stateless widgets instead of functional widgets? flutter/flutter#19269
• https://flutter.dev/docs/perf/rendering/best-practices#controlling-build-cost
• feat: lint when widgets are being returned outside the build method dart-archive/linter#2582 (comment)
• https://stackoverflow.com/q/53234825/5083247
• https://youtu.be/IOyq-eTRhvo
• https://www.reddit.com/r/FlutterDev/comments/mp4xpx/comment/gu8j6cz

[incendial]

If anyone is interested to try this rule, we have it implemented in additional dart linter called Dart Code Metrics. Any feedback is welcome!

[pq]

/fyi @dnfield @goderbauer -- we talked about something similar the other day...

[dnfield]

I'm failing to see what this adds to address the concerns raised in dart-archive/linter#2582 (comment) and dart-archive/linter#2582 (comment)

IOW - there seem to be valid cases where you would return a widget from a function to make code more readable.

That said, passing around BuildContexts to other functions can introduce other issues, particularly if those functions start to think they can hold on to that context (e.g. across an async call boundary or otherwise storing it as a variable).

I'm not strongly for or against this lint.

[NatoBoram]

dart-archive/linter#2582 (comment)
I agree with @goderbauer that banning these outright is not a good idea. Certainly it's usually better to use a widget rather than a method to build widgets, but that's more of a guideline than a hard-and-fast rule. Like @jacob314 says, it might be better as a clippy-style suggestion during development ("would you like to extract this out into a widget class?") than a lint.

Both is good. Such a lint could also have that autofix on save, it would simplify creating new widgets.

dart-archive/linter#2582 (comment)
I am not sure if banning these outright is a good idea. There are legit use cases where you may want to return a widget from a non-build method, as Jacob also pointed out above. The most prominent ones are builder methods. Simple example:

import 'package:flutter/widgets.dart';

class Foo extends StatelessWidget {
  Widget _buildMyWidget(BuildContext context) { // Should not lint.
    return Container();
  }
  
  @override
  Widget build(BuildContext context) {
    return Builder(
      builder: _buildMyWidget,
    );
  }
}

Not sure how one would differentiate this from other methods?

The point of this lint is to stop doing that and make new widgets.

---

The Flutter team has officially taken a stance in this.

https://youtu.be/IOyq-eTRhvo

[dleurs]

Any update in this request ?

dcm is not free anymore

[srawlins]

There are no plans to implement this linter rule. @dnfield voiced his opinion above and no one else on Flutter has voiced strong support.

[pq]

I see the value here but have to defer to flutter folks to get a bit more momentum.

@goderbauer?

[NatoBoram]

Every concern has been addressed years ago, too. Not only that, but there's now an official stance from the Flutter team itself even if individual members don't show up. I don't think there's any reason left to hold back.

[srawlins]

Can you summarize the stance from the Flutter team? I apologize, but I'm not going to scour that video for what you are referring to.

[dleurs]

@srawlins Flutter in recommand creating Statefull / Stateless widget instead of functions that returns a Widget

[srawlins]

A recommendation (without knowing any more about it) is very different from a rigorous rule. In order for a rule to be high value (which may lead it to be high priority), it needs to have a very low false positive rate.

If members of the Flutter framework team also would recommend that such a rule be added to the flutter_lints package, meaning it would be of high value, and have very low false positive rates, and the recommendation is very strong, like a developer would have to be in an extreme scenario in order to ever have a function return a Widget, then it could be considered high priority.

Even then, it may not be implemented for many months, as it will be weighed against many other priorities.

[dleurs]

It is quite a serious recommendation, even backed by Remy Rousselet 2 years ago

"
The definitive answer is to use classes over functions.
That is what the Flutter team and most experts will recommend.
"

I think this request fits well inside an optionnal linter rule. Most linter rules are not following rigorous rule, and I think this request improve perfomances significantly

[NatoBoram]

Can you summarize the stance from the Flutter team? I apologize, but I'm not going to scour that video for what you are referring to.

Here's a transcript for people in a hurry.

First and foremost, remember that when setState is called within a widget, its entire build method is rerun. This means that if a user toggles a favorite icon in the corner of a large piece of your UI, having rendered that icon in a helper method will require Flutter to rebuild the entire wrapping widget.

You may have heard that there are multiple trees that Flutter uses to build your UIs, and the widget tree is only the top one. Creating a few more widgets in that top tree is hardly ever going to slow down your app, but unnecessarily rebuilding whole sections of your UI can cause those deeper trees: the element tree and the render object tree to waste precious CPU time. And what's worse, consider what happens if you animate your icon between states. Now your app is unnecessarily redrawing expensive UI chunks 60 times per second for the duration of the animation, when all it had to do was animate the pixels inside the icon.

So, instead of refactoring that favorite icon into a helper method, use an entirely new widget up front, so that when it toggles states, Flutter is able to precisely target what it re-renders. And for truly best results, use const constructors wherever possible as that allows Flutter to short-circuit the most amount of unnecessary work.

Congrats! Now your app is running faster and everyone's happy.

It's time to write some tests for your UI. So you open up that widgets file again to remind yourself what to capture in a widget test. One obvious candidate is that the favorite icon we've been talking about: changes color as expected.

Which piece of code would you rather write a test for? The helper method version? Or the separate widget version? The separate widget version couldn't be simpler, whereas the helper method version requires you to reconstruct every dependency needed by _expensiveWidget1(), and _expensiveWidget2(), and they were expensive, so it's probably a lot. As if that's not enough, there are other scenarios where you can accidentally hold on to a stale BuildContext.

Consider this code which uses a Builder, but accidentally uses a different name for one of the builtContexts, allowing further nested code to use an old, unreliable builtContexts. Refactoring this into a separate widget can make the bug impossible. Now our MyIconButton only has access to one BuiltContext, and it's always the correct one. Yes, it's true that creating new widgets severs that free connection back to all of the available attributes.

Except, now that we've explored it more, it never really was free, was it? It always came at the cost of performance, testability and occasionally, even accuracy. When I was speaking about this issue with Remi Rousselet, the creator of Provider, Riverpod, Freezed, and many other packages, he summarized a situation like this:

"Classes have a better default behavior. the only benefit of methods is having to write a tiny bit less code. There's no functional benefit."

Thanks Remi, for helping draw attention to this issue with some instructive Dart Pads.

And that's it, folks. Next time you're breaking up an unwieldy build method, don't be afraid of specialized widgets. You pressing a few more keys during development, might just save your users dropped frames during runtime.

And here's that quote from Remi

The definitive answer is to use classes over functions.
That is what the Flutter team and most experts will recommend.

The problem is that sometimes articles are too old or authors were lazy/unaware of the topic.

If you see an official example using methods over classes, you can safely refactor it to classes. They've accepted such PRs on numerous occasions.

[goderbauer]

What Dan said in #58303 and what is said in the comments linked from there is still accurate. While in general it is a good idea to prefer widget classes over functions it is not a hard rule. In fact, there are many cases where one has no other choice, but to define a function that returns a widget (for example to be passed to Builder.builder or Builder-like widgets). There is nothing wrong with that and that is by design.

I don't see us adding this lint to flutter_lints or recommending it in any other way.

[NatoBoram]

In fact, there are many cases where one has no other choice, but to define a function that returns a widget (for example to be passed to Builder.builder or Builder-like widgets).

Can you find other cases than the ones noted in the proposal?

Disallow functions from returning a Widget except in a few exceptions.

• @override is used // <- Builder-like widgets
• It's a type, not an implementation // <- Builder-like widgets
• It's an anonymous function // <- to be passed to Builder.builder

There's also code examples in the proposal that directly addresses this.

BAD

class BadBuilder extends StatelessWidget {
  const BadBuilder({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) => Builder(builder: _getExample);

  /// Disallowed.
  Widget _getExample(BuildContext context) => const Example();
}

GOOD

class Good extends StatelessWidget {
  const Good({Key key}) : super(key: key);

  /// Allowed because of `@override`.
  @override
  Widget build(BuildContext context) => const Example();
}

class GoodBuilder extends StatelessWidget {
  const GoodBuilder({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) => Builder(
        builder:

            /// Allowed because it's anonymous.
            (context) => const Example(),
      );
}

[JaredEzz]

+1

I'd like to be able to add this to my analysis_options.dart to get warnings when returning Widgets from methods because it interrupts the build() chain causing problems with Bloc state management. It would be helpful for newer members of my team who are still unfamiliar with some Flutter conventions.