New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove no-return-await as it sets traps during code refactoring and causes inferior stack traces #1442
Comments
I value your input, @CherryDT. So, your claim is that the Your examples make sense to me. Personally, when I am using TypeScript, I am protected from that kind of refactoring errors. But that's just a side note. Is there discussion in ESLint about the usefulness of I feel that what @ilyavolodin suggested is reasonable and would allow us in Standard to choose the opposite. Rename the rule to allow the opposite or make a new rule that does the opposite. @CherryDT would you be interested in making that contribution? That would allow you to enjoy your preferred style regardless of what Standard ends up deciding. And I would appreciate input from more users on this ❤️ . |
Sounds very reasonable, it's not fun to lose the frame in the stack trace 👍 |
@mightyiam Unfortunately I'm not familiar with how ESLint rules work internally, I'm not sure right now how to make said contribution. But nonetheless, I appreciate your open ear on this. Did I understand it correctly in case you would actually reconsider this rule in the Standard, that you would then only consider switching to the opposite rule (hence the need for it to be exist), but not consider merely removing the existing rule from the ruleset? (Oh and just out of interest - does TypeScript also prevent the try/catch trap?) |
Since we have no implementation of an alternative and since the rule seems to me to be more detriment than use, I would have it removed. |
Contrary to ESLint Documentation, returning await has valid use cases. By explicitly marking that the return value needs to be awaited, code is more clear and easier to refactor. See standard/standard#1442
May I ask what the status on this is? I'm just confused whether it is now decided that it'll be removed or not, since @mightyiam wrote it should be removed after all but that was already almost half a year ago. Thank you! |
There is some discussion about this in a few places, e.g. mightyiam/eslint-config-love#206, eslint/eslint#12246, typescript-eslint/typescript-eslint#1378 The more I think of it the more I'm leaning towards:
One of the last arguments was that that part of the stack frame can be a "red herring", but I really don't see how that is? When would it not be useful to see the full call-chain that led up the failing call? In fact, I see the opposite of this all the time in Node.js. You get an error that just says "ENOENT", but you don't see that it was the function @mightyiam how do you feel about this? @feross it would also be great if you could chime in with your thoughts! ❤️ |
@LinusU thank you for the input. I am not a big async function user. So I'll wait for more input from others. |
If it requires a node flag to get a benefit from |
@ljharb this is secondary. It doesn't require a node flag to mess up your refactoring. |
I'm not aware that you need a specific flag in the newer versions of Node.js? In fact, as far as I know, it's supported in every currently active release (>= 10.13.0) Or am I missing something? 🤔 |
@CherryDT what i mean is, |
The better stack traces are a nice side effect but the major issue is that it covers up what the code is actually doing, namely waiting for another asynchronous result. Hence all my examples about how it hinders readability and adds traps for later modifications. My point is that it is bad practice to encourage that. I and my colleagues have wasted several hours of our lives due to traps created by this rule. That can't be the goal... There are many other rules following the philosophy of making the code "normalized" and easier to read and less likely to break on changes (to code or environment), so why should this one actually push into the opposite direction? For example, there is (In the path case, it would break on another platform, and in the error case it would break when someone makes the assumption that errors have a property Correct me if I'm wrong, but I thought the goal of StandardJS was to eventually save time developing, increase code clarity and avoid common pitfalls (such as these refactoring issues) - not micro-optimize code. For example, the extra parentheses in I quote:
(Empasis mine.) Maybe I should edit this issue to reorder my points, since actually point 2 is the major issue, not point 1. I don't understand why everyone is arguing about "stack traces for one engine" (here and at eslint) when the main problem is another one. |
It's an async function, that's what makes it wait for an async result. The extra When I say "slower", i don't mean "takes a few extra ms", i mean "takes another tick" which might be seconds, depending on your app. |
That may be technically right but logically wrong. This is not how humans think. Again, the standard should aid humans, not computers. When I'm modifying one line of code I'm not always scrolling up and down and reading the whole rest of the file in detail. Plus, here the point is not that it covers up the async function's job, but the fact that the thing I'm returning is also an async function call so it requires special care upon almost any modification, even if it's done at a totally different point in my function (such as wrapping a piece of code in
No it doesn't take another tick. node: Chrome (assuming that It comes actually at the end of the current tick (after the other synchronous code, but before going idle - not even the next tick, if you look closely!) and not the next-next-next-next-next tick despite my ridiculous use of multiple |
It's how this human thinks, and how the JavaScript language works. Something that encourages an incorrect understanding of what the language does is worse for humans, even if it appears to be better for some humans in the short term. You're right, "tick" is the wrong term. It's a new microtask, though. |
Sorry, I didn't mean to call you not human. That didn't come out right. Anyway, it seems you are not addressing any of the code clarity and refactoring-traps issues that I mentioned, from what I understand your point is that it is more important to work "low-level" than to create maintainable code. In this case let's just agree to disagree because I'm not sure what to respond to that. |
From my perspective, code is clearer when it avoids the unnecessary |
For the record (and for anyone interested): I wanted to get to the bottom of how much it affects the performance. Using a native debugger I found that it creates the same amount of microtasks either way, but I'm not sure about how reliable my findings there are because I simply put breakpoints on all symbols in node that contained anything related to "enqueue microtask" and counted how often they were hit. I then also checked async hooks and how often they fire. The result is that they fire the same amount of times, just the order in which things execute is a tad different: Then I tried to actually time the execution. The method is a bit crude (I tried to make sure that I really catch the relevant part only, that's why it times until the last So, yes, the extra I also verified that I'm not measuring some sort of sleeping time by accident, by running twice the amount of iterations, and yes the numbers are also around twice as big: |
@mightyiam in node and v8, sure, but standard applies to code that's used in other engines. I'd want to see results in a lot more of them before conceding a performant argument. However, I don't think perf actually matters here; i think |
I still don't see how it is "incorrect code" though.
|
Sure, i agree - and you should use |
Hmm, I don't really see how it "incorrect"? and it isn't really "redundant" since the behaviour is different.
This has been brought up in another thread about this, but this isn't because of one JS engine. I don't see how one engine could choose to add a function that isn't running anymore to the stack trace?
Not really though, if you return a Promise from an async function it doesn't wait for that result. It immediately returns that promise and the function is no no longer executing. This is why it would also be wrong for a specific engine to keep the function on the stack trace, and why
I really don't understand why this would give an incorrect understanding of the language. To me it seems like the root of the problem here is that Promises flat maps automatically when resolved with another promise. This is convenient, but this is really what hides what is happening here. Without this feature return a promise without doing In e.g. Rust this is very nice because you can see this very clearly, and thus it requires you to do their equivalent of
I really agree with this, an extra await is not going to be your performance bottleneck, and we are optimising for code readability and correctness firstly.
I really don't think that it will confuse people though 🤔 As I said earlier, I feel that the thing confusing people is really the automagic-flat-mapping... I still strongly feel that we should remove |
(@ljharb - First: I respect your opinion. I just think that "incorrect" is a very absolute statement that goes beyond opinions, and I think that the way you view the language and how to work with it - which is totally valid as everyone can build the mental model that works best for them - will for most other people lead to worse code quality, hence I think it should not be advocated by default. Of course I cannot really be sure about what "most other people" think, but that's why we are discussing it here. I thought your perspective was that you ignore the refactoring and readability issues because the "language purity" has higher priority for you - that's why I said to agree to disagree originally - but now I understand that you don't view those issues as issues at all because they are not issues for you. I personally believe that they would be issues for most other people though, that's why I continue to elaborate.)
My point was that the purpose does exist: The purpose is to do exactly that, logically keep the awaiting inside the function - both for computers (stack issue - which could even be relevant to code in some edge cases, e.g. profiling, since the stack can accessed programmatically) and humans (refactoring issue). Definition of "return*" for further use: From a callee's perspective, it is what is passed to Logically, these are different function types for me:
...even though technically the middle two happen to be the same. I view this as a consequence of "the concept of In other languages, the concept of Therefore, I think, leaving out the It's just like how the I see it as an implementation detail that it actually works without the In fact I think the current implementation in JS is actually a bit counterintuitive, because it is impossible to actually return* a pending promise from an async function. (This is the gotcha I meant to keep in mind above.) But, this is much easier to keep in mind and you much more rarely bump into it as a limitation (and then you can use EDIT: I just thought about another example to maybe illustrate my point more clearly. This example does not use async function itself, it's about the abstract idea I'm trying to advocate here. Consider this code (based on real-life examples): // Let's assume `req.query` is a parsed URL query string
// Let's also assume that `data` is an array with items.
const PAGE_SIZE = 10
const page = req.query.page
const items = data.slice((page - 1) * PAGE_SIZE, PAGE_SIZE)
const totalPages = Math.floor(data.length / PAGE_SIZE)
return {
items,
totalPages,
description: `Page ${page} of ${totalPages}`
} In my eyes, this code is bad and has a hidden trap. As mentioned in my comment, I would therefore change it to: const page = Number(req.query.page) Now, you could argue that it doesn't matter, since However, I disagree. It has essentially the same benefits as in my a) Benefit for tooling - here it's not a stack trace, but it would make IntelliSense-type tools work better because they would know for sure that b) Benefit of preventing refactoring traps - see this example where later on, we add more pagination output (again, from real life): return {
items,
totalPages,
prevPage: page > 1 ? page - 1 : null,
nextPage: page < totalPages ? page + 1 : null,
description: `Page ${page} of ${totalPages}`
} Do you spot the problem? If the URL has result = {
items: [...],
totalPages: 5,
prevPage: 2,
nextPage: '31', // oops
description: 'Page 3 of 5'
} So, why not prevent this possible future coding mistake proactively in the first place? And in this example it was easy to spot during a first test. But now think about what will happen if it's not Yes I know that TypeScript would have solved this as well, but that's not the point. Bottom line: Code should convey intent. |
I also don't see where this argument comes from. I posted a comment at typescript-eslint/typescript-eslint#1378 (comment) with another real life example where the missing frame would have pointed to the culprit. |
@LinusU the promise returned from an Async function is never the same promise you eventually return from it; the spec requires it have a different identity. Separately, you can never have a promise of a promise, it’s not a monad. I’m seeing arguments here in favor of return await that directly contradict how the language is specified, so i hope my point that allowing it increases confusion comes across. |
@CherryDT note that the concept of await wasn’t implemented using promises, it is promises. Promises aren’t an implementation detail that you can ever avoid thinking about - async/await simply does not replace the need to directly use, and understand, promises. Why do you think we keep adding new Promise combinators to the spec, and have no await syntax for any of them? |
I don't think so. Promises in the way we were using this word in this thread, more specifically ECMAScript Promise Objects (or earlier, with differences that are actually not overlapping the point of this discussion, Promises/A+) are a JS-specific interface of the much broader language-independent promise/future/deferred concept. async/await is a syntax construct to create synchronous-looking code that is actually asynchronous, and this is also a language-independent programming concept, much like an array or a class or a The "auto-flattening" is convenient for the promise/feature/deferred concept, but counter-productive for the async/await concept. But for neither it is part of the concept itself, just an implementation decision. For me, the concept of async/await is definable like this:
Note that this concept doesn't describe how to interact with
In a nutshell:
This concept maps almost perfectly to JS's implementation, except for the behavior that we are discussing now. Now that it is clear why async/await (and the generic concept of a Future) are its own thing, a concept separate of ECMAScript Promise Objects, let's look at how they overlap in JS. async/await in JavaScript has inherited some of the semantics of ECMAScript Promise Objects because the existing promises were probably the obvious choice of implementation, like in most languages, especially since promises were already very popular and async/await now allowed for a new, neat way of writing code that interacts with the existing ECMAScript Promise Objects-based ecosystem. Not all futures/promises are self-flattening in other languages. They just happen to be in ECMAScript Promise Objects which is what JS happened to implement into the language, and I'm quite sure that async/await was not on the table when ECMAScript Promise Objects came about. In other words: If the concept of promises/futures is telephony, and the concept of async/await is mobile telephony, then ECMAScript Promise Objects would be the US numbering plan where numbers are given on a strictly geographic basis (since it didn't matter to the idea of telephony and "having a unique number" itself but was useful at that time). Nowadays, the fact that mobile phone numbers have to belong to the owner's home's geographical region is certainly not part of the idea of mobile telephony and has become actually inconvenient (you'd have to change the number when moving even though there is no need anymore due to physical wiring) and useless (since it can no longer tell you to which physical location you are currently calling). (At least that's what I understand about how things are in the US - I hope my example is correct.) This can be compared to how ES6 class X {
constructor () {
return { a: 1 }
}
xyz () {}
}
const x = new X()
console.log(x instanceof X) // false !
console.log(x.a) // 1
console.log(x.xyz) // undefined ! ...which is a result of how the concept of a "class" was implemented in JS, namely by putting syntactic sugar on the existing prototype system and constructor semantics (which was not designed with this future usage in mind). It doesn't mean this is how classes should work ideally, so I'm happy if my linter tells me I created "surprising" code, just like with the Now that I've spent some time to self-analyze how my mental model of async/await as a language-independent concept exactly looks, I'd be very curious to know if others have the same mental model. I know you, @ljharb, don't, but I wonder how others see that! It is hard to find definite descriptions of concepts like this, since they are not standardized but just evolved. |
imo the JS implementation is the only concept that matters to JS devs; the long tail of JS devs likely have never written another language. Promises/A+ was useful until Promises were added to the spec, at which point it's no longer relevant except as historical background. |
You are right. My mistake. I changed my post accordingly. It doesn't change anything though, since the flattening behavior is the same in Promises/A+ and ECMAScript Promise Objects. And you are also right regarding JS devs, but I think that ultimately programmers get to understand concepts, ideally. (Obviously, you have to understand the implementation in your current language/framework/etc. as well, but you should be aware what is part of the concept and what is not.) |
Would you mind elaborating on this? 🤔 |
@LinusU see the rest of that comment for examples. |
Do you mean this part?
I'm not sure how that affects the arguments 🤔 |
This is false. I seem to have slightly misread part of your comment; you weren't claiming you can have a Promise of a Promise, but contrasting JS to a language where that is possible.
None the less, that is the semantic the language has, and |
... and other than this secondary usefulness, the primary extra value of not provoking a number of foreseeable developer errors in the future... which is independent of any implementation or standard and the main point of my issue |
@ljharb why is this? 🤔 do you have any links where I can read up on this? |
Because that’s the way the spec works? An async function produces a new promise and returns it at the first |
Ah, okay, I understand how you mean. What I was trying to convey was that the promise of the async function will be settled with the new promise that is being returned, and the async function will no longer be executing even though the final promise hasn't settled yet. async function sleep (ms) {
await new Promise((resolve) => setTimeout(resolve, ms))
}
async function example () {
return sleep(1000)
}
const a = example()
// the "example" function won't be on the stack here
await a
I'm not sure this is correct, but I might be misinterpreting what you are trying to say here. If it resolves the value before returning, than the following code should catch the rejection, right? async function throwLater () {
await new Promise((resolve) => setTimeout(resolve, 100))
throw new Error('Test')
}
async function example () {
try {
return throwLater()
} catch () {
return 1337
}
}
// This will actually reject
assert(await example() === 1337) I think that just the example above is justification enough for always doing |
Certainly if you're returning from an |
It's not in the standard though (for JS), and I doubt there can be rules that catch the other cases I described above unless the code is TypeScript. So again it should be at the developer's discretion in this case... |
Well, I have to remember that I need to add it inside the try, which is very easy to forget if the linter normally forces you to not add it. I've actually seen this caught in code review more than once, people forgetting to add I still strongly feel that we should:
|
Fixed in 14.3.4 |
What version of this package are you using?
14.3.1
What problem do you want to solve?
The
no-return-await
rule introduced in #695 is the only one with which I continue to disagree, and I think since the arrival of async stack traces in Node, there is now another reason to rethink it.Therefore I want to open a discussion here. My point is that the
no-return-await
rule (which disallows "redundant" use ofreturn await promise
) has two negative effects:1) It reduces the usefulness of error stack traces
Node now has zero-cost async stack traces with the
--async-stack-traces
flag. There, it makes a difference whetherreturn promise
orreturn await promise
is used - the former omits a stack frame.Example:
Result:
Note how there is no stack frame for
a
.2) It lays out a trap for code refactoring by making it easy to overlook that an async function is being called
Example with subtle unintended behavior:
Example with more obvious unintended behavior:
Another example:
Or even:
There are many more examples like this which don't immediately crash (it is easy to spot something like
somethingAsync().toUpperCase() is not a function
, but my examples above are more subtle). If I modify existing code, I may not be familiar with every function that is being called (and I may even use a refactor tool where I just select the returned value and extract to a variable for example). Usually, I'd easily recognize thatawait somethingAsync()
should probably stay this way, but withreturn somethingAsync()
I may not realize that in any place other than thereturn
statement, I'd need anawait
in front (and even if it stays in areturn
, other things like the aforementionedtry
block may suddenly require a change here - and imagine that thereturn
is somewhere inside a larger block of code which I'm now wrapping withtry
)!I had all of these issues from my examples happen in real life.
To work around this issue, I manually mark
return somethingAsync()
with a comment, e.g.return somethingAsync() // async
but that seems ugly and much less useful than having the obviousawait
there (plus, it doesn't solve thetry
case).What do you think?
What do you think is the correct solution to this problem?
Remove the
no-return-await
ruleAre you willing to submit a pull request to implement this change?
I guess so...? (but I guess it would be easier for you)
The text was updated successfully, but these errors were encountered: