8000 ugly inline · Issue #60169 · dart-lang/sdk · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ugly inline #60169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
stephane-archer opened this issue Feb 19, 2025 · 6 comments
Open

ugly inline #60169

stephane-archer opened this issue Feb 19, 2025 · 6 comments
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-refactoring 8000 Issues with analysis server refactorings P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@stephane-archer
Copy link

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;
  }
@stephane-archer stephane-archer added the legacy-area-analyzer Use area-devexp instead. label Feb 19, 2025
@johnniwinther johnniwinther added area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. and removed legacy-area-analyzer Use area-devexp instead. labels Feb 19, 2025
@keertip keertip added devexp-refactoring Issues with analysis server refactorings type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Feb 19, 2025
@FMorschel
Copy link
Contributor
FMorschel commented Feb 20, 2025 •
edited
Loading

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
Copy link
Contributor
FMorschel commented Feb 20, 2025 •
edited
Loading

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
Copy link
Member

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
Copy link
Contributor

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!

@bwilkerson bwilkerson added P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug and removed type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Feb 24, 2025
@FMorschel
Copy link
Contributor

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

@bwilkerson
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-refactoring Issues with analysis server refactorings P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@keertip @stephane-archer @bwilkerson @johnniwinther @FMorschel
0