async/await: nowait keyword? #13376
Comments
|
I strongly agree with the problem this issue is trying to address. My colleagues and I continue to hit this all the time. (I'd estimate at least once a week.) And it frequently masks bugs. It goes without saying that this only happens on I imagine the specific solution proposed in this issue (of a I'd be a fan of having this check built into the compiler rather than the linter, since it's truly a correctness issue, not a style issue. Thanks to everyone for the consideration! |
|
I have recently started migrating a huge project from promises to async/await. having this feature can really ease such migrations. It's very hard to detect dangling promises until something goes terribly wrong with hours of trying to reproduce the issue only to find a function call was missing |
|
This would be highly valuable but the problem with a However, we can avoid this problem by taking the idea and introducing a flag, To suppress the error, the language could require a type assertion, which would otherwise be meaningless declare function f(): Promise<void>;
// --requireAwait
async function g() {
f(); // Error
f() as {}; // OK
await f(); // OK
const p = f(); // OK
p; // Error
p as {}; // OK
Promise.all([f(), p]); // Error because of .all
await Promise.all([f(), p]); // OK
return f(); // OK
}There are some issues with this idea though.
|
|
@aluanhaddad I like that idea. Since typescript is going to introduce decorators like Edit: or maybe opt-out with a flag: |
|
@alitaheri that would be much cleaner. Did you mean |
|
yes, I had forgotten that it was a bit different. Thanks for the reminder 😅 😅 |
|
@aluanhaddad: I love your proposal. My colleague @ryanwe has had a similar thought: this issue can be caught when a caller doesn't make use of a returned Promise. I don't feel like the issues you list are a big deal. To me, it's a major step forward for the compiler to help guard against the 90% bug case, even if it doesn't address the last 10%. (The linter can help w/ that last 10%, e.g. @alitaheri: I might not understand your proposal. Where would I put this |
RyanCavanaugh
added
In Discussion
|
There's no need for |
|
@RyanCavanaugh I think you are missing a key point. The problem is not about whether the promise gets executed. The problem is that the writer forgets to put the "await" in front of a call to an async method/function and gets a behavior that he/she is not expecting. So I believe this is a compiler warning/error issue only -- not a functional one. I'd be fine if this was "opt-in" in the sense that one would have to choose to turn on a compiler option to get warnings about calls to methods returning promises inside async functions that are unused. If using this compiler option, the 'nowait' keyword could be used if the writer has the (rare) need to invoke the method but not wait for the result. |
|
@kduffie I think what @RyanCavanaugh is saying is that a flag is sufficient and that no new syntax is required. That is a great thing. I don't think he is dismissing the issue as it is now marked as in discussion. |
|
The funny thing my initial type level syntax suggestion, was going to involve asserting that the type of the expression was async function f() {
returnsPromise() as void;
}which is a type error. async function f() {
void returnsPromise();
}Which is far more elegant and is a JavaScript construct 😅 |
|
Ah. I see. Sorry for the confusion. He's saying that "void" would tell the compiler that you understood that promise returned is intentionally unused. With the new compiler flag present, one would get an error if there is neither an await nor a "void". Got it. Just fine with me. |
|
@kduffie I believe so. I think that would be a practical solution, but we will see. |
|
Chatted with @rbuckton about this and his take was that we could enforce this rule in The "in getSomePromise().then(() => whatver());would be incorrectly flagged as an error because |
|
And yes as usual @aluanhaddad has cleared up my terse comments with precision and insight |
RyanCavanaugh
added
Help Wanted
|
Approved as the default behavior (i.e. no flag):
Workarounds would be e.g. If anyone thinks there are good examples of functions that return Promises where this would be burdensome, please speak up now with specifics! Edit: Improved clarity of rule |
|
can believe its for spproved for asyncs/promises only |
|
I think this is a fairly common pattern for firing off a bunch of async work at once and then awaiting in one spot. declare function fetchUser(): Promise<any>;
declare function fetchAccount(): Promise<any>;
async function loadStuff() {
const userPromise = fetchUser();
const accountPromise = fetchAccount();
const [user, account] = await Promise.all([userPromise, accountPromise]);
//do stuff
} |
|
the example @jwbay mentioned would still be allowed. the ones that would be flagged as errors are expression statements. e.g. async function loadStuff() {
fetchUser(); // error, did you forget await?
const accountPromise = fetchAccount(); // OK
} |
|
I tried the new no-floating-promises rule in tslint 4.4 and it works great! EXCEPT that I'm using VisualStudio code and its tslint integration won't show this error because this rule requires type checking and it appears that that isn't supported. Anyone know if there are plans to fix VSCode so that it can handle type checking rules? |
|
@kduffie we're working on an extensibility model that will allow TSLint to report errors in editors |
|
tslint already shows errors in the Problems window in vscode (using the tslint extension) but I'm now seeing the following -- which I assume is a limitation of the tslint extension. Is that what you mean when you say, "... an extensibility model"? vscode-tslint: 'no-floating-promises requires type checking' while validating: /Users/kduffie/git/kai/ts_modules/task-helper.ts |
|
I asked for this ages ago and was told to use a linter for it instead... At the end of my issue I said "linting works for us", but that was a lie. How can I use a linter for it when the linter cannot tell that the return type of an arbitrary functions is a promise or not, whereas the typescript compiler can? |
|
I just tried implementing a simple version of this to see what breaks, and I'm not quite clear on what rule is both correct and consistent. I started with the rule "A statement expression that is not an assignment expression must not have a type that is an instance of the global Promise type", which seems to be the sort of error people are making. That does the right thing in the straightforward case: declare function doThing(): Promise<string>;
async function f() {
// Should error
doThing();
~~~~~~~~~~
!!! error TS7062: Unawaited Promise
let m: Promise<string>;
// Should not error
m = doThing();
// Should not error
void doThing();
}However, declare function bar(): Promise<string>;
async function foo() {
// Queue up some work
bar().then(() => {
console.log("Job's done");
});
}Other code also appears wrong despite being fairly idiomatic, e.g. this is technically an unawaited promise: async function foo() {
(async function() {
// Do some deferred work
})();
}What have people mostly run into? Is the idea to just restrict this check to function and method calls occurring at the top level of a statement (unless the method is a method of |
|
The biggest problem I think to address is when developers execute a promise (whether intentionally or accidentally), but forget to await it and do something with the results. Skimming through code it's easy to miss an un-awaited promise, as it can be easily mistaken for a synchronous operation. So in your examples, simply adding the P.S. I would go a little further with this: a promise shouldn't be used in a truthy context as it's almost certainly a mistake and the coder probably intended for it to resolve to a value: That would cover all the common pitfalls we have using promises in async functions. |
|
We have a large Typescript nodejs project and depend heavily on these ESLINT rules to catch errors. The most common errors happen when an "await" is forgotten in more complex statements, such as within predicate clauses of conditional statements. But without these checks, we would occasionally just fail to include an await on a simple statement invoking an asynchronous function or method. We use "void" instead of "await" whenever we intentionally want a separate "thread" to continue without waiting for it to complete. That convention has worked really well for us. |
|
Are people really wanting an error on this block, though? It seems very superfluous to have to use async function foo() {
// Queue up some work
bar().then(() => {
// do some other stuff
console.log("Job's done");
});
// ^ error, floating promise created by `then` call
} |
@jpike88 This already is an error. Can you clarify? |
|
@RyanCavanaugh It seems perverse to mark the function as "async" and then use this construct. The whole point of an async function is to allow you to write it as: For that reason, if you actually write I agree with you that a promise should never be implicitly used as truthy. You can always be explicit, ie., |
But are unused Promises in non-async functions OK, then? It seems weird that we would say that this is an error async function foo() {
doSomeSetup(); // <- ⚠️actually async!
doSomethingThatDependsOnSetup();
}but not this function foo() {
doSomeSetup(); // <- ⚠️actually async!
doSomethingThatDependsOnSetup();
}when the motivating scenario for the feature is the "didn't realize the function was async" failure mode, which is just as much present in non-async functions. |
|
That's true. In our situation, almost every method/function is async. The two most common mistakes are forgetting to add the async keyword to a function/method declaration or to forget to add the await keyword in front of a method/function call (whether that is part of a compound statement or not. For us, these are the most critical to detect. So for me, in either case (async function or not), I would want it to be an error to invoke a method that returns a promise without using either "void" or "await" in front of it. (I acknowledge that the use of "void" is just syntactic sugar, but the whole point of Typescript is to allow the syntax to be rigorous.) |
Hi Ryan. I'm very happy with you getting this. Yes, I already have a lot of old code written in that way. 99% of the times when I write async function foo() {
// Queue up some work
bar().then(() => {
// do some other stuff
console.log("Job's done");
});
// ^ error, floating promise created by `then` call
}I forgot the In less of 1% of the cases I realy want a floating promise. Then I prefer that the TS compiler always complain about unasigned o unreturned |
|
In my opinion the complain about floating promise must be inside async and not async functions and functions that returns Promises and function that returns any other type or functions that returns void. I.E. all functions. |
It provides consistency to the code. If a promise can have either nothing,
The below code doesn't flag anything in vscode, this is what I mean by a promise being used in an if statement without being awaited or voided. |
|
In my opinion this can be achieved with The |
|
@emilioplatzer I disagree, I am for a core type checking rule that assumes that all Promise/async calls are assumed to be |
|
@jpike88 I agree with you. In my opinion this must be addressed in the type system. The importance of detected not awaited promises is that in the 99% of the cases that is an error (the omission was not intentional). I also have the same opinion about function dividie(a:number, b:number):NonDiscard<{result:number, error:string}> ...and that Ditto that. I realize that this |
|
Breaks, part 1: #53146 (comment) Even for something being considered as an opt-in, this is pretty bad as far as results go. Puppeteer in particular has dozens of errors, of which none of them appear to be legitimate. Probably more work needs to be done here to figure out a rule that doesn't have a high false positive rate, or make the case that at least one of the hits in that PR are good finds. I think it's a hard sell that webpack has dozens of code smells. On the plus side, perf was not meaningfully impacted. |
|
FWIW I am familiar with the Puppeteer codebase and it doesn't surprise me that it would find a lot of issues. Floating promises are a major painpoint for DevTools and Puppeteer which adopted Promises in the same era. Therefore, I wouldn't immediately write them all off as false positives. That said, it is quite a list and it probably needs extensive analysis which are legitimately floating and which aren't. |
|
@RyanCavanaugh I personally think by skimming through the examples you provided, they are coding in much lower level styles in which case they shouldn't be opting in to something like this. I would also call looping over a lot of floating promises, or firing off a lot of floating promises in succession, a code smell. Promise.all exists for a reason! I wouldn't consider anyway your findings to be a high false positive rate, quite a few of these files are for typing purposes only, and aren't worth trying to pore over due to their inapplicability to any practical case that wouldn't drive someone mad trying to figure out. I think the best way to treat this is that it's OK and perfectly legitimate (rarely!) to be firing promises off without directly handling their results in the same 'thread', but TypeScript should give you a way to distinguish between a promise that you are floating either by accident or on purpose, which currently is impossible to tell as no visual markers are enforced to qualify the intention of the Promise usage. So in other words, those library maintainers if they choose to opt-in can just place the |
|
I expect that in the current state of my codebase all of the floating promises that you can find are legitims (thanks for our testers). But I'm sure that all of the floating promises that you can find in the diffs of all commits in my repository are buggy ones. I was thinking about how to indicate that a promise is not floating. I can use // @ts-expect-error: letting float a promise on purpose
function letItFloat(Promise<any> _) {}And use it like: async function runForever() {
while (var x = await producer()) {
await consumer(x)
}
}
letItFloat(runForever());Tomorrow I can replace tsconfigAnd maybe this feature can be turned on in tsconfig and being false by default. |
|
We just ran into a problem again where a un-awaited promise was pushed to production (but managed to slip through automated and QA tests due to timing differences), which would have resulted in a very nasty, hard to diagnose bug. I only caught it because I decided to pore over the closed PRs a second time out of boredom. If discussion around this could intensify until a resolution is reached that would be ideal. |
Detection of floating promises is probably my most wanted feature. I very often forget to await a promise and tracing down these kind of bugs without tooling can be difficult to spot. It's true there is a high false positive rate for this one, but I think it's good practice to explicitly opt-out of false positives to make it clear that was intended to the reader. For example, the C# compiler warns when not awaiting or assigning a task and I've always found that super helpful. Ability for API authors to supress the errorPerhaps for APIs that are meant to be "fire and forget" and optionally awaited there could be a way for API authors to define that in order to supress this error for API consumers. For example: async function serve1(handler: ...): Promise<void> {
// ...
}
async function serve2(handler: ...): Floatable<Promise<void>> {
// ...
}
// error, unawaited promise (add `void` to supress)
serve1(() => { ... });
// no error, promise is floatable
serve2(() => { ... });...where Calling Async in SyncRegarding calling an async function in a sync function: yes, I believe this should error similarly to how it should in a top level module statement. It's super rare to call an async function from a sync one. I very commonly do this by accident. I'd rather be verbose opting out of false positives with a |
or add error type to Promise and make |
and others
I have a large project that has now been migrated over from javascript to typescript and we're very pleased with the results. Bugs have fallen dramatically with the improved compiler support in combination with rigorously written code.
Ours is a server-side product that runs under node (ts-node, actually). By its nature, it is highly asynchronous, so the move to Typescript had the major benefit of providing us access to async/await. It has been working really well, and dramatically reduced our code size and complexity.
The one problem it has introduced is that we occasionally miss putting an "await" keyword in front of calls to methods returning promises (i.e., other async functions) when inside an async function. This "send and forget" is something that one occasionally might want to use, but for the most part, this creates a lot of chaos in our design pattern, as in most cases, the call is to something that will complete asynchronously, but most of the time we need to wait for that to complete. (That method cannot be synchronous because it depends on external services that are asynchronous.) It is not uncommon in our case to have 5 or 10 await statements in a single method.
A missing "await" keyword can be hard to find -- especially when the method being called doesn't return anything that is needed by the caller. (If it returns something, type-checking will usually catch the mismatch between the type of the Promise returned from the call.)
Ideally, we could have a "nowait" keyword that would go where one would otherwise put an "await" to indicate to the Typescript compiler that the design intent is to explicitly NOT WAIT for the called method to be completing. The tsc compiler could have a new flag that controls whether to create a warning if a method call returns a Promise that is missing either "await" or "nowait". The "nowait" keyword would have no effect whatsoever on the compiled code.
The text was updated successfully, but these errors were encountered: