[linter] no_await_no_async

[stephane-archer]

if the last await is deleted from a function, the function should not be async

void foo(String bar) async { // should ask for deleting the async keyword
// await Future.delay(Duration(second: 2));
print(bar);
}

[scheglov]

Interesting. I hoped to use such or similar lint to automatically find places in the analyzer itself where we have async methods. But it seems that this is not trivial.

If the return type is void, then there is no issues.

But if the return type is a Future, we cannot just change the return type, because the client might pass it where a Future is required. This requires global analysis, so is not a good fit for the current linter approach, which is local, at most at library level.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/403320

[stephane-archer]

@scheglov Just complaining about the async keyword if there is no await without touching the return type (local approach) is already great in case it is too complicated to do the global analysis.

What is your current status on this issue? I see it's already in code review. What approach did you choose?

[scheglov]

My current approach is to report when a fix can be done without changing the type.

In a separate discussion @srawlins pointed out that we can also support Future<T> return types when all returns return Future, and never values. I will update my CL correspondingly.

[bwilkerson]

And my comments on the CL are to the effect that I think the lint should report in all cases, irrespective of whether we can write an automated fix for it. I can't think of any code that involves using an async modifier on a function / method whose body doesn't contain an await where removing the async (and possibly rewriting other portions of the function / method) isn't either possible or desirable. In other words, I can't think of a false positive that we need to avoid. Am I missing a use case?

[scheglov]

Rewriting

Future<int> f() async {
  return 0;
}

into

Future<int> f() {
  return Future.value(0);
}

is possible, but not desirable.

[srawlins]

Yeah I can see how replacing return 0; with return Future.value(0); is not a slam dunk. I can see how devs would prefer the former (with async), if there was no performance cost incurred by writing async. Personally, I can see myself preferring the latter, after some getting used to. But if you have a function that must return Future<int>, and the body of the function has a half dozen int literals that it returns, it would be super tedious to write Future.value(...) over and over.

[FMorschel]

I'd see that happening (the preference for 0 over Future.value(0)) most of the time with overriden methods only, if not I'd actually prefer the lint to trigger, but I think it is another use-case.

If I could have a nudge (discussed at #58862) telling me the return type could be changed I'd really like that. So that for functions that are used often that were once async for some depency that is not needed anymore can have a small performance gain (we could start with deprecation but that would be easy to do) or even for helper functions used sporadically that can easily be refactored.

This, I guess, would be another issue, I'll open it later for a future discussion if anyone likes the idea.

[scheglov]

Benchmark code

Future<int> getValueAsync() async {
  return 42;
}

Future<int> getValueFuture() {
  return Future.value(42);
}

int getValueSync() {
  return 42;
}

Future<void> runAsyncBenchmark() async {
  final timer = Stopwatch()..start();
  var acc = 0;
  for (var i = 0; i < 10000000; i++) {
    acc = (acc + await getValueAsync()) & 0xFFFFFF;
  }
  timer.stop();
  print(' Async Benchmark: ${timer.elapsedMicroseconds} us');
}

Future<void> runFutureBenchmark() async {
  final timer = Stopwatch()..start();
  var acc = 0;
  for (var i = 0; i < 10000000; i++) {
    acc = (acc + await getValueFuture()) & 0xFFFFFF;
  }
  timer.stop();
  print('Future Benchmark: ${timer.elapsedMicroseconds} us');
}

void main() async {
  // Warm-up (very basic in this example)
  print('--- warm-up');
  await runAsyncBenchmark();
  await runFutureBenchmark();

  // Run benchmarks for measurement
  print('--- benchmark');
  await runAsyncBenchmark();
  await runFutureBenchmark();
}

Results

--- warm-up
 Async Benchmark: 903326 us
Future Benchmark: 1491952 us
--- benchmark
 Async Benchmark: 881653 us
Future Benchmark: 1482963 us

It looks that async is faster than creating explicit Future.

[scheglov]

When we can change the return type, it would be really good to do so.

Benchmark code

Future<int> getValueAsync() async {
  return 42;
}

int getValueSync() {
  return 42;
}

Future<void> runAsyncBenchmark() async {
  final timer = Stopwatch()..start();
  var acc = 0;
  for (var i = 0; i < 10000000; i++) {
    acc = (acc + await getValueAsync()) & 0xFFFFFF;
  }
  timer.stop();
  print('Async Benchmark: ${timer.elapsedMicroseconds} us');
}

Future<void> runSyncBenchmark() async {
  final timer = Stopwatch()..start();
  var acc = 0;
  for (var i = 0; i < 10000000; i++) {
    acc = (acc + getValueSync()) & 0xFFFFFF;
  }
  timer.stop();
  print(' Sync Benchmark: ${timer.elapsedMicroseconds} us');
}

void main() async {
  // Warm-up (very basic in this example)
  print('--- warm-up');
  await runAsyncBenchmark();
  await runSyncBenchmark();

  // Run benchmarks for measurement
  print('--- benchmark');
  await runAsyncBenchmark();
  await runSyncBenchmark();
}

Results

--- warm-up
Async Benchmark: 895403 us
 Sync Benchmark: 3889 us
--- benchmark
Async Benchmark: 888209 us
 Sync Benchmark: 3973 us

It looks more than 100x faster for this trivial code.

[FMorschel]

I'd also like to mention dart-lang/language#870 related to the return of Futures without await. JIC anyone finds this discussion in the future.

[incendial]

https://dart-review.googlesource.com/c/sdk/+/403320

Hi @scheglov, if you don't mind, here are a few missing test cases that can help avoid some false positives:

// 1, same applies to IfElement
Future<void> function() async {
  final stream = Stream.fromIterable(['1', '2', '3', '4']);

  await for (var s in stream) {
    print(s);
  }
}

// 2, AFAIK async won't be highlighted right now, but should be
Future<String> anotherAsyncMethod() async => Future.value('value');

// 3, the body has implicit return (useFunction is copied from your test cases)
useFunction(() async {
  if (condition) return Future.value('value');
});

// 4, removing async will make the return type Future<String?>?
Future<String?> get(String? x) async => x == null ? null : Future.value(x);

[scheglov]

Thank you, @incendial.

Yes, I found (3) while working https://dart-review.googlesource.com/c/sdk/+/403082

And I forgot (1) about await for, you probably mean ForElement, not IfElement?

And we need support for ExpressionBody, so (2).

And we need to check for nullable Future expression type vs. non-nullable Future return type (4).

[scheglov]

I think we have two properties of a function here: internal and external.

1. Internal only. Whether it is possible and desirable to drop async, and keep the return type as is. This is what my CL is about. For example when the return type is Future<T> and there are return T, it is not desirable.

2. Internal and external. Whether it is possible to change the return type from Feature<T> to T. Internally requires that there are no await, and we don't return Future<T> values. Externally this requires that the value returned from the function is not used where only Future<T> is allowed; maybe as easy as: it must be await immediately. In general this requires global analysis, but can be done without it when the function is private / local.

[incendial]

And I forgot (1) about await for, you probably mean ForElement, not IfElement?

Yep, that's what I meant

[FMorschel]

I think this may be the same as #58901?

[bwilkerson]

Rewriting ... into ... is possible, but not desirable.

I think it's important to separate the question of which cases the lint should flag and the question of which fixes we should automate.

Are you suggesting that

Future<int> f() async {
  return 0;
}

shouldn't be flagged by the lint, or that the one possible fix that I mentioned shouldn't be implemented?

[scheglov]

I think for code

Future<int> f() async {
  return 0;
}

we don't want to report the lint because we don't know how f is used, maybe the result is passed to a place that wants Future<int>. So, reporting the lint for such cases will produce a lot of diagnostics that the user not necessary can do anything about.

But an Quick Assist to convert into

int f() {
  return 0;
}

would be fine, because we expect that the user knows what he is doing, and will update any follow-up errors, or revert the change.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/403320 should fix false positives and false negatives mentioned above.

[scheglov]

The CL landed.
Now we need a quick fix in DAS.