dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.3k stars 1.59k forks source link

update discarded_futures lint rule to not trigger when a widget expect a futur #59455

Open stephane-archer opened 7 months ago

stephane-archer commented 7 months ago

Steps to reproduce

in the following code:

  @override
  Widget build(BuildContext context) {
    LutFoldersProvider lutFoldersProvider = Provider.of<LutFoldersProvider>(
      context,
    );
    return FutureBuilder<List<DirectoryTree>>(
      future: _getDirectoryTrees(lutFoldersProvider.lutFolders),
      builder:
          (BuildContext context, AsyncSnapshot<List<DirectoryTree>> snapshot) {
        if (snapshot.hasError) {

I get the following warning:

Asynchronous function invoked in a non-'async' function.
Try converting the enclosing function to be 'async' and then 'await' the future.

but future is expecting a Future<List<DirectoryTree>>? so this warning doesn't make sense in that case. I will not await the futur. So discarded_futures lint rule should be updated to allow this use case.

Expected results

see up

Actual results

see up

Code sample

Code sample ```dart [Paste your code here] ```

Screenshots or Video

Screenshots / Video demonstration [Upload media here]

Logs

Logs ```console [Paste your logs here] ```

Flutter Doctor output

Doctor output ```console [✓] Flutter (Channel stable, 3.19.6, on macOS 13.4.1 22F770820d darwin-x64, locale en-GB) • Flutter version 3.19.6 on channel stable at /Users/fractale/flutter • Upstream repository https://github.com/flutter/flutter.git • Framework revision 54e66469a9 (3 days ago), 2024-04-17 13:08:03 -0700 • Engine revision c4cd48e186 • Dart version 3.3.4 • DevTools version 2.31.1 [!] Android toolchain - develop for Android devices (Android SDK version 34.0.0) • Android SDK at /Users/fractale/Library/Android/sdk ✗ cmdline-tools component is missing Run `path/to/sdkmanager --install "cmdline-tools;latest"` See https://developer.android.com/studio/command-line for more details. ✗ Android license status unknown. Run `flutter doctor --android-licenses` to accept the SDK licenses. See https://flutter.dev/docs/get-started/install/macos#android-setup for more details. [✓] Xcode - develop for iOS and macOS (Xcode 14.3.1) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 14E300c • CocoaPods version 1.15.2 [✓] 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.88.1) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.86.0 [✓] Connected device (2 available) • macOS (desktop) • macos • darwin-x64 • macOS 13.4.1 22F770820d darwin-x64 • Chrome (web) • chrome • web-javascript • Google Chrome 124.0.6367.61 [✓] Network resources • All expected network resources are available. ! Doctor found issues in 1 category. ```
lrhn commented 7 months ago

The discarded_future is documented as linting if an asynchronous function is called from a non async function. It's working as documented.

If you expected another behavior (likely, since the name suggests a different behavior), then this is not the lint for you. (Or the lint should be changed, but currently it's doing what it says it's doing, so now likely we want another lint.)

stephane-archer commented 7 months ago

let me rephrase. discarded_future is in the Errors lint section that indicates "Possible errors or mistakes in your code." https://dart.dev/tools/linter-rules Here my build method can't become async and I use a FutureBuilder to "await" the future. If there is a mistake in my code or something wrong I would expect a linter in the Errors section to trigger. Do I do something wrong? If it's the correct way of doing this, I would consider this a "false positive" If the "await" happens in the FutureBuilder then it should not trigger. But I'm not sure how a FutureBuilder can "await" a future without the need of forcing the caller to be async so I probably missed something.

lrhn commented 7 months ago

The discared_futures lint is opinionated. I believes you should never invoke a function returning a Future in a function that is not async. That ensures that you can await the future (and with unawaited_futures, that you do await the future).

The lint is for people who buy in to the idea that all async operations should happen in async functions, because it's safer that way.

You may not agree with that sentiment. I don't, so I don't use that lint. Others may be perfectly happy being told to add async to their function when they start doing something asynchronous inside it. It helps them, and for a lot of code development, that's all they need.

You can write perfectly good programs where you call asynchronous functions in non-async functions. Before async/await, it was all we had! But today, we do recommend using async when working with futures, and it makes some amount of sense to get warned if you are doing something asynchronous in a function that isn't async.

If you only rarely do something that triggers the lint, and want to keep that code as is, it's easy to add an // ignore at that point. You still get warned when you otherwise forget an async.

If you do it often, the lint stops being useful for you. The cost outweighs the benefits. And that's fine. Not all lints are suitable for everybody, or for all kinds of code. That's why they're optional and opt-in.

I'm not sure I would call your case it a "false positive", as much as code where the underlying assumption of the lint just doesn't apply. It's not a false positive under the assumption that all asynchronous-function-calling functions should be async. You are disagreeing with whether the lint applies to your kind of code, not with whether the lint is doing the right thing.

If you want the lint to accept your code, it's because you want a different lint. This lint is working exactly as advertised, within parameters and stated assumptions, and it's warning about exactly the kind of code that it is designed to consider a problem.

(I'm quite willing to discuss the design of the lint, how it only applies to function calls and not other ways to create a future, and whether it was a good idea to begin with, but given the specified design, I think it's working as intended here. It may just not be a lint for you. And the name is misleading.)

stephane-archer commented 7 months ago

How can I make the code provided valid and without the lint to complain (no ignore)? I'm missing something.

If it's not possible and the FutureBuilder is the "right way" to call an async function in sync code then an exception should be made.

https://dart.dev/tools/linter-rules/avoid_void_async avoid_void_async has an exception when it's valid and not considered an error. From my understanding you advocate for no exception and the lint should report the exception has an error here.

bwilkerson commented 7 months ago

I am not familiar with FutureBuilder, so I can't give you advice on how to use it. You might have more success asking this question on the Flutter discord channel (https://discord.com/channels/608014603317936148).

wildsylvan commented 3 months ago

btw

The future must have been obtained earlier, e.g. during State.initState, State.didUpdateWidget, or State.didChangeDependencies. It must not be created during the State.build or StatelessWidget.build method call when constructing the FutureBuilder FutureBuilder<T> class