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? |
|
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 |
|
Two things:
|
|
@fregante I think that For example if full example:async function sleepAndReturn<T>(ms:number, returned:T):Promise<T>{
return new Promise((resolve)=>{
setInterval(function(){
resolve(returned);
},ms)
});
}
async function main(){
sleepAndReturn(100,'X');
const x = await sleepAndReturn(100, 'z');
const y = sleepAndReturn(300,3);
console.log(x,y);
}
main(); |
|
In that case it's not floating, you're assigning a promise to a variable, which is perfectly fine and unlikely to ever be part of TSC. If you're looking for that part, it would however been a decent lint rule, e.g. In reality you could also avoid the issue by using |
|
It seems quite arbitrary to be concerned with ignored returned promises, but not with other ignored returned values. Consider this example: Both the floating promise and this example fall into the error class:
I'm for the There's so many programming errors that this would help avoid Here's another one: |
|
I think ignoring a Promise is not as arbitrary as ignoring regular return values as in many cases, ignoring a promise will unexpectedly skip big chunks of code being executed at all. Take the very common mistake It's very counterintuitive that ignoring the 'return' value of storeInDB means that the code is not even executed, so nothing is actually stored in the db. |
|
There is precedence in other languages to implement such a feature in a checker: https://errorprone.info/bugpattern/CheckReturnValue Note that this checker implements it as opt-in. It doesn't force it on all methods, but only those marked. I have successfully used this feature for methods that were known harmful to ignore the value. To me it makes sense to replicate that in TypeScript as well. |
@mathijs81 If the function that's called returns a function, that the caller is expected to then call, then big chunks of code will not be executed. Non-async functions can still have just as critical side-effects. |
It's not clear what the mistake is here. I guess the programmer is supposed to expect each DB operation to happen in sequence, but as written they will happen in parallel instead. That's actually the issue with ignoring promises. Not that code won't run, but that code won't run at the intended time. |
|
@nth-commit @fregante I think I get your point. Ignoring any return value can be a source of errors.I prefer to be forced to add "something" in this case. Because there are a little few times when I really meant I want to drop. Adding that "something" doesn't hurts and helps a lot. ("something" could be a keyword or a hint or a @-comment). I know some libraries are designed to return something "just in case you need it", well maybe we need to handle that adding some "ignore ignored return in that module". Floating promises can be a source of errors.Is not the same as ignoring return value. If you store a promise in a variable and you don't use it you also have a case of floating promise. That's why we need something different here. You can argue that having unused values is algo a source of errors and controlling that you are safe. Again, comparing that value with a boolean is not using the promise, is using the variable that has the promise. A real case of useYou have this code: async function showData(name:string, user:string){
const data = await getData(name);
const hasHighPermission = checkPermission(user, name, 'high');
if (hasHighPermission) {
display([data.basic, data.high]);
} else {
display([data.basic]);
}
}with Then in a future refactor you make I know, I shouldn't do that. But that things happens Now You can still say. But now TS is checking "always true conditions". Yes, yes. Maybe if you turn on absolutely all possible hints and options I'll catch all the floating promises. But in some legacy or cooperative project you cannot turn on all, because the effort to silence them will be huge. I am still convinced that this is enough important to have its own check |
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: