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.09k stars 1.56k forks source link

Proposal: Provide an "Async Aware" operator: #25986

Open typotter opened 8 years ago

typotter commented 8 years ago

While it is helpful to unravel futures using async/await, when the non-future item you need is several futures deep, the awaits quickly become hard to read. This comes up a lot in testing/pageloader objects

Example:

expect( await( await (await childElement).description1)).innerText. "blah");

How about introducing an async operator to make this a little cleaner? Something like this:

expect( await childElement->description1->innerText, "blah");

The first await is for the Future returned by inner text. the "->" operator sees that the left side is a future, awaits it, then acts as the access operator. Doesn't have to be "->", but I like the parallel of de-futurizing to pointer dereferencing in c++.

rkj commented 8 years ago

+1, this would be an awesome feature.

lrhn commented 8 years ago

It is unreadable, but it gets more readable if you introduce temporary variables:

var element = await childElement;
var description = await element.description1;
var text = await description.innerText;
expect(text, "blah");

Another option is to add a post-fix operator meaning the same thing as await _. Being postfix it allows easier chaining. Let's call it !!, then you can write;

expect(childElement!!.description1!!.innerText!!, "blah");

Obviously not a good syntax, it's unreadable too :)

mezoni commented 8 years ago

How about introducing an async operator to make this a little cleaner?

I agree with @lrhn. It would be much better if you try to write the more readable (and clear) source code.

P.S. Many lines of source code can rewritten in your manner (into a single line) but this does not means that it is good style (and necessarily require special language designs to simplify this monstrosity).

rkj commented 8 years ago

Of course we can introduce local variables, and most of the time we do it. But it introduces lot of noise and completely unnecessary boilerplate, compare:

  expect(await (await appBar.billingInternal).visibleText, 'Internal');
  expect(await (await appBar.lcsCustomer).visibleText, 'LCS');
  expect(await (await appBar.tier).visibleText, 'Tier 3');
  expect(await (await appBar.internalCustomerId).visibleText, 123);

to a version that would have a variable in between of all - that is not used for anything other than visible text. I have never seen this chained awaits in production code, but it happens in most of the tests we have.

There was a number of complaints we received when we moved to async pageloader, and they were:

  1. "The awaits seem to add no value and add complexity, even in getting simple field values".
    If you could rewrite all the expects like this:

    expect(await pageObject.helloWho, equals("Tommy"));

    into:

    expect(pageObject->helloWho, equals("Tommy"));

    then this problem would go away.

  2. "Awaits often need to be nested, leading to confusion about where to put awaits".
    This is what the bug is about.
  3. "Any helper properties/methods that wrap PageElement fields also have to be awaited because the fields have to be awaited"
  4. "The await keyword has low precedence, resulting in somewhat unintuitive parentheses placement. e.g. the statement await element.getProperty() != null will compare an incomplete future to null and await on that boolean"
    This would also be nicely solved with an operator (assuming element->getProperty(), would not be translated to (await element.getProperty)())
mezoni commented 8 years ago

@rkj Maybe you forgotten (or you did not know) that the keyword await means an unary operator await. The same as the character ! means an unary operator logical NOT.

I do not think that the bunch of operations with the same operator can be replaced in a single operation (with a single operator which will be applied to each operand).

We do not mean matrix operations, but talk about the operations of the random data (which may alternate with types).

mezoni commented 8 years ago

@rkj

"Awaits often need to be nested, leading to confusion about where to put awaits".

I disagree. You can look on the operator await as on the some mix of operator and statement at the same time, where the statement is the same as yield statement (that is, the enter/leave point).

Eg.

// phase 0
// Enter f1()
await f1();
// Leave f1()
...
...
// phase 1
// Enter f2()
var x = await f2();
// Leave f2()

This is the same as:

f1();
yield return;
f2();
var x = yield return;

When you try to awaits often need to be nested, leading to confusion about where to put awaits then you really will be confused only because you try nest many complex enter/leave points into the very big linear statement with many operations which are logically (and practically) not so easy.

rkj commented 8 years ago

@mezoni "awaits often need to be nested, leading to confusion about where to put awaits" was a feedback from many engineers at Google using the async https://github.com/google/pageloader tests. Most experienced developers are not confused by this at all, but new people certainly are. But I would leave that aside, new operator won't help with confusion much.

The problem about nesting is real though. We have hundreds of occurrences, even though we actively discourage it in code reviews - to be replaced with vars, as you mentioned earlier. But this is really a lipstick on a pig, as when we used sync API before the code was much nicer and compact.

As per unary operator, I am not a compiler person, but with my limited understanding of it, the proposal for '->' would mean to be a binary operator that could be implemented roughly as this pseudocode:

operator ->(a, symbol b) {
  Future tmp = a.b; // invoke method/getter/message b on a
  return await tmp;
}
mezoni commented 8 years ago

@rkj

This is a very bad style of programming.

expect( await( await (await childElement).description1)).innerText. "blah");

What do you think? Is this code good or bad (see below)? Restriction: we cannot reduce the calls to func()

expect( func( func (func (childElement)).description1)).innerText. "blah");

What is your solution in such case (with many calls to func())?

  1. Propose a language improvements to have a way to reduce the number of calls to func()
  2. Refactor the source code into the separate lines of statements
rkj commented 8 years ago

Let me start with func() - I have not seen something like that too often - (assuming you mean f1(f2(f3(a).b).c)), it most cases it is good to split it. If you literally mean f(f(f(a).b).c), then it would be pretty crazy API.

Going back to the original request, it is concerned with the form of a.b.c that is pretty common in synchronous API, but becomes (await (await a).b).c in asynchronous API. This dichotomy is somewhat painful and seems like it could be great alleviated with Ty's proposal. Dart already provides great async features - the await is simply amazing in making the code more readable and easier to reason about, and a great improvement over anything else I've seen.

As per the code example, as Ty mentioned it is unreadable, and bad, and what we currently do is we do split it in multiple lines, but this have negative overall effect of tests readability. If you look at my example from https://github.com/dart-lang/sdk/issues/25986#issuecomment-195755581 and split everything into variables test have more boilerplate lines that just assign variables than actual testing code. Also notice in Lars example the var text - that would need to be either reassign before every expect making it hard to tell what is actually being tested, or would need to have those ridiculosly long names that just repeat code above. Is this really better:

var billingInternal = await appBar.billingInternal;
var billingInternalVisibleText = await billingInternal.vsibileText;
expect(billingInternalVisibleText, 'Internal');
var lcsCustomer = await appBar.lcsCustomer;
var lcsCustomerVisibleText = await lcsCustomer.visibleText
expect(lcsCustomerVisibleText, 'LCS');
...

? It has 3x as many lines, actual expect is somewhat hidden, and there is lot of repeated strings. We mostly use page object to move this code to, so the tests stay clean so more realistically we would have:

expect(await appBar.billingInternalVisibleText, 'Internal');
expect(await appBar.lcsCustomerVisibleText, 'LCS');

but this comes with the cost that now the appBar PO must have all the specific methods that are ever used in the test unwrapped inside, making them longer and less generic - all for our dislike to parenthesis/missing this feature.

mezoni commented 8 years ago

Going back to the original request, it is concerned with the form of a.b.c that is pretty common in synchronous API

// 2 operations of member access // total 2 operations in single statement a.b.c

// 2 operations of member access // 2 operations of await enclosed in parentheses // total 4 operations in single statement (await (await a).b).c

This is not the same statements.

First statement produces 2 computations when second statement produces 4 computations. Do not need compare them as equal statements (even if by the human logic they looked as the logically identical) because second statement is twice harder and you cannot ignore (and reduce) it because every computation is important in each case.

P.S. Compiler does not have an associative thinking as human.

Afsar-Pasha commented 4 years ago

What about .await (used by rust)? The given example can be written as

expect(childElement.await.description1.await.innerText. "blah");
lrhn commented 4 years ago

The expr.await syntax is definitely on our radar.

It should be a non-breaking change because await is already a reserved word inside an asynchronous function. It even opens up the possibility of maybeFuture?.await and cascades like x..foo.await..bar.await (as mentioned in #23000 too).

Reprevise commented 3 years ago

Any update on this?

subzero911 commented 2 years ago

x..foo.await is counter-intuitive. A desired syntax is

final store = Store()
  ..await init()
  ..await login();

It would be better to reopen https://github.com/dart-lang/sdk/issues/23000

nikhilbadyal commented 2 years ago

Any updates?