ugly inline

[stephane-archer]

input code:

  @override
  Future<List<String>?> getSavedLutFolders() async {
    final SharedPreferences sharedPreferences = await _sharedPreferences;
    final List<String>? bookmarks =
        sharedPreferences.getStringList(_sharedPreferencesKey);
    if (bookmarks == null) {
      return null;
    }

    List<String> result = [];
    for (var bookmark in bookmarks) {
      try {
        final path = await _bookmarkToPath(bookmark);
        result.add(path);
      } catch (e) {
        log("Error while resolving MacOS sandbox bookmark: $e");
        await _rmBadBookmark(bookmark, bookmarks, sharedPreferences);
      }
    }
    return result;
  }

  Future<void> _rmBadBookmark(
    String bookmarkToRemove,
    List<String> bookmarks,
    SharedPreferences sharedPreferences,
  ) async {
    bookmarks.remove(bookmarkToRemove);
    await sharedPreferences.setStringList(_sharedPreferencesKey, bookmarks);
  }

inline _rmBadBookmark

result code:

 @override
  Future<List<String>?> getSavedLutFolders() async {
    final SharedPreferences sharedPreferences = await _sharedPreferences;
    final List<String>? bookmarks =
        sharedPreferences.getStringList(_sharedPreferencesKey);
    if (bookmarks == null) {
      return null;
    }

    List<String> result = [];
    for (var bookmark in bookmarks) {
      try {
        final path = await _bookmarkToPath(bookmark);
        result.add(path);
      } catch (e) {
        log("Error while resolving MacOS sandbox bookmark: $e");
        await (
          String bookmarkToRemove,
          List<String> bookmarks,
          SharedPreferences sharedPreferences,
        ) async {
          bookmarks.remove(bookmarkToRemove);
          await sharedPreferences.setStringList(_sharedPreferencesKey, bookmarks);
        }(bookmark, bookmarks, sharedPreferences);
      }
    }
    return result;
  }

expected code:

  @override
  Future<List<String>?> getSavedLutFolders() async {
    final SharedPreferences sharedPreferences = await _sharedPreferences;
    final List<String>? bookmarks =
        sharedPreferences.getStringList(_sharedPreferencesKey);
    if (bookmarks == null) {
      return null;
    }

    List<String> result = [];
    for (var bookmark in bookmarks) {
      try {
        final path = await _bookmarkToPath(bookmark);
        result.add(path);
      } catch (e) {
        log("Error while resolving MacOS sandbox bookmark: $e");
          bookmarks.remove(bookmark);
          await sharedPreferences.setStringList(_sharedPreferencesKey, bookmarks);
      }
    }
    return result;
  }

[FMorschel]

This seems to only happen when you have both a Future as a return type and at the usage you are awaiting it. I'll do some more testing and see if I can solve this.

Edit: This only happens with no return, which is weirder IMO.

This is probably intended to make Future< T> when T is something being assigned, that you won't lose this behaviour. I'm not sure why this is the chosen approach since there can only be one return. I'll dig into this to see if I find who discussed this behaviour and ask so we can think of any edge cases here.

[FMorschel]

Okay, I could come up with two simple tests that represent the reason of this:

Future<void> function() async {
  await function2();
}

Future<void> function2() async {
  return Future.delayed(Duration.zero);
}

Will be refactored:

Future<void> function() async {
  await Future.delayed(Duration.zero);
}

But the problem happens if you remove the return and add no await:

Future<void> function() async {
  await function2();
}

Future<void> function2() async {
  Future.delayed(Duration.zero);  // No more `return`
}

If you refactor the same as above, the resulting code is wrong since now you are awaiting the Future.delayed you weren't (makes more difference if the Duration is meaningful, this is simply an example).

So in this case, removing the await before the call to function2 would be the path forward. I'll see if I can do something like this.

[bwilkerson]

It might be reasonable to remove the await in this particular example, but it seems like it would be hard to get this right when some of the returns are conditional, some return an explicit Future, and others return a non-Future that will be wrapped because of the async modifier.

To do this right I think we need flow analysis, which is implemented, but not exposed in a way that we can make use of it.

[FMorschel]

I've managed to reduce the fix to actually two lines of code, the rest of the refactor already handled everything. I'm not sure if this would have any other edge cases but I suspect not.

Here is the CL https://dart-review.googlesource.com/c/sdk/+/411440.

Added you as a reviewer @bwilkerson. Thanks!

[FMorschel]

Hey @bwilkerson, I have a question for you on the CL. Can you take a look, please? Thanks a lot!

[bwilkerson]

Sorry for the long delay.

The tl;dr is that async programming is hard and we think it would be hard for many users to understand what had changed if the refactoring were to make changes that also changed the semantics of the code.

In particular, we need to be careful to not remove an await, because the presence of the await is what gives other async code the opportunity to run. There are times where it would be safe to not wrap the code to be inlined in a closure, but determining whether or not it's safe requires a level of analysis that we don't currently have available to us. In order to prevent introducing subtle bugs we chose to be (overly) cautious and to always wrap the code in a closure when it might be wrong to not do so.