Add JS-specific diagnostic message for `resolve()` in Promise where type argument can't be inferred #48533

nicdard · 2022-04-03T10:45:38Z

Continues #46637
I have cherry picked the commit from there.
Address the comments of the above PR, restricting the change to JS files.

jakebailey · 2022-04-22T16:11:11Z

src/compiler/diagnosticMessages.json

@@ -3365,6 +3365,10 @@
        "category": "Error",
        "code": 2809
    },
+    "Expected {0} arguments, but got {1}. TypeScript may need a JSDoc hint that the call to 'new Promise()' produces a 'Promise<void>'": {


I believe this was the message suggested in the issue, so it make sense to use it, but this wording strikes me as off as I'm not sure that we have any error messages that effectively "blame" TypeScript itself for needing a hint about the code's behavior, at least not in this particular wording. I would have written something more like

This promise may need a JSDoc-style type that the call to 'new Promise()' produces a 'Promise'

To shift things around to pointing at the particular code that's at fault, not at the language itself.

But, this is bikeshedding. @DanielRosenwasser @andrewbranch

Yeah, I agree this message doesn’t quite fit. What exactly does the codefix for JS do? Does it add an annotation both for variable declarations and expressions (i.e. a type assertion)?

It adds a type tag:

var a = /** @type {Promise<void>} */(new Promise(resolve => resolve()));

How about

'new Promise()' needs a JSDoc hint to produce a 'resolve' that can be called without arguments.

'resolve' could be substituted with an interpolation to use the parameter name the user wrote.

The first interpolation ({0} arguments) is unnecessary as we know it’s 0.

(cc @DanielRosenwasser)

Nice, this provides better feedback indeed :)

src/compiler/checker.ts

andrewbranch · 2022-04-25T23:59:45Z

src/compiler/diagnosticMessages.json

@@ -3365,6 +3365,10 @@
        "category": "Error",
        "code": 2809
    },
+    "Expected {0} arguments, but got {1}. TypeScript may need a JSDoc hint that the call to 'new Promise()' produces a 'Promise<void>'": {


Yeah, I agree this message doesn’t quite fit. What exactly does the codefix for JS do? Does it add an annotation both for variable declarations and expressions (i.e. a type assertion)?

andrewbranch · 2022-04-26T18:58:17Z

In a separate PR, we should consider expanding the codefix to cover

/** @param {() => void} resolve */
function alwaysResolve(resolve) { resolve() }
new Promise(alwaysResolve);

(both JS and TS versions)

nicdard · 2022-04-26T20:37:34Z

In a separate PR, we should consider expanding the codefix to cover
/** @param {() => void} resolve */
function alwaysResolve(resolve) { resolve() }
new Promise(alwaysResolve);
(both JS and TS versions)

I see, it is complaining indeed now but no code fix is provided, I would be happy to help with this and open another PR later :)

andrewbranch · 2022-04-26T20:40:46Z

Yes. It’s a different error but the same fix can be applied.

nicdard · 2022-04-27T06:48:29Z

src/compiler/checker.ts

-                : Diagnostics.Expected_0_arguments_but_got_1;
+            const isVoidPromiseError = !hasRestParameter && parameterRange === 1 && args.length === 0 && isPromiseResolveArityError(node);
+            if (isVoidPromiseError && isInJSFile(node)) {
+                return getDiagnosticForCallNode(node, Diagnostics.Expected_1_argument_but_got_0_new_Promise_need_a_JSDoc_hint_to_produce_a_0_that_can_be_called_without_arguments, "resolve");


Is there any convenient way to get the name of the function? I tried symbolName(node.symbol) without success

resolveName(node.expression, node.expression.escapedText, SymbolFlags.Value, undefined, undefined, false)?.name

It may also be reasonable just to call it resolve since that’s the parameter name in the library file.

Ok, I will change it to a straightforward string

andrewbranch · 2022-04-27T16:24:16Z

src/compiler/diagnosticMessages.json

@@ -3369,6 +3369,10 @@
        "category": "Error",
        "code": 2809
    },
+    "Expected 1 argument, but got 0. 'new Promise()' need a JSDoc hint to produce a {0} that can be called without arguments.": {


Suggested change

"Expected 1 argument, but got 0. 'new Promise()' need a JSDoc hint to produce a {0} that can be called without arguments.": {

"Expected 1 argument, but got 0. 'new Promise()' needs a JSDoc hint to produce a {0} that can be called without arguments.": {

Oh sorry for this!

…better feedback

andrewbranch

Thank you!

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Apr 3, 2022

nicdard marked this pull request as draft April 3, 2022 10:57

nicdard force-pushed the fix/46570 branch from 9c58309 to dcd8622 Compare April 3, 2022 11:06

nicdard marked this pull request as ready for review April 3, 2022 11:07

nicdard marked this pull request as draft April 3, 2022 11:19

nicdard force-pushed the fix/46570 branch 5 times, most recently from 403227f to d89aac4 Compare April 3, 2022 13:32

nicdard marked this pull request as ready for review April 3, 2022 13:56

nicdard force-pushed the fix/46570 branch from d89aac4 to ad2e50e Compare April 3, 2022 13:58

sandersn assigned andrewbranch Apr 22, 2022

sandersn requested review from andrewbranch and jakebailey April 22, 2022 15:46

jakebailey reviewed Apr 22, 2022

View reviewed changes

andrewbranch changed the title ~~Fix/46570~~ Add JS-specific diagnostic message for resolve() in Promise where type argument can't be inferred Apr 26, 2022

andrewbranch requested changes Apr 26, 2022

View reviewed changes

nicdard force-pushed the fix/46570 branch 2 times, most recently from 49d95c5 to 94dc46b Compare April 26, 2022 18:32

osaimola and others added 2 commits April 27, 2022 01:00

change error message on Promise

3fa0125

fix(46570): Unhelpful Promise type argument hint in JS file

47998e5

nicdard force-pushed the fix/46570 branch 3 times, most recently from f300244 to 8a66d15 Compare April 27, 2022 06:47

nicdard commented Apr 27, 2022

View reviewed changes

andrewbranch reviewed Apr 27, 2022

View reviewed changes

refactor: Reword void Promise message for JSDoc type hint to provide …

8ba5d87

…better feedback

nicdard force-pushed the fix/46570 branch from 8a66d15 to 8ba5d87 Compare April 27, 2022 17:58

andrewbranch approved these changes Apr 27, 2022

View reviewed changes

andrewbranch merged commit d45012c into microsoft:main Apr 27, 2022

nicdard deleted the fix/46570 branch April 27, 2022 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JS-specific diagnostic message for `resolve()` in Promise where type argument can't be inferred #48533

Add JS-specific diagnostic message for `resolve()` in Promise where type argument can't be inferred #48533

nicdard commented Apr 3, 2022 •

edited

Loading

jakebailey Apr 22, 2022

andrewbranch Apr 25, 2022

nicdard Apr 26, 2022 •

edited

Loading

andrewbranch Apr 26, 2022

nicdard Apr 26, 2022

andrewbranch Apr 25, 2022

andrewbranch commented Apr 26, 2022

nicdard commented Apr 26, 2022 •

edited

Loading

andrewbranch commented Apr 26, 2022

nicdard Apr 27, 2022

andrewbranch Apr 27, 2022

andrewbranch Apr 27, 2022

nicdard Apr 27, 2022

andrewbranch Apr 27, 2022

nicdard Apr 27, 2022

andrewbranch left a comment

	"Expected 1 argument, but got 0. 'new Promise()' need a JSDoc hint to produce a {0} that can be called without arguments.": {
	"Expected 1 argument, but got 0. 'new Promise()' needs a JSDoc hint to produce a {0} that can be called without arguments.": {

Add JS-specific diagnostic message for resolve() in Promise where type argument can't be inferred #48533

Add JS-specific diagnostic message for resolve() in Promise where type argument can't be inferred #48533

Conversation

nicdard commented Apr 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicdard Apr 26, 2022 • edited Loading

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 Apr 26, 2022

nicdard commented Apr 26, 2022 • edited Loading

andrewbranch commented Apr 26, 2022

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

Choose a reason for hiding this comment

andrewbranch left a comment

Choose a reason for hiding this comment

Add JS-specific diagnostic message for `resolve()` in Promise where type argument can't be inferred #48533

Add JS-specific diagnostic message for `resolve()` in Promise where type argument can't be inferred #48533

nicdard commented Apr 3, 2022 •

edited

Loading

nicdard Apr 26, 2022 •

edited

Loading

nicdard commented Apr 26, 2022 •

edited

Loading