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.28k stars 1.58k forks source link

Implement PREFER async/await over using raw futures. #57495

Open JPaulsen opened 7 years ago

JPaulsen commented 7 years ago

From [Effective Dart] (https://www.dartlang.org/guides/language/effective-dart/usage#prefer-asyncawait-over-using-raw-futures)

JPaulsen commented 7 years ago

I am not sure about how to implement this rule, by now I am linting every FunctionExpression that doesn't have a keyword (async, sync, etc) and every ReturnStament inside (at least 1) is a then, onError, timeout or whenComplete method invocation of a target that is a Future.

These are my tests by now:

// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

// test w/ `pub run test -N prefer_async_await_over_raw_futures`

import 'dart:async';

Future<bool> bad1() { // LINT
  return longRunningCalculation().then((result) {
    return verifyResult(result.summary);
  }).catchError((e) {
    return new Future.value(false);
  });
}

Future<bool> bad2() { // LINT
  return longRunningCalculation().then((result) {
    return verifyResult(result.summary);
  });
}

Future bad3() { // LINT
  return longRunningCalculation().whenComplete(() {
    return verifyResult(null);
  });
}

Future bad4() { // LINT
  return longRunningCalculation().timeout(null, onTimeout: () {
    return verifyResult(null);
  });
}

Future<bool> verifyResult(summary) => null;

Future longRunningCalculation() => null;

Is this correct @bwilkerson?

bwilkerson commented 7 years ago

I haven't thought about this enough to be convinced that I know the right answer here, but I'm fairly sure you can't turn an invocation of timeout into an async/await form, so invocations of that method should disable the lint.

As for the rest, some questions to consider:

But in general, I think that for a lint like this it's perfectly reasonable to start with a narrow definition that covers the most common cases and to expand the definition later based on user feedback. So I'm fine with most of what you said, though I'd probably require that the return type is already expected to be either Future or dynamic (not specified).

If you're going to tackle closures then the expected return type is a little harder to compute because it depends on context. If the closure is an argument to a function, then we'd need to look at the type of the parameter. If it's being assigned to a variable we'd need to look at the variable's type. It might be better to not handle closures for the first pass.