dart-lang / language

Design of the Dart language
Other
2.68k stars 205 forks source link

async by default #2026

Open bsutton opened 2 years ago

bsutton commented 2 years ago

So here is a crazy idea and not being a language expert I have no idea if it is feasible but anyway....

Having to add async and await almost everywhere in your code feels like unnecessary boilerplate.

Like nnbd I'm wondering if it would be possible to change the language to 'async by default'.

Essentially I see two parts to this.

A method that returns a Future is always async. In an async function any code that returns a future is implicitly awaited.

So the following code:

 Future<void> close() async {
    await Hive.close();
  }

becomes:

 Future<void> close()  {
     Hive.close();
  }

If I want to call an async function but not await it then I would write:

 Future<void> close()  {
     unawait Hive.close();
  }

Any code that returns a future in a non-async would generate a lint warning.

 void close()  {
     Hive.closeReturnsFuture(); // generates an error.
  }

My impression is that these changes would eliminate a massive amount of boiler plate and would be fairly easy to create a migration tool for.

mraleph commented 2 years ago

FWIW this is close to how Kotlin implements this with "suspending functions". You need to annotate the function as suspending, but then internal "awaits" on calling other suspending functions are implicit.

Overall it's a neat design (which only works in statically typed languages - which Dart 2 is, but Dart 1 was not), but it's also a massive breaking change today - probably requires rolling it out using yet another modifier on a function rather than introducing this behaviour into normal async functions.

TBH, I think if we go that route we should probably instead consider introducing fibers instead.

I could not find another issue which would discuss type driven implicit await, but I am pretty sure this was discussed multiple times. /cc @lrhn @munificent

lrhn commented 2 years ago

The proposal is that inside an "async" function, any expression with a type of Future<X> is automatically awaited and effectively has static type X. You have to use a new language feature to avoid this, unawait expr.

That's definitely possible. It may complicating type inference. If you write int x = expression;, should the context type of expression now be FutureOr<int>. If so, that'll be complicating type inference (we currently have that behavior at return statements in async functions, and it is actually a problem). If not, we'll do inference with int as type hint, then accept Future<int> as well and insert the await. That makes type inference less useful. Take List<double> l = [Future.value(1)];. With an await before the Future.value, the 1 would have a context type of double and the code works. Without that future element type, which it won't have if the context type is a plain double, the code fails. However, with a context type of FutureOr<double>, there will also be code which reifies that context type where it should have used double. Because of the way context type is pushed down and used as first priority for inferring type arguments, Dart is very sensitive to the context type used for an expression, and using FutureOr does makes some code worse.

What about Future<Future<int>>, should we accept that and do two awaits?

It's ... complicated. Introducing a new kind of async function which does the automatic coercion, and not doing it in existing async functions, will probably lessen the potential break, but it will also complicate the language, and at a point where the language is already incomprehensible to a lot of people.

(I'd actually rather do int foo() async! { ... } for a new kind of async function, than I'd auto-detect Future<int> foo() { ... } as meaning async. There are functions which are asynchronous, but not async, and they are important for code implementing low-level asynchronous functionality.)

I'm also not sure I'm sold on the idea from a readability point of view.

It's very important for state management to be aware where Dart code may allow interleaving of other code. I guess Kotlin has full concurrency, so they have to assume worst-case all the time, but Dart authors are used to single-threaded execution, and an await is a place where arbitrary other code may run. So, I'm not convinced that removing the await will make the code easier to read, since you'd no longer have any visible distinction between a normal function call and an asynchronous function call.

I don't think having a second kind of async function is better than having just one. Forgetting an await is rarely a problem outside of expression statements, and with the lint, I think those are well covered. Not wanting to write the await is also taking it away from those reading the code.

Levi-Lesches commented 2 years ago

I think there's value in manually writing out await. In Flutter, there are places where you want to wait for computations to complete, and others where you want to load the UI first. If you forget it, there's a lint, and if you don't need one, you can ignore the lint. It's probably not worth it to go from sync-by-default to async-by-default just because of how much code will need to be changed.

As for writing out async, I agree that having a return type of Future and an await in the function body seems to always lead to async in the declaration. But it's still nice to see it there, and it seems that omitting it leads to some typing issues. So this feels like it should be an IDE auto-complete. Typing out Future<Foo> functionName(Foo param) would suggest async {. I don't really think this needs to be a language feature.

bsutton commented 2 years ago

@irhn The intent is to not change the semantics of the code. I see this as akin to removing the 'new' keyword.

As such this shouldn't cause a change in the type inference.

For the method signature, having Future and async is clearly redundant so this one seems like a bit of a no-brainer.

For awaits in the body, the logic would be that we are only applying a single await.

If you have Future<Future<>> then like now you need to have additional awaits.

So :

     await (await getFutureFuture());

becomes

   await getFutureFuture();

A clear problem here is during the migration when people have a mix of code where they have forgotten the await in some instances and remembered in others.

@Levi-Lesches The reality is that in most cases you want to await the code and not awaiting it is actually a problem. In the few cases that you actually want to not wait then the 'unawait' keyword that I'm proposing is the solution. People tend to get away with not awaiting calls in flutter but in cli/server apps this is a real problem. Either way not awaiting/unawait your methods is bad practice as your code becomes indeterminate and static analysis is difficult as the reader can only guess at your intent - did you mean to leave the code unawaited or did you just forget.

lrhn commented 2 years ago

As such this shouldn't cause a change in the type inference.

It has to. If we insert the await on any expression typed as Future<T> for some T, then we need to do type inference on that expression before knowing whether to insert the await.

Take C v = await e; vs C v = e;.

Currently, with the explicit await and a context type of the await expression of C, the awaited expression, e, is type-inferred with a context type of FutureOr<C>.

Without the explicit await, we don't know that an await might be inserted. We certainly won't change the context type of every expression to FutureOr just because it might become a future, so without the await, the context type of the expression e is definitely going to be C.

That is an unavoidable difference. There are expressions where it changes the type to change the context type, especially since we prefer using the context type for instantiation.

T id<T>(T value) {
  print("T is $T");
  return value;
}
void main() async {
  int v = await id(Future<int>.value(1)); // T is FutureOr<int>
  int w = id(Future<int>.value(1)); // The argument type 'Future<int>' can't be assigned to the parameter type 'int'.
}

A workaround could be writing the await explicitly anyway, and then we won't insert one. (But that doesn't mesh with needing only one await for Future<Future<...>>).

munificent commented 2 years ago

Given that Dart does not do specializing when compiling code that uses generics (which is an architectural choice that affects many many aspects of the language and platform), I don't think this is technically feasible. Consider:

void foo<T>(T callback()) {
  print('before');
  var result = callback();
  print('after');
}

Should this be compiled as:

void foo<T>(T callback()) {
  print('before');
  callback().then((_result) {
    print('after');
  });
}

(In other words, insert the implicit await and then desugar that to the continuation-passing style it represents.)

Or:

void foo<T>(T callback()) {
  print('before');
  var result = callback();
  print('after');
}

(In other words, compile it like synchronous code.)

If we always compile every use of a generic parameter as if it might be asynchronous, there will be a performance hit and a user-visible behavior change—stuff may get suspended and interleaved that they expect to run synchronously.

If we always compile it synchronously, it won't insert the implicit await when T is instantiated with a Future type.

We can't compile it both ways as needed based on the instantiated type argument, because Dart doesn't do generic specialization like that.

Levi-Lesches commented 2 years ago

@bsutton:

Either way not awaiting/unawait your methods is bad practice as your code becomes indeterminate and static analysis is difficult as the reader can only guess at your intent - did you mean to leave the code unawaited or did you just forget.

Not awaiting code isn't bad practice -- Flutter apps are practically built on futures and streams to sync data to the UI while avoiding stutter. And even if you don't see it directly, Flutter uses the event loop under-the-hood to make sure UI is always rendering while other, less important stuff happens later. In an environment where this is expected, like State.initState, not explicitly documenting this isn't necessarily bad practice because it's expected. That being said, I do use the unawaited_futures lint with an // ignore comment to make sure I really mean to leave the future unawaited.

The reality is that in most cases you want to await the code and not awaiting it is actually a problem. In the few cases that you actually want to not wait then the 'unawait' keyword that I'm proposing is the solution.

The point of my comment was that since leaving a future unawaited is perfectly valid in a lot of cases, and we already have a lint warning users not to forget to await their futures, I don't think it's worth the effort to change the default from synchronous vs asynchronous. It looks like a lot of work on the Dart team's side, and then we'd have to go check every single future in our code to see if we need to change it -- since now we'd need to remove the await keyword to avoid type ambiguities, and deliberately unawaited futures will now be automatically awaited.

bsutton commented 2 years ago

@munificent

I hadn't previously understood this callback scenario but I looks problematic from a greater language perspective as it is.

I've written a little test script:

void main(List<String> arguments) async {
  foo(callme);
  foo(callme2);
}

Future<void> foo<T>(T Function() callback) async {
  print('before');
  await callback(); // error
  print('after');
}

Future<int> callme() async {
  print('1');
  return 1;
}

int callme2() {
  print('2');
  return 2;
}

T callme3<T>(T a) {
  print(a);
  return a;
}

The attempt at making an async call to a typed callback in the foo method generates an error:

'await' applied to 'T', which is not a 'Future'

So this suggest that Dart always treats these methods as sync calls. This doesn't make much sense to me, what code is generated in these scenarios? The call is still run async but you are saying the compiler doesn't know its an async call?

Does this not mean that the problem goes away? For a typed callback we have to always treat them as sync.

This also relates to a broader issue:

With the current implementation if you create a sync method the linter fails to warn the user that they may be calling an async method without awaiting it. I've actually previously raised a feature request for a new lint to handle this:

void foo<>() {
  print('before');
  copyImportantStuffAsync();
  deleteWhatRemains();
  print('after');
}

This pattern is a major source of bugs in the cli programming I do. In the above scenario if the delete runs before the copy, data is lost.

I should also note that I've essentially requested two separate enhancements.

I've not seen any arguments against dropping the 'async' from the method signature when the return type is Future. A typed return type looks to have the same existing problems as the callback but I don't think that impinges on out ability to remove the redundant 'async' statement.

bsutton commented 2 years ago

@Levi-Lesches

Not awaiting code isn't bad practice -- Flutter apps are practically built on futures and streams to sync data to the UI while avoiding stutter.

Using async code isn't bad practice and as you note flutter is built on async code. The bad practice is not making this explicit in your code. Failing to wait/unawait code leaves maintainers guessing as to what the intent is.

Did the original developer intend that the call be unawaited or did they just forget?

As to existing awaits the intent would be for a migration tool to remove these. The migration tool should probably enable the await/unawait lints and require the user to clean these up before the migration starts.

I do realise that changing to await by default would be a chunk of work but we are talking about the long term future of the language.

The removal of the requirement for the async keyword should however be fairly straight forward (says me, the naive non compiler writer).

Levi-Lesches commented 2 years ago

I mentioned before that I do see value in unawaited_futures, and using an explicit // ignore comment to document when it's not needed. Given those two, I don't really see the need to remove the await keyword. In fact, I find it helpful to think of Futures as a regular Dart object that needs to be explicitly deconstructed before the value can be used. Dart and futures were my first introduction to async programming and I found it really intuitive: you get a regular object that you can pass around unevaluated, but you can also call await (which is essentially a method in my mental model) to get it to actually evaluate. Making all this implicit might hide the thought process and make it seem that in the right circumstances, values just magically pop out of futures.

bsutton commented 2 years ago

@Levi-Lesches I understand your argument (and like your mental model of futures) so the question is whether making sync and async calls 'look' the same is a problem.

From a human static analysis perspective the reader certainly loses information.

The same argument however can me made for 'var'. In fact I'm made just that argument in an issue here shortly after var arrived. Now 'n' months later I no longer worry about var and it doesn't noticably affect my ability to do static analysis of my code.

@irhrn wrote:

It's very important for state management to be aware where Dart code may allow interleaving of other code.

I would argue that as soon as your function returns a Future then you have to be assuming that other code is running.

I'm not certain that explicitly awaiting each function adds any additional information.

For the internals of the function I can assume that each line will run sequentially unless I explicitly unawait a function.

So the question that remains is how important is it to know between two specific lines whether other code might run? Given the IDE will give you this information (as it does for var) how often do you need to know this that it's important enough to pollute the entire code base. I think that I would argue it's rare enough that it doesn't outweigh the advantages of 'await by default'.

The few times I've had to worry about this it has been constrained to the scope within a class and I'm adding so many comments that the await function is lost in the sea of explanation over the state management.

Levi-Lesches commented 2 years ago

As a side note, if this does happen, I'd much rather defer over unawait. unawait would have to be explained historically -- undoing the effects of a then-obsolete await keyword -- while defer is just straight to the point.

munificent commented 2 years ago
Future<void> foo<T>(T Function() callback) async {
  print('before');
  await callback(); // error
  print('after');
}

You might have some lint enabled as an error. As far as I know, that code is fine according to the language.

This doesn't make much sense to me, what code is generated in these scenarios? The call is still run async but you are saying the compiler doesn't know its an async call?

Without an await, it will synchronously invoke the callback and then discard any future that it returns. If that future later completes, you don't get the result. If it completes with an error, it may abort the program.

lrhn commented 2 years ago

I would argue that as soon as your function returns a Future then you have to be assuming that other code is running.

If you call a function which returns a future, then no other arbitrary code will run until you await that future. If you don't, your code is running synchronously. That's why you need to write the await, and why reading an await is the signal that something else might happen.

You can argue that any time you call another function, something might happen, but in general, you can predict what that something is based on the function you call. When you await, arbitrary code may run. That's the big difference.

lrhn commented 2 years ago

In general, Dart does not let you abstract over asynchrony. You can't treat synchronous code and asynchronous code in a uniform manner. That's a problem with FutureOr, you'll have to either split into a synchronous path and an asynchronous path, or coerce it into one asynchronous path (by awaiting the FutureOr, which is allowed since you can await non-futures, well, unless you enable the await_only_futures lint).

A type parameter which can either be a future or a non-future type (which is all unbounded type parameters) is almost always treated as representing a synchronous value. A value which can be anything is not expected to be a future, and if it is a future, it's treated as a value, not an asynchronous computation. Or in short, the type T is not a future type in static analysis, no matter what it might be bound to at runtime.

We might want to have a warning when an asynchronous function (statically typed as returning a Future) is assigned to a non-asynchronous function type (returning a non-future top type), because it's very likely that if that function gets called, and we can assume it will, otherwise the code is unnecessary, then its result will not be awaited. The same goes for overriding a non-asynchronous function with a synchronous function in a subclass (which the SDK does in at least one place, and it has been a problem).

bsutton commented 2 years ago

@irhn So I'm trying to understand these two statements:

you can't treat synchronous code and asynchronous code in a uniform manner A type parameter ... is almost always treated as representing a synchronous

Are they not contradictory?

So we also seems to be some inconsistency here (partly mine).

That's why you need to write the await, and why reading an await is the signal that something else might happen.

This type of static analysis problem already exists in dart.

1) async call in a sync function in existing dart can't be awaited

void func() {
callAsyncFunc(); /// can't await and no warning
}

2) async call in an async function in existing dart

Future<void> func()  async {
callAsyncFunc(); /// forgot to await, and only get a warning with the correct lint enabled.
}

3) the proposed await by default async call using proposed async by default

Future<void> func() {
callAsyncFunc(); // call works as expected, no effort required by developer.
}

Both 1. and 2. are common sources of bugs that the current compiler lets through.

  1. has no error and no lint available (I've previously opened an issue for a lint on this one).
  2. can be covered by an existing lint

So dart already allows you to call an async function with zero clue to the user that async code is going to execute. I find this very problematic.

I've had serious bugs caused by 1 and 2 that are extremely hard to track down.

Since I've enabled the lint for 2. bugs from this one are no longer an issue. I do have to wonder why this is a lint and not an error by default. I'm guessing for historical reasons but it feels like a bad decision going forward. We are encouraging users to blindly create bugs.

In the 3) (the proposed change) gives you a clue (the Future return) and in 99% of cases creates code that does what you expect it to (e.g. await the function).

The existing state of play has users creating code that doesn't do what they expect.

So we are left with one issue. Is the future return type sufficient information for static analysis.

As I've noted previously I see this change as having similar repercussions as did the change to var and in the reality the reduced visibility hasn't affected my ability to perform static analysis.

The reality is that only a very small percentage of code is sensitive to other code running asynchronously (outside the scope of the current function) and as I've noted, if there is a dependency, it tends to be heavily commented.

The counter argument is that if we have lints for 1 and 2 and enforce them as errors then we get a similar results with less effort.

My argument is that decluttering the code is significantly advantageous when you take a long term view of Dart and is akin to removing 'new' and moving to 'var'. One thing I love about Dart how it has made a significant effort to reduce boilerplate and declutter the language. Awaits are everywhere and apart from var probably the most heavily used keyword so seems like a good target for the next stage of decluttering the language.

If no one objects I'm going to open a separate issue for the removal of the 'async' keyword in the method signature as I think it is really a separate issue that is being overshadowed by this discussion.

bsutton commented 2 years ago

@irhn So now I'm going to contradict myself.

You raised the question about whether we await multiple times i.e.

Future<Future<int>> a(){}

var b => await await a();

In my original response I said that we should only await once. I'm now disagreeing with myself.

Essentially I'm looking to automate the unawaited_futures lint.

Whenever I see this lint I just add an await in the code.

As such I'm looking for this enhancement to have an implied await anywhere that lint would trigger and in no other circumstances.

This means that in sync functions this action wouldn't be applied. Instead, for sync functions we should have an error that requires the user to unawait the call.

I'm still confused about the issues raised round semantic changes to the language. I'm not certain if these are a result of a mis-understanding of my intent or my naivety regarding compilers.

When I add an await manually it doesn't change the semantics so I don't understand why automating the addition of the await would change the semantics.

With respects to @munificent concerns about callbacks then the rules are the same.

As the callback doesn't return a Future we treated the callback as a sync call as the compiler does now.

My proposal is that we only auto await calls in a function that returns a future. I 'think' this answers @munificent concern.

leafpetersen commented 2 years ago

When I add an await manually it doesn't change the semantics so I don't understand why automating the addition of the await would change the semantics.

It most certainly does change the semantics - it causes an await! This would be a massively breaking change to do without introducing a new keyword (that is, starting from a state in which no existing code is subject to automatic awaits).

That said, the core idea can be worked out in at least some settings, and has been explored before. async/await is a monad, and this paper is a fairly detailed exploration of one approach to automatically inserting binds and returns as needed for any monad. There may have been follow on work, I don't know.

bsutton commented 2 years ago

Sorry, poorly worded on my part.

A change in semantics over and above what you expect by adding the await keyword.

S. Brett Sutton Noojee Contact Solutions 03 8320 8100

On Thu, 16 Dec 2021 at 15:50, Leaf Petersen @.***> wrote:

When I add an await manually it doesn't change the semantics so I don't understand why automating the addition of the await would change the semantics.

It most certainly does change the semantics - it causes an await! This would be a massively breaking change to do without introducing a new keyword (that is, starting from a state in which no existing code is subject to automatic awaits).

That said, the core idea can be worked out in at least some settings, and has been explored before. async/await is a monad, and this paper https://www.cs.umd.edu/~mwh/papers/monadic.pdf is a fairly detailed exploration of one approach to automatically inserting binds and returns as needed for any monad. There may have been follow on work, I don't know.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/language/issues/2026#issuecomment-995436963, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OCAWOMCFNALTWNCUOLURFVZHANCNFSM5J5REQGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

lrhn commented 2 years ago

So I'm trying to understand these two statements:

you can't treat synchronous code and asynchronous code in a uniform manner A type parameter ... is almost always treated as representing a synchronous

Are they not contradictory?**

Not as intended, but it's subtle.

The point is that if a type parameter is not bounded by Future, the code almost certainly is going to be treating it as a non-future value, whether it actually is a future or not. It won't be awaited. So, it's treating it synchronously, whether it's a future or not.

You can't write code which treats non-futures synchronously and futures asynchronously in a general way. The result is always going to be either be tainted by the asynchrony, or treat futures like any other synchronous value.

When I say that you "can't abstract over asynchrony", I mean that you can't (easily) write (good) code and let a parameter, type or value, decide whether that code is synchronous or asynchronous.

It's technically possible to do:

/// Calls `after` after `action` has provided a result.
T Function<T>(T action(), void after()) {
  var result = action();
  if (<T>[] is List<Future>) { 
    // T <: Future
    return (result as Future).whenComplete(after);
  }
  after();
  return result;
}

That's not easily-written or good code, because the check for T being a Future type is tagged on and not visible in the function signature or types at all. That's why I need a cast. (Also, if I hadn't had whenComplete, and had had to rely on then instead, I wouldn't have been able to make the code type correct.)

Anyway, for this proposal, automatically inserting await in a function which is deemed asynchronous (returns a type which is statically recognized as a future type) at any point where an expression has a static type which is a future type, is definitely possible. To not be breaking, it needs a way to avoid the await, because some futures are not awaited. Some functions returning Future do not use await and might not be async.

For this to work, it should also affect functions returning Streams, which could be async* functions. That's a little more dubious, because returning a stream without being async* is more common than returning a Future without being async.

So, I'd probably find it more palatable if we use the async/async* markers to enable this feature, and won't enable it on any function returning a future or stream.

bsutton commented 2 years ago

@tatumizer agreed.

I'm not proposing we change Async* and I can't see the need for Async .

It's worth noting that dart already Auto awaits lambda functions.

I'm also proposing that we enforce unawait in sync functions to remove the ambiguity of intent when calling an async function. At the very least there should be a lint recommending an await.

Levi-Lesches commented 2 years ago

At the very least there should be a lint recommending an await.

Again, there is: unawaited_futures.

I can't see the need for async.

I'll take the other side here and argue that it's actually helpful to have all the information needed to understand a function in its declaration. This discussion makes it clear that if we go forward with removing await/async, the compiler needs to do a lot of sophisticated work to decide whether a given function should be sync or async. But that means 1) that may be impossible when the body of the function is unknown at compile-time (if it's overriden), and 2) humans need to do all that analysis too.

Sure, in the very simple case, it may be obvious to see that a future is being awaited, but the Dart team pointed out several nuances that you can't expect beginners to understand. After all, good code means readable and easily understandable code. Explicitly marking your function as asynchronous lets the reader quickly see "this function is named X, returns Y, takes paramter Z, and is a/synchronous".

bsutton commented 2 years ago

The Async keyword on an Async method is completely redundant. The future return type or lack there of removes any doubt as to what type of fuction is being called.

I don't believe an overridden function can have a different return type to the super and if it could we are not making the problem any worse than it is now.

I don't believe that this proposal requires the compiler to do much more than it currently does, after all, as you noted, we already have a lint that has no problem identifying these instances in an Async method. I will let the dart Devs comment further on this one.

The lint you are referring to only applies to Async functions. I'm requesting a new lint that requires unawait on async functions that are called within a sync function.

On Fri, 17 Dec 2021, 8:04 pm Levi Lesches, @.***> wrote:

At the very least there should be a lint recommending an await.

Again, there is: unawaited_futures https://dart-lang.github.io/linter/lints/unawaited_futures.html.

I can't see the need for async.

I'll take the other side here and argue that it's actually helpful to have all the information needed to understand a function in its declaration. This discussion makes it clear that if we go forward with removing await/ async, the compiler needs to do a lot of sophisticated work to decide whether a given function should be sync or async. But that means 1) that may be impossible when the body of the function is unknown at compile-time (if it's overriden), and 2) humans need to do all that analysis too.

Sure, in the very simple case, it may be obvious to see that a future is being awaited, but the Dart team pointed out several nuances that you can't expect beginners to understand. Explicitly marking your function as asynchronous lets the reader quickly see "this function is named X, returns Y, takes paramter Z, and is a/synchronous".

— Reply to this email directly, view it on GitHub https://github.com/dart-lang/language/issues/2026#issuecomment-996549812, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32ODYOXTNZLKBZ3TQQD3URL4I5ANCNFSM5J5REQGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

cedvdb commented 2 years ago

The core idea of the proposal should be established as beneficial or not before putting any dynamics, generics or any edge case into the view. Or even compiler / analyzer inner working for that matter, that's discussion about whether or not it is feasible and is only understood by a few.

The original proposal was about implicitely adding await to an expression when:

In the discussion above there was no counter-argument to idea that this would make code stronger in both expressiveness and robustness. It would be nice to have examples where this would be a problem. If there is none we can establish that it would be beneficial.

The argument that unawaiting future in State.initState is common, is a non issue since that function does not return a Future.

lrhn commented 2 years ago

@bsutton

The Async keyword on an Async method is completely redundant. The future return type or lack there of removes any doubt as to what type of fuction is being called.

Not currently true. You can write a "synchronous" (non-async) function which returns a future. There are functions which are written in this way, You can make those functions automatically async and insert unawait, they should keep working the same, but the way futures are passed around in there means you need plenty of unawaits. (If we assume every expression of with static type Future (not FutureOr or Future?) is implicitly awaited, unless it is an unawait expression or the operand of an unawait expression.)

We also have functions with non-Future return types, which are async. Those should be unaffected by this proposal.

I don't believe an overridden function can have a different return type to the super and if it could we are not making the problem any worse than it is now.

It can. An overriding method can have a return type which is a subtype overridden methods return type. Since Future is a subtype of void, that can happen. (I highly recommend not making that particular override, though). A return type of FutureOr or Future? can meaning fully be overridden by Future, though.

I don't believe that this proposal requires the compiler to do much more than it currently does, after all, as you noted, we already have a lint that has no problem identifying these instances in an Async method. I will let the dart Devs comment further on this one.

The compiler will have to do more, because inserting the await now has to be part of type inference, because it changes the type of the expression. It can't just be done afterwards, like the current lint. It can hide coding errors where you call the wrong function and get a future instead of the expected value. That would currently just be a type error, with this proposal, it needs to be handled in the compiler. Implicit type coercion hides errors!

The lint you are referring to only applies to Async functions. I'm requesting a new lint that requires unawait on async functions that are called within a sync function.

You're also requiring that those functions become async functions. Since they do have return types of Future, that's possible. It might change timing.

@cedvdb That code doesn't do what you think it does. It's the Future.value constructor which unwraps the future it gets as argument, not the await. Try:

Future<void> test() async {
  final a = Future<Future<Future<int>>>.value(
      Future<Future<int>>.value(Future<int>.value(3)));
  final b = await a;
  print(b);  // Instance of '_Future<Future<int>>'
}
bsutton commented 2 years ago

@irhn so there are some issues but if I understand correctly none of them are intractable.

You can make those functions automatically async and insert unawait, they should keep working the same, but the way futures are passed around in there means you need plenty of unawaits.

In my experience this is the exception not the rule. In comparison to the amount of times we have to 'await' every method we still end up with a very large net gain.

The compiler will have to do more So the compiler will have to do more work and I know I'm not the one that is going to have to do the work, but if more compiler work means less developer work...

Levi-Lesches commented 2 years ago

In the discussion above there was no counter-argument to idea that this would make code stronger in both expressiveness and robustness.

I still disagree that this makes code clearer or more expressive. First off, there is no difference in functionality, only the default, so it's not like we're introducing some new feature here. If anything, some manual work and thought may be required to ensure that any automatic migrations align with the author's original intentions.

I don't think the problem is really the automatic async, because, as was pointed out, a return type of Future already hints that something asynchronous may be going on. It's the automatic await that bugs me. This makes Futures "magical": they represent a function containing some computation or operation, but for some reason, when you plug that value in somewhere, it transforms itself into the result of that computation right away, and the Future itself disappears. Essentially, this is my mental model of what a Future really is:

/// A wrapper for a [computation] that can be evaluated with the [awaited] method.
class MyFuture<T> {
  T Function() computation;
  MyFuture(this.computation);

  T awaited() => computation();
}

// This is my understanding of a MyFuture: 
int getServerToken() => 3;
void test() async {
  final tokenFuture = MyFuture(getServerToken);  // Obtain a MyFuture<int>
  print("Beginning operation on $tokenFuture");  // Do other stuff in the meantime
  final token = tokenFuture.awaited();           // Evaluate when we're ready
  print("Operation complete. Result: $token");   // Now we have the result
}

// Now, to compare with the built-in Future implementation:
Future<int> getServerTokenAsync() async => 3;
Future<void> asyncTest() async {
  final tokenFuture = getServerTokenAsync();     // Obtain a Future<int>
  print("Beginning operation on $tokenFuture");  // Do other stuff in the meantime
  final token = await tokenFuture;               // Evaluate when we're ready
  print("Operation complete. Result: $token");   // Now we have the result
}

/// These two functions output the same results:
void main() {
  test();
  asyncTest();
}

I'm probably missing some technical details about the event loop and such, but this is the general model that's presented to beginners with codelabs and documentation, and it hasn't led me astray so far. With this mindset, Future is just a regular object, and await myFuture is a shorthand for myFuture.await(), and not some built-in language mechanic (even though Future.await may be).

Making awaits implicit and especially adding the notion of unawaited (or delayed, or deferred, or whatever), changes this simple abstraction into something that's now baked into the language, which not only makes it harder to debug, but may make the concept of asynchrony harder for users to understand in general. Currently, the main problem users have with Futures is "how do I make it do what I want?", to which the answer is "add a keyword that tells Dart to do it". Simple enough. However the proposal changes that to "how come this is doing something I don't want?", to which the answer is "Dart made an assumption, you have to override it". I get that typing out await a lot may feel verbose, but what good are words if not to communicate intent?

Levi-Lesches commented 2 years ago

Sorry, my mistake, I meant rather to say that the future is unwrapped immediately, not necessarily in the event loop, but in lines of code. That is, by the next line of code (not necessarily by the next user-side event), the future is lost. I'll edit the comment.

cedvdb commented 2 years ago

await by default sounds nice because it makes code read like synchronous code which most of the time is more readable. It's also more robust because most of the time it's the intent of the developer, that is forgetting an await is a bug. Fortunately that bug will give a compile error most of the time.

It will however give WTF moments with race conditions on concurrent function affecting the same data. Maybe the same can be said about deferred by default but it's (maybe?) a bit more obvious ?

I like the idea but this really begs the question why other languages have not go this route.

Levi-Lesches commented 2 years ago

Please run this program. Does it match your expectations?

Yes, although I'm not sure what your point is. That happens, to my understanding, because when Dart hits the await Future.delayed line, it says "okay, I should check on the other foo I have running in the meantime", which results in Dart jumping out of foo and back into main, alternating calls to unawaited and awaited. The reason they end up interleaved is because the first foo isn't explicitly awaited (if it were, Dart would jump "out of" main, realize it has nothing left to do, and continue on with evaluating unawaited). If you do, you'll notice all the unawaited lines finish before the awaited lines begin. Additionally, the await in foo is also super important, and without it, the whole program would complete instantly.

If anything, that only illustrates the importance of keeping usage of await and the-lack-of-await consistent and deliberate. Which is hard to do when the compiler automatically inserts awaits everywhere you didn't ask it to.

Levi-Lesches commented 2 years ago

It's also more robust because most of the time it's the intent of the developer, that is forgetting an await is a bug. Fortunately that bug will give a compile error most of the time.

It will however give WTF moments with race conditions on concurrent function affecting the same data. Maybe the same can be said about deferred by default but it's (maybe?) a bit more obvious ?

This is the crux of my argument. We have two options, each one leaves very obvious and very important bugs for beginners, intermediates and tired pros to stumble upon. Difference is, if you forget await today, you either get a compile-time error or a lint -- or both. If we switch the default to auto-insert await, those compile-time errors turn into bugs. Not direct runtime errors, but rather an elusive modification to the order your code executes, which can lead to invalid data, jank in UI, or other runtime errors. Not to mention blurs the line between async/sync in ways it arguably shouldn't. I'd much rather the red squiggly line.

bsutton commented 2 years ago

@cedvdb

I like the idea but this really begs the question why other languages have not go this route.

Someone has to be first :)

I think we need to look at the problem from two perspectives, the novice experience and the experts experience.

When starting out with Dart, 99% of my WTF moments came from forgetting to await things. For the novice user this change will make the code do what they expect it to do 99.99% of the time.

This sole data point makes the change worthwhile.

As to race conditions, the reality is that in dart these are rare. In over two years of working with Dart I've only had to deal with race conditions a handful of times.

For a novice I'm not convinced that the presence/absence of an await is going to make a concurrent bug any easier/harder to find.

From an expert's perspective ( I think I can refer refer to myself as such). Forgetting an await happens about every 30 seconds when I'm programming and as such is a constant irritation.

The absence/presence of an await will not affect my ability to find concurrent bug as I know what I'm looking for and how Dart works.

So: pros:

cons:

I don't think we should be arguing to not proceed based on a rare use case where the change 'might' make it harder to find a bug as opposed to a change that will fix bugs for everyone from day one and make code less cluttered.

So I think the summation is that we make the world better for everyone and the possible downsides are rare.

If you have other pro/cons please lets add them to the list.

edit: added pro/cons

lrhn commented 2 years ago

Con:

Currently, the static type of Future<int>.value(0) is Future<int>. You can't assign that to int. If you insert an await, that changes the type of await Future<int>.value(0) to int which you can assign to an int variable.

There is no implicit type conversion or coercion.

If you write double x = await Future.value(1);, it works. The type inference will give the 1 a context type of double. The await is available to type inference, which makes it change the context type from double to FutureOr<double> when passing through the await.

With implicit await, if you try to write double x = Future.value(1);, I'll almost guarantee that it doesn't work. "Something magical" happens between = and Future, but type inference doesn't know that until it has already found the type of Future.value(1).

Type inference needs to know where await is in the program in order to do a good job, because await affects types. If awaits are inserted based on static types, found during type inference, that information just isn't there soon enough.

Levi-Lesches commented 2 years ago

Also,

eliminate common bugs encountered by novices/experts forgetting to await

and

code does what a novice user expects it to do in 99+% of cases

are basically the same thing. (Which I don't necessarily agree with. Keep in mind Flutter has the all-encompassing FutureBuilder used in almost every tutorial that needs an unawaited future to work. If beginners who barely even know Dart at that point learn they need to prevent their objects from transforming every time its referenced, that can get confusing.) Also,

improve static analysis by forcing users to unawait functions.

is actually a con? Having to put more work into the type system to detect these situations that just work today (or that we already have lints for).