Add 'Remove unnecessary await' suggestion and fix #32363

andrewbranch · 2019-07-11T21:31:24Z

Also related to #30646

I hesitated to use the word “unnecessary” since your program could be relying on bizarre side effects of the asynchrony introduced by awaiting something sync’ly available, and indeed the message I wrote for the checker suggestion speaks only of types:

'await' has no effect on the type of this expression. ts(80007)

But, it was just too awkward to try to write a code fix message that tries to dance around the issue—so it simply says

Remove unnecessary 'await'

Screen capture 🎥

sandersn

Need a test case with parenthesized numbers to make sure dots get treated correctly.
Definitely say "Remove unnecessary await". It's punchier than a more accurate message.

sandersn · 2019-07-11T22:37:54Z

src/services/codefixes/removeUnnecessaryAwait.ts

+            return;
+        }
+
+        const parenthesizedExpression = awaitExpression && tryCast(awaitExpression.parent, isParenthesizedExpression);


what about (await 1).toFixed() ? Does this correctly produce 1..toFixed()?

Gooood call. It does not 🙈

Is it too embarrassing to leave the parens in and just produce (1).toFixed() ? I mean, most people don't like parens, but they've always treated me well.

I think that’s what I’ll do, when the inner expression is a DecimalIntegerLiteral.

andrewbranch · 2019-07-11T22:42:39Z

Definitely say "Remove unnecessary await". It's punchier than a more accurate message.

Are you suggesting I change the checker suggestion message, or just affirming my choice for the refactor?

sandersn · 2019-07-11T23:06:19Z

Affirming your choice for the refactor.

andrewbranch · 2019-07-11T23:24:13Z

Other test failures show that await x being the same type as x when x: any shouldn’t trigger the suggestion 👌 Fixed now.

fatcerberus · 2019-07-12T06:29:44Z

'await' has no effect on the type of this expression

FWIW I personally find this a bit confusing without thinking about it; when dealing with async code I don’t intuitively think in terms of types - I just go “oh this runs in the background, I should await it”. So my initial reaction to the “no change of type” message is “what on earth are you talking about, of course await wouldn’t change the type” and only 10 seconds later would I go “...oh, Promise, right.”

Something to think about at least, especially since the async-await syntax is specifically designed to hide the existence of Promise objects.

ajafff · 2019-07-12T14:34:03Z

src/services/utilities.ts

@@ -1992,6 +1992,10 @@ namespace ts {
        return typeIsAccessible ? res : undefined;
    }

+    export function isDecimalIntegerLiteral(node: Node): node is NumericLiteral {
+        return isNumericLiteral(node) && /^\d+$/.test(node.text);


this actually doesn't do what it's supposed to do. NumericLiteral#text contains the normalized value, e.g. 1.0 -> 1

you probably want to use .getText() here to get the raw source text of the numeric literal

ajafff · 2019-07-12T14:40:06Z

src/services/codefixes/removeUnnecessaryAwait.ts

+
+        const parenthesizedExpression = tryCast(awaitExpression.parent, isParenthesizedExpression);
+        // (await 0).toFixed() should keep its parens (or add an extra dot for 0..toFixed())
+        if (parenthesizedExpression && isPropertyAccessExpression(parenthesizedExpression.parent) && isDecimalIntegerLiteral(awaitExpression.expression)) {


this also needs to handle NewExpression without parens:

(await new C).foo(); // currently fixed to new C.foo(); // which executes as new (C.foo)(); // should instead be (new C).foo()

ajafff · 2019-07-12T14:42:12Z

src/services/codefixes/removeUnnecessaryAwait.ts

+
+        const parenthesizedExpression = tryCast(awaitExpression.parent, isParenthesizedExpression);
+        // (await 0).toFixed() should keep its parens (or add an extra dot for 0..toFixed())
+        if (parenthesizedExpression && isPropertyAccessExpression(parenthesizedExpression.parent) && isDecimalIntegerLiteral(awaitExpression.expression)) {


also needs to handle await inside NewExpression#expression:

new (await getC()).Class(); // currently fixed to new getC().Class(); // which executes as (new getC()).Class(); // should instead be new (getC()).Class();

andrewbranch · 2019-07-12T15:46:44Z

FWIW I personally find this a bit confusing without thinking about it; when dealing with async code I don’t intuitively think in terms of types...

@fatcerberus that’s definitely fair, but for a checker message I wanted to be fairly careful and accurate with the language we use. The only possible alternative I can think of is something like

Type '{0}' is not Promise-like and may not need to be awaited

or

Type '{0}' is not Promise-like. Did you mean to omit 'await'?

@sandersn @DanielRosenwasser thoughts?

sandersn

Two suggestions to consider for now or later.

src/services/codefixes/removeUnnecessaryAwait.ts

sandersn · 2019-07-12T15:49:09Z

src/services/codefixes/removeUnnecessaryAwait.ts

@@ -27,12 +27,34 @@ namespace ts.codefix {
        }

        const parenthesizedExpression = tryCast(awaitExpression.parent, isParenthesizedExpression);
-        // (await 0).toFixed() should keep its parens (or add an extra dot for 0..toFixed())
-        if (parenthesizedExpression && isPropertyAccessExpression(parenthesizedExpression.parent) && isDecimalIntegerLiteral(awaitExpression.expression)) {
+        const preserveParens = parenthesizedExpression && (


I still think this is the right thing to do, but now that we have 3 exceptions, the queasy feeling in my stomach says that there more out there. I almost would like to always preserve parentheses, except that the result is so ugly in the common case.

What if we flipped it to only skip parens for identifiers and callexpressions?

Yeah, I had the same feeling. I think this is probably a good plan.

sandersn · 2019-07-12T15:56:37Z

@fatcerberus I disagree, I use types to guide my thinking whenever I write monadic code.

I'm not a normal programmer though. Specifically:

I haven't written much async/await code, so I haven't gotten into the habit of thinking concretely about its behaviour.
Most of the monadic code I've written is in Haskell, where all monads use the same syntax.
And where there are tons of different monad utilities.

Still, I think a type checker reporting an error in terms of types is reasonable.

fatcerberus · 2019-07-12T15:57:24Z

@andrewbranch That's fair, but on the other side of that same coin, being too literal with calling out "type" can be confusing too when the existence of a type is an implementation detail, see e.g. discussion starting here w.r.t. excess object literal property errors (an endless source of confusion for newbies): #32158 (comment)

In that case the compiler says "type ... is not assignable to" which is literally true (it's a "fresh object literal" type), but ends up looking like a statement about structural typing in general, leading people to believe types are closed.

fatcerberus · 2019-07-12T16:01:16Z

@sandersn

I haven't written much async/await code, so I haven't gotten into the habit of thinking concretely about its behaviour.

Yeah, I like monads too but that's the thing, async/await is specifically designed to hide the existence of the promise monad and let you write basically the same code you would if the monad wasn't there. So re-surfacing the type discrepancy in that context reads as confusing to me.

When I decide whether to write an await the mindset is "okay I know this is a string, now do I have wait for it or not, is the function async?", as opposed to explicit monadic code where I know I'm concretely dealing with Promise abstractions. (concrete abstractions - there's an oxymoron for you!)

fatcerberus · 2019-07-12T16:05:53Z

I personally like this one:

Type '{0}' is not Promise-like and may not need to be awaited

It seems like a nice compromise: Still in terms of types, but much clearer about what went wrong, so I don't think my strings morphed into numbers or something 😄

sandersn · 2019-07-12T16:19:56Z

:drakememe: (2620b0d, d0191da)

andrewbranch · 2019-07-12T18:02:51Z

Type '{0}' is not Promise-like and may not need to be awaited

I don’t like this because

I don’t think printing the type is particularly necessary or useful
“and may not need” is kind of a mouthful and sounds like “anemone”
It sounds wishy-washy

'await' has no effect on the type of this expression

I like this because

It’s an unambiguous fact
The core of the concept is conveyed immediately—you could basically stop reading at “'await' has no effect” and think “Oh, I don’t need 'await'”
If you were unclear about what’s happening with types, you don’t really have to care once you see the codefix message “Remove unnecessary 'await'”

@fatcerberus I do very much appreciate the input—keep the suggestions coming in general, but I’m going to stick with the original this time. Thanks all!

(This makes me think of one of my favorite lines from an excellent film 😛)

fatcerberus · 2019-07-12T18:17:56Z

It’s an unambiguous fact

Counterpoint: So is Type { ... } is not assignable to type { ... } (because it's an object literal and has extra properties), but that doesn't stop people from being misled into thinking the same error should apply to structural typing in general. There are lots of "bug" reports here that attest to that one. Sometimes being overly literal is bad too. Just ask anyone who ever got tricked by one of these: https://tvtropes.org/pmwiki/pmwiki.php/Main/LiteralGenie

😄

If you were unclear about what’s happening with types, you don’t really have to care once you see the codefix message “Remove unnecessary 'await'”

Point taken on this though, so I'll admit defeat. 👍

ajafff · 2019-07-12T20:08:00Z

src/services/codefixes/removeUnnecessaryAwait.ts

+        }
+
+        const parenthesizedExpression = tryCast(awaitExpression.parent, isParenthesizedExpression);
+        const removeParens = parenthesizedExpression && (isIdentifier(awaitExpression.expression) || isCallExpression(awaitExpression.expression));


@andrewbranch it seems you dropped the code necessary to avoid changing the parsing of NewExpression.

However, there's a whole category of exceptions that's not handled here: if the parenthesized AwaitExpression is at the start of the statement, the fixed code could parse as declaration instead of expression:

(await function(){}()); (await class {static fn(){}}.fn()); (await {fn() {}}.fn());

These cases are already handled in factory.ts, see the uses of getLeftmostExpression

andrewbranch requested review from orta, sandersn, DanielRosenwasser, weswigham, rbuckton, RyanCavanaugh and sheetalkamat July 11, 2019 21:31

sandersn requested changes Jul 11, 2019

View reviewed changes

sandersn approved these changes Jul 11, 2019

View reviewed changes

ajafff suggested changes Jul 12, 2019

View reviewed changes

sandersn approved these changes Jul 12, 2019

View reviewed changes

andrewbranch mentioned this pull request Jul 12, 2019

Better error and quick fix for missing await #30646

Closed

5 tasks

andrewbranch added 7 commits July 12, 2019 10:21

Add remove unnecessary await fix

e752f70

Add test for removing unnecessary parens after await is gone

38247e0

Fix handling of numbers in property access expressions

e0a781e

Don’t offer suggestion when awaited type is any/unknown

58d960e

Fix random other test

a54ed2d

Fix new expression edge cases

e27d260

Only remove parens for identifiers and call expressions

f22dc9d

andrewbranch force-pushed the enhancement/unnecessary-await branch from d0191da to f22dc9d Compare July 12, 2019 17:24

andrewbranch merged commit 89badcc into microsoft:master Jul 12, 2019

andrewbranch deleted the enhancement/unnecessary-await branch July 12, 2019 18:03

ajafff reviewed Jul 12, 2019

View reviewed changes

andrewbranch mentioned this pull request Jul 12, 2019

Improve accuracy of remove unnecessary await fix #32384

Merged

weswigham mentioned this pull request Nov 14, 2022

Add noUnnecessaryTypeAssertions compiler option, quickfix #51527

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'Remove unnecessary await' suggestion and fix #32363

Add 'Remove unnecessary await' suggestion and fix #32363

andrewbranch commented Jul 11, 2019

sandersn left a comment

sandersn Jul 11, 2019

andrewbranch Jul 11, 2019

sandersn Jul 11, 2019

andrewbranch Jul 11, 2019

andrewbranch commented Jul 11, 2019

sandersn commented Jul 11, 2019

andrewbranch commented Jul 11, 2019

fatcerberus commented Jul 12, 2019 •

edited

Loading

ajafff Jul 12, 2019

ajafff Jul 12, 2019

ajafff Jul 12, 2019

andrewbranch commented Jul 12, 2019

sandersn left a comment

sandersn Jul 12, 2019

andrewbranch Jul 12, 2019

sandersn commented Jul 12, 2019

fatcerberus commented Jul 12, 2019

fatcerberus commented Jul 12, 2019

fatcerberus commented Jul 12, 2019

sandersn commented Jul 12, 2019

andrewbranch commented Jul 12, 2019

fatcerberus commented Jul 12, 2019 •

edited

Loading

ajafff Jul 12, 2019

Add 'Remove unnecessary await' suggestion and fix #32363

Add 'Remove unnecessary await' suggestion and fix #32363

Conversation

andrewbranch commented Jul 11, 2019

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbranch commented Jul 11, 2019

sandersn commented Jul 11, 2019

andrewbranch commented Jul 11, 2019

fatcerberus commented Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbranch commented Jul 12, 2019

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Jul 12, 2019

fatcerberus commented Jul 12, 2019

fatcerberus commented Jul 12, 2019

fatcerberus commented Jul 12, 2019

sandersn commented Jul 12, 2019

andrewbranch commented Jul 12, 2019

fatcerberus commented Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

fatcerberus commented Jul 12, 2019 •

edited

Loading

fatcerberus commented Jul 12, 2019 •

edited

Loading