Add elaboration & quickfix for async-able arrow function #36342

weswigham · 2020-01-22T01:12:53Z

Starts to fix part of #35300. The most simple/common case, anyway.

src/compiler/checker.ts

amcasey · 2020-01-22T01:29:41Z

src/compiler/checker.ts

+                    if ((getFunctionFlags(node) & FunctionFlags.Async) === 0
+                        // exclude cases where source itself is promisy - this way we don't make a suggestion when relating
+                        // an IPromise and a Promise that are slightly different
+                        && !getTypeOfPropertyOfType(sourceReturn, "then" as __String)


I'm surprised we don't have a helper for this.

This meaning...? Seeing if a type is promisish (highly non-technical term)? Yeah, I went looking inside getAwaitedType to see if we had a helper and saw that all we did was essentially look for a then property (to match spec behavior, ofc), so that's what I did here. A helper would essentially just curry one argument and cast the result, so it doesn't seem terribly needed.

I meant checking for a then property (don't we have a Thenable concept?).

Nope. Thenable is just a Promises/A+ spec thing that roughly maps to "object with a then property" (with expected shape and behavior). We have no interface or anything special for that.

amcasey · 2020-01-22T01:36:05Z

src/compiler/checker.ts

+                        // exclude cases where source itself is promisy - this way we don't make a suggestion when relating
+                        // an IPromise and a Promise that are slightly different
+                        && !getTypeOfPropertyOfType(sourceReturn, "then" as __String)
+                        && checkTypeRelatedTo(createPromiseType(sourceReturn), targetReturn, relation, /*errorNode*/ undefined)


I would have guessed it would be cheaper to unwrap targetReturn, so we wouldn't have to instantiate Promise<source>.

targetReturn could be any number of compatible promise-likes which don't unwrap cleanly (like a custom MyStringPromise). It's much easier to wrap source. (And better mirrors what the runtime does)

amcasey · 2020-01-22T01:42:04Z

src/services/codefixes/addMissingAsync.ts

+        errorCodes,
+        getCodeActions: context => {
+            const { sourceFile, errorCode, cancellationToken, program, span } = context;
+            const diagnostic = find(program.getDiagnosticsProducingTypeChecker().getDiagnostics(sourceFile, cancellationToken), getIsMatchingAsyncError(span, errorCode));


Can this code be shared with isMissingAwaitError in addMissingAwait.ts? It seems like finding a diagnostic with particular related information might be a general utility.

Not quite! In the other quick fix, we're checking if the diagnostic's span matches the input span, then returning the diagnostic if it matches and has the target related span code. In this quick fix, we don't do the filtering on related span at the matching step, because we're looking to find the span of the related span (so we can use that instead). So, yes, there's a common iteration bit, but to handle the bits variable between the two is essentially just a some with variable condition, onto which we then append an optional extra filter/map on the final result. Which seems like a bit too multifunctional for a single helper for my tastes.

But it should totally feel similar, since I started writing this quick fix by copying the addMissingAwait quick fix, which is why the span-matching check is almost identical in structure.

DanielRosenwasser · 2020-01-22T02:01:40Z

@typescript-bot pack this

typescript-bot · 2020-01-22T02:01:46Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 7371ec1. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2020-01-22T02:15:09Z

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/61229/artifacts?artifactName=tgz&fileId=8E6ED393FD96984B3BE328E00A9B4F09CC75460ECDAD2F74B6CEDB418CBD8B8202&fileName=/typescript-3.8.0-insiders.20200122.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

DanielRosenwasser · 2020-01-25T02:06:09Z

src/compiler/diagnosticMessages.json

@@ -5436,6 +5436,14 @@
        "category": "Message",
        "code": 95099
    },
+    "Add 'async'.": {


There's already a similar quick fix for this, so the text can be reused.

DanielRosenwasser · 2020-01-25T02:06:52Z

tests/baselines/reference/errorOnUnionVsObjectShouldDeeplyDisambiguate.errors.txt

          c: () => "hello",
                   ~~~~~~~
 !!! error TS2322: Type 'string' is not assignable to type 'Promise<string>'.
 !!! related TS6502 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:4:8: The expected type comes from the return type of this signature.
+!!! related TS1356 tests/cases/compiler/errorOnUnionVsObjectShouldDeeplyDisambiguate.ts:19:10: Did you mean to mark this function as 'async'?


This is kind of a weird thing now that I think about it - why would I want to mark this as async? There's no await anywhere there.

If a user did write await in the function body, we already provide the same error message/quick fix to mark the function as async, so this could be annoying.

interface Stuff { b: () => Promise<string>; } function foo(): Stuff | Date { return { b: _ => await "hello" // two error spans with duplicate related spans } }

So this is only distinct actionable advice information for a user if they hadn't written await. Would making this async actually be the right thing? It does come up, it's unclear if it's the real fix to a problem.

This is kind of a weird thing now that I think about it - why would I want to mark this as async? There's no await anywhere there.

Because async () => "constant" is way, way more canonical and concise than () => Promise.resolve("constant") when the API you're using expects a function returning a promise.

So this is only distinct actionable advice information for a user if they hadn't written await

More like, using await means the containing function should be async to make what you're trying to do work, while if assignability to a promise return fails, it should be async to satisfy the API. Different reasons entirely that happen to have the same fix.

sandersn · 2020-02-14T23:37:53Z

It looks like this just needs a change to re-use an existing error message, plus a merge from master, before it's ready to go.
@weswigham does that sound accurate?

Also reuse existing error messages.

weswigham · 2020-02-15T01:03:22Z

@sandersn merged and changed to reuse the existing related span diagnostic messages (though I think it's maybe misleading to reuse the same message for both kinds of the similarly fixed diagnostics, since the "fix all" won't fix all instances of both, just the one).

sandersn · 2020-02-21T18:40:34Z

@weswigham this is ready to merge, isn't it?

weswigham · 2020-02-21T20:38:12Z

Yeah, if the messages and such look good to you now.

Add elaboration & quickfix for async-able arrow function

7371ec1

weswigham requested review from elibarzilay, DanielRosenwasser and amcasey January 22, 2020 01:12

amcasey approved these changes Jan 22, 2020

View reviewed changes

orta added the Author: Team label Jan 23, 2020

typescript-bot assigned weswigham Jan 23, 2020

DanielRosenwasser reviewed Jan 25, 2020

View reviewed changes

sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020

Merge branch 'master' into did-you-mean-async-arrow

9b0ea94

Also reuse existing error messages.

sandersn approved these changes Feb 21, 2020

View reviewed changes

weswigham merged commit ebca423 into microsoft:master Feb 21, 2020

weswigham deleted the did-you-mean-async-arrow branch February 21, 2020 23:51

Add elaboration & quickfix for async-able arrow function #36342

Add elaboration & quickfix for async-able arrow function #36342

Uh oh!

Conversation

weswigham commented Jan 22, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Jan 22, 2020

Uh oh!

typescript-bot commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Feb 14, 2020

Uh oh!

weswigham commented Feb 15, 2020

Uh oh!

sandersn commented Feb 21, 2020

Uh oh!

weswigham commented Feb 21, 2020

Uh oh!

Uh oh!

weswigham Jan 22, 2020 •

edited

Loading

weswigham Jan 22, 2020 •

edited

Loading

typescript-bot commented Jan 22, 2020 •

edited

Loading

typescript-bot commented Jan 22, 2020 •

edited

Loading