[Testing] If I remove thisif and just leave its body, all unit tests still pass. That means there's at least a missing test case.

This is still the case.

Added a test but it doesn't matter:

declareletfoo:()=>void;foo+=()=>1;

still doesn't fail with condition removed.

Sounds to me like theif can be removed, then! 🔪

Change request: remove theif?

Just noting for posterity,node.operator in anAssignmentExpression can be any of:

"="|"+="|"-="|"*="|"**="|"/="|"%="|"<<="|">>="|">>>="|"&="|"|="|"||="|"&&="|"??="|"^="

The operators not mentioned in theif all produce TypeScript errors when given functions. So I agree with adding thevalid test case containingfoo += () => 1. That makes sure the removal of theif doesn't break those cases.

At this point this rule already did everythingno-misused-promises'scheckVoidReturn did, but better. It doesn't have problems#8054 or#8739. Maybe it's worth splittingno-misused-promises into 3 separate rules in the future? (this one being one of them)

This is a great question.no-misused-promises was already largely overlapped byno-unnecessary-condition. This newstrict-void-return pretty much takes on the rest ofno-misused-promises, makingno-misused-promises redundant if you have bothno-unnecessary-condition andstrict-void-return...

I'd be in favor of deprecatedno-misused-promises in favor of usingno-unnecessary-condition +strict-void-return. The only benefit I can think of forno-misused-promises would be projects that want toonly apply the checks for Promises... Maybe these two rules could each be given some kind of"only check Promises" option?

Also of note is thatno-misused-promises'scheckVoidReturns is pretty configurable. Maybe, if this rule is to replaceno-misused-promises, it'd be useful to have each of those configurable options? Or, on the other hand, maybe those options are holdovers that real-world don't generally use? Investigation needed. I think those options can be a followup & shouldn't block this PR.

What do you think?

Also cc: @typescript-eslint/triage-team in general, and@kirkwaiblinger +@alythobani from#8765.

no-misused-promises was already largely overlapped by no-unnecessary-condition. This new strict-void-return pretty much takes on the rest of no-misused-promises, making no-misused-promises redundant if you have both no-unnecessary-condition and strict-void-return...

Yeah the only thing left I think would be thechecksSpreads option:

constmyPromise=Promise.resolve({num:2,str:"2"});constmyObject={...myPromise};// Expected a non-Promise value to be spreaded in an object. eslint(@typescript-eslint/no-misused-promises)

I do agree it could make sense to replacechecksVoidReturn withstrict-void-return. Although there may be tradeoffs in terms of eng effort and/or UX complexity if we wanted to retain all the configurability on top of having anonlyChecksPromises option.

As forchecksConditionals, I actually just foundmicrosoft/TypeScript#34717 andmicrosoft/TypeScript#39175—looks likecheckConditionals has been covered by TypeScript for a couple years now :)

The only benefit I can think of for no-misused-promises would be projects that want to only apply the checks for Promises

Yeah e.g. one example I've seen when looking into this topic (void function assignability), isusingpush withforEach:

declarefunctionforEach<T>(arr:T[],callback:(el:T)=>void):void;lettarget:number[]=[];forEach([1,2,3],el=>target.push(el));// OK

It's possible some users would prefer to only check Promises so they can still use shorthands like the above without linter errors (and/or just mainly care about forgetting toawait Promises), in which case anonlyChecksPromises option would be useful if we did replacechecksVoidReturn withstrict-void-return.

Maybe, if this rule is to replace no-misused-promises, it'd be useful to have each of those configurable options? Or, on the other hand, maybe those options are holdovers that real-world don't generally use?

It looks like#4619 was originally the impetus for adding the options (#4623); and based on the thread it looks like there are at least a few people who find the configurability you added very helpful!

Maybe a lame response, but is there a compelling reason not to land this first, then consider the no-misused-promises deprecation, and which options we might need to port or create in order to do so, afterwards?

Just thinking, deprecating no-misused-promises might have some strings attached, such as some nontrivial updating of the docs in no-floating-promises that explain how to lint against promise antipatterns outside of ExpressionStatements.

Agreed with landing this first, then considering a deprecation as a followup.

In fact, this rule is pretty big and scary. We don't really have a process for declaring rules as "canary" or "experimental".#8676 is the closest we have to a feature request. Maybe we should set a precedent?

(I don't think this PR should be blocked on that)

no-misused-promises was already largely overlapped by no-unnecessary-condition. This new strict-void-return pretty much takes on the rest of no-misused-promises, making no-misused-promises redundant if you have both no-unnecessary-condition and strict-void-return...

Yeah the only thing left I think would be thechecksSpreads option:

constmyPromise=Promise.resolve({num:2,str:"2"});constmyObject={...myPromise};// Expected a non-Promise value to be spreaded in an object. eslint(@typescript-eslint/no-misused-promises)

I do agree it could make sense to replacechecksVoidReturn withstrict-void-return. Although there may be tradeoffs in terms of eng effort and/or UX complexity if we wanted to retain all the configurability on top of having anonlyChecksPromises option.

As forchecksConditionals, I actually just foundmicrosoft/TypeScript#34717 andmicrosoft/TypeScript#39175—looks likecheckConditionals has been covered by TypeScript for a couple years now :)

The only benefit I can think of for no-misused-promises would be projects that want to only apply the checks for Promises

Yeah e.g. one example I've seen when looking into this topic (void function assignability), isusingpush withforEach:

declarefunctionforEach<T>(arr:T[],callback:(el:T)=>void):void;lettarget:number[]=[];forEach([1,2,3],el=>target.push(el));// OK

It's possible some users would prefer to only check Promises so they can still use shorthands like the above without linter errors (and/or just mainly care about forgetting toawait Promises), in which case anonlyChecksPromises option would be useful if we did replacechecksVoidReturn withstrict-void-return.

Maybe, if this rule is to replace no-misused-promises, it'd be useful to have each of those configurable options? Or, on the other hand, maybe those options are holdovers that real-world don't generally use?

It looks like#4619 was originally the impetus for adding the options (#4623); and based on the thread it looks like there are at least a few people who find the configurability you added very helpful!

Maybe a lame response, but is there a compelling reason not to land this first, then consider the no-misused-promises deprecation, and which options we might need to port or create in order to do so, afterwards?

Just thinking, deprecating no-misused-promises might have some strings attached, such as some nontrivial updating of the docs in no-floating-promises that explain how to lint against promise antipatterns outside of ExpressionStatements.

[Testing] This isn't covered by any tests - either it's a valid case that should be tested, or it's not and we can remove it.

[Refactor] -1 from me in general on this extra set of assertions. There's a reason we haven't leaned into this pattern more: values should be their expected type before being used.

E.g. instead of:

memberTsNode:ts.MethodDeclaration|ts.PropertyDeclaration,// ...ESLintUtils.assert(ts.isClassLike(memberTsNode.parent),'...');

...we'd want either:

• Ifts.MethodDeclaration | ts.PropertyDeclaration's.parentis always ats.ClassLikeDeclaration, the AST's types should reflect that
• Ifts.MethodDeclaration | ts.PropertyDeclaration's.parentisn't always ats.ClassLikeDeclaration, thenmemberTsNode's type should have something like{ parent: ts.ClassLikeDeclaration } added

In other words: I think these assertions are surface-level fixes for type issues that should be fixed at a deeper level.

I'll also note that we generally try to work with the TSESTree AST rather than TypeScript's, when possible. Will mention this ongetBaseTypesOfClassMember itself.

replaced withas

Continuing from the comment on assertions: I'll also note that we generally try to work with the TSESTree AST rather than TypeScript's, when possible. We have more flexibility and power to make the AST more specific because we're the ones who define it (and we define it as a discriminated union).

In this case it'd be more typical for a typed lint rule to take in the TSESTree node. You can also pass in theservices: ParserServicesWithTypeInformation to use APIs likeservices.getSymbolAtLocation.

Done

[Typo] Grammar fix:

[Feature] In this PR's OP,allowReturnPromiseIfTryCatch is only mentioned in the collapsed note around fixers & suggestions. Seems to me it's no longer necessary, right?

ACK that it's not much added code but I don't think we have consensus that it's a strategy we'd want to add to the rule. Could you please split it out? We can always talk about it more in a followup issue - either its own and/or the one around fixers & suggestions.

Removed

This is still the case.

fixed

[Question] Separate from the promise/try-catch options, I don't know that we really need the remainingallowReturn* options. They're added complexity for the first version of a rule we haven't tried out in the real world yet. Do you see a strong need for them? (if not, I'm -1 on including them)

removed

Sounds to me like theif can be removed, then! 🔪

Change request: remove theif?

Just noting for posterity,node.operator in anAssignmentExpression can be any of:

"="|"+="|"-="|"*="|"**="|"/="|"%="|"<<="|">>="|">>>="|"&="|"|="|"||="|"&&="|"??="|"^="

The operators not mentioned in theif all produce TypeScript errors when given functions. So I agree with adding thevalid test case containingfoo += () => 1. That makes sure the removal of theif doesn't break those cases.

[Docs] Nit: I think the"is void returning function type" title says the same thing as this comment.

[Question] This usesVoid but the next check usesVoidLike. Swapping to any other combination of the type flags doesn't produce any test failures. Was there a reason to choose one vs. the other?

I'm genuinely asking, I don't know 😄

[Docs] Some users bristle at definitive, declarative statements like"if you do X, then Y will happen" when their design allows X without Y. Even if X and Y are not good.

[Testing] Removing thisif check doesn't fail any unit tests. Either this check should exist and tests are missing, or this is dead code that can be removed.

Similar note to earlier on the type narrowing.

[Testing] Removing thisif check doesn't fail any unit tests. Either this check should exist and tests are missing, or this is dead code that can be removed.

[Testing] Removing thisif checks doesn't fail any unit tests. Either they should exist and tests are missing, or this is dead code that can be removed.