Add quick fix to add 'void' to Promise resolved without value #40558
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
| }; | ||
|
|
||
| addSourceFile(Harness.Compiler.defaultLibFileName); | ||
| compilationOptions.lib?.forEach(addSourceFile); |
There was a problem hiding this comment.
Suggested change
| compilationOptions.lib?.forEach(addSourceFile); | |
| ts.forEach(compilationOptions.lib, addSourceFile); |
There was a problem hiding this comment.
But why though? The only advantage ts.forEach has is that it shortcuts exit on return, otherwise the native forEach is faster. Also, the original code used lib?.forEach: https://github.com/microsoft/TypeScript/pull/40558/files/e348bc9aa81e2db7fe68e1150b80a1dbd4585a6d#diff-ef3d6509d8056f71e3e2ec4b4c766e6aL346
|
@DanielRosenwasser: @andrewbranch suggested I add a unique error message for this case in checker. The best I've come up with is something like this: Not sure I like the wording though. Any suggestions on a description? |
|
Alternatives:
(2) seems a tad repetitive and I'm worried the |
|
I went with (3), but we can change it later if we come up with something better. |
amcasey
approved these changes
Sep 15, 2020
|
|
||
| function makeChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, span: TextSpan, program: Program, seen?: Set<ParameterDeclaration>) { | ||
| const node = getTokenAtPosition(sourceFile, span.start); | ||
| if (!isIdentifier(node) || !isCallExpression(node.parent) || node.parent.expression !== node || node.parent.arguments.length !== 0) return; |
There was a problem hiding this comment.
Wasn't this checked when the diagnostic was produced?
There was a problem hiding this comment.
Yes, but I'd rather recheck then blindly assert. The the place where we produce this error is so distant from this location, it would be easy for someone to change something and break this. Most of the checks are fairly trivial though.
| changes.insertText(sourceFile, typeArgument.end, needsParens ? ") | void" : " | void"); | ||
| } | ||
| else { | ||
| // make sure the Promise is type is untyped (i.e., `unknown`) |
There was a problem hiding this comment.
Wasn't this checked when the diagnostic was produced?
There was a problem hiding this comment.
Not quite. This error can be produced from the following case, but we can't apply a quick fix to it:
interface Obj {
m(): Promise<number>;
}
const obj: Obj = {
m: () => new Promise(resolve => resolve());
}We don't check the type of the promise in the checker, just the arity of the call to resolve. Adding void to new Promise would result in an error, and I'm not quick-fixing the definition of m on the Obj interface, as it could be code you don't own (such as from an external library), or could have repercussions throughout the codebase as you introduce a void that wasn't previously there. Instead, I check whether the promise is untyped (unknown in TS, any in JS).
|
@DanielRosenwasser: I'd like to get a perf test run in with the new change in checker, but I think this is probably fine to merge. Any perf regression would only be in cases where we are already reporting a diagnostic, so clean code isn't affected. |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 54681bd. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:Comparison Report - master..40558
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a quick fix to add
voidto aPromisethat is resolved without a value, i.e.new Promise(resolve => resolve()).The quick fix is designed to work both in TypeScript, and a few limited cases of JavaScript as well:
TypeScript:
new Promise(resolve => resolve())->new Promise<void>(resolve => resolve())new Promise<number>(resolve => resolve())->new Promise<number | void>(resolve => resolve())new Promise<number | string>(resolve => resolve())->new Promise<number | string | void>(resolve => resolve())new Promise<{ x: number } & { y: number }>(resolve => resolve())->new Promise<({ x: number } & { y: number }) | void>(resolve => resolve())JavaScript:
As there is no way to specify type arguments directly using JSDoc in a JS file, this works by inserting or modifying an
/** @type {} */(...)cast around the call tonew Promise(...):new Promise(resolve => resolve())->/** @type {Promise<void>} */(new Promise(resolve => resolve()))/** @type {Promise<number>} */(new Promise(resolve => resolve()))->/** @type {Promise<number | void>} */(new Promise(resolve => resolve()))/** @type {Promise<number | string>} */(new Promise(resolve => resolve()))->/** @type {Promise<number | string | void>} */(new Promise(resolve => resolve()))/** @type {Promise<{ x: number } & { y: string }>} */(new Promise(resolve => resolve()))->/** @type {Promise<({ x: number } & { y: string }) | void>} */(new Promise(resolve => resolve()))The quick fix is not supported on a
new Promisewith a contextual type in either TS or JS currently, as that could have further-reaching side-effects