[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.

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.

[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 😄

dunno

[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.

I think in this case the types are wrong.

AFAIK it's not possible to have TSEmptyBodyFunctionExpression as an object's property value.{ f() } is an syntax error. This is only valid in an abstract class body.

AssignmentPattern is also not valid as value of Property of ObjectLiteral, but it is valid for Property of ObjectPattern. Types could be improved by splitting Property into 2 types.

I will add type assertions for now.

[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.

There is already a test for this and if I remove this check then the types are wrong. TS functions say they don't accept null but if you pass null they just do nothing and return more null so nothing crashes.

[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.

Added test for empty body func.

Removingkind == 'method' actually makes the rule catch more cases.

[Testing] Removing the&& node.value... !== JSXEmptyExpression doesn't fail any unit tests. Either this check should exist and tests are missing, or this is dead code that can be removed.

If the reason it's here is to narrownode.value.expression's type down toTSESTree.Expression then maybe either the types are suggesting a legit case to handle, or are wrong?

Added valid tests for JSXEmptyExpression (prop={}) and JSXElement (prop=<elem/>) but I dunno how to get JSXSpreadChild.
I triedprop={...value} but it doesn't work.

Oh if that's just not doable in the AST then we can just ignore it. A good followup issue would be to tighten up the AST types to not include it.

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)