[Testing] If I remove this if 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:

declare let foo: () => void;
foo += () => 1;

still doesn't fail with condition removed.

At this point this rule already did everything no-misused-promises's checkVoidReturn did, but better. It doesn't have problems #8054 or #8739. Maybe it's worth splitting no-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 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...

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

Also of note is that no-misused-promises's checkVoidReturns is pretty configurable. 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? 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 the checksSpreads option:

const myPromise = Promise.resolve({num: 2, str: "2"});
const myObject = {...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 replace checksVoidReturn with strict-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 an onlyChecksPromises option.

As for checksConditionals, I actually just found microsoft/TypeScript#34717 and microsoft/TypeScript#39175—looks like checkConditionals 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), is using push with forEach:

declare function forEach<T>(arr: T[], callback: (el: T) => void): void;
let target: 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 to await Promises), in which case an onlyChecksPromises option would be useful if we did replace checksVoidReturn with strict-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.

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 the checksSpreads option:

const myPromise = Promise.resolve({num: 2, str: "2"});
const myObject = {...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 replace checksVoidReturn with strict-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 an onlyChecksPromises option.

As for checksConditionals, I actually just found microsoft/TypeScript#34717 and microsoft/TypeScript#39175—looks like checkConditionals 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), is using push with forEach:

declare function forEach<T>(arr: T[], callback: (el: T) => void): void;
let target: 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 to await Promises), in which case an onlyChecksPromises option would be useful if we did replace checksVoidReturn with strict-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:

• If ts.MethodDeclaration | ts.PropertyDeclaration's .parent is always a ts.ClassLikeDeclaration, the AST's types should reflect that
• If ts.MethodDeclaration | ts.PropertyDeclaration's .parent isn't always a ts.ClassLikeDeclaration, then memberTsNode'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 on getBaseTypesOfClassMember itself.

replaced with as

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 the services: ParserServicesWithTypeInformation to use APIs like services.getSymbolAtLocation.

Done

[Typo] Grammar fix:

[Testing] A few more edge cases to test

• extends Bar implements Foo, Baz
• extends class { ... } and extends class WithName { ... }
• implements class { ... } and implements class WithName { ... }

class implements class {} {} is already invalid TS

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

Oop, sorry I missed this question. Looking at the non-spread test case that hits this line:

declare function cb(): void;
const foo: Record<string, () => void> = {
  cb1: cb,
  cb2: cb,
};

We'd be able to trigger it by switching one of those properties to a spread:

declare function cb(): void;
const foo: Record<string, () => void> = {
  cb1: cb,
  ...{},
};

fixed

[Question] Separate from the promise/try-catch options, I don't know that we really need the remaining allowReturn* 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