[Docs] Interesting, we don't have a standard for whether we refer to Promises with an upper-case P or lower-case p... Outside the scope of this PR, maybe it'd be a good followup to file an issue about standardizing this in docs?

Not a blocker or request for changes 😄 just ruminating.

[Question] Is there a use case for adding an option to allow users to provide an array/tuple where only some of the elements are thenables? Like Promise.all([callAsyncThing(), "other", callAsyncThing()])?

I can't think of one 🤔 so maybe this is a non-actionable comment?

Btw, sorry if I missed this in previous discussion - I looked through and couldn't find anything.

This is the main case I'm specifically looking to lint, and is the most common case where mistakes are made. There is no advantage to doing this from the perspective of control flow (i.e., how long the returned promise takes to fulfill), however, it does change the return value. However, whenever you really do want to provide a non-Thenable to change the control flow, you can always wrap it in Promise.resolve to get exactly the same behavior while making it way more clear how the aggregator will behave. This is still true even when considering if the returned promise is fulfilled synchronously vs asynchronously: these promise aggregators only fulfill synchronously when passed an empty array.

I would argue especially for all of these promise aggregators, the most important thing is making the control flow clear. Any non-Thenable value passed to a promise aggregator will be treated as Promise.resolve(value) according to the spec; see e.g., PerformPromiseAll. Thus given that a value in the list is known to be non-Thenable, there is a static conversion available that will make the control flow easier to understand, or you can be explicit and wrap the value in Promise.resolve, which jumps out at the reader and thus also makes the control flow easier to understand.

Given:

declare const aPromise: Promise<number>;
declare const bPromise: Promise<number;
declare const cNumber: number;

// all

Promise.all([aPromise, bPromise, cNumber])
// equivalent to
Promise.all([aPromise, bPromise, Promise.resolve(cNumber)])
// which resolves in the same amount of time as
Promise.all([aPromise, bPromise])

// previously working, now causes lint error
const [a, b, c] = await Promise.all([aPromise, bPromise, cNumber]);

// suggested change
const [a, b] = await Promise.all([aPromise, bPromise]);
const c = cNumber;

// allSettled

Promise.allSettled([aPromise, bPromise, cNumber]);
// equivalent to
Promise.allSettled([aPromise, bPromise, Promise.resolve(cNumber)]);
// resolves in the same time as
Promise.allSettled([aPromise, bPromise]);

// previously working, now causes lint error
const [aPromise2, bPromise2, cPromise] = await Promise.allSettled([aPromise, bPromise, cNumber]);

// suggested change
const [aPromise2, bPromise2] = await Promise.allSettled([aPromise, bPromise]);
const cPromise = Promise.resolve(cNumber);

// race

await Promise.race([aPromise, bPromise, cNumber]);
// equivalent to
await Promise.race([aPromise, bPromise, Promise.resolve(cNumber)]);
// always resolves immediately (but never synchronously, as Promise.race is never synchronous)

// previously working, now causes lint error
// this is a if a is immediately resolved, b if b is immediately resolved, otherwise c
const aOrBOrC = await Promise.race([aPromise, bPromise, cNumber]);

// suggested change if you really did want the above behavior (most of the time you do NOT)
const aOrBOrC = await Promise.race([aPromise, bPromise, Promise.resolve(cNumber)]);

// suggested change if you actually intended to wait for something to happen
const aOrB = await Promise.race([aPromise, bPromise])

// note that most of the time this comes up, this is whats actually happening:
declare const aPromise: Promise<number>;
declare const bPromise: Promise<number>;
declare const cWrappedPromise: { promise: Promise<number> };

// lint error that catches a real subtle bug
const aOrBOrC = await Promise.race([aPromise, bPromise, cWrappedPromise]);

// correct code
const aOrBOrC = await Promise.race([aPromise, bPromise, cWrappedPromise.promise]);

// any is the same as race for the above purposes, the difference is how rejected values are handled

[Non-Actionable] I realized a situation where this option would be helpful! [playground link]

declare function fetch(url: string): Promise<object>;

function example() {
  return Promise.all([
    fetch("/"),
    "in the middle",
    fetch("/")
  ]);
}

If I was the developer here using Promise.all directly to avoid having to make & manage a [object, string, object] tuple, I'd be annoyed at the rule asking me to explicitly Promise.resolve("in the middle"). There's no benefit in this situation beyond consistency - at the cost of simplicity.

But, I get what you're saying that this is really the main point of the rule. I think it's fine for now to leave as-is. Since it's only enabled in the strict config, it should just show up for users who have opted into the more strict opinions.

Please do add an option that allows mixed arrays. I often parallel a bunch of tasks and sometimes tasks shift between being sync and async (for example, because it no longer dynamically imports a library, or it no longer reads a config file). I really don't want to break the symmetry in this case.

In that case, +1ing Josh's request 🙂

[Testing] Missing unit test coverage for this and a few other lines. If it's a case where we know the value is definitely not nullish (e.g. an array type having a numeric index) then a ! is reasonable.

[Refactor] Similar to the isBuiltinSymbolLike comment, this is only ever used with a Set<string>. I think expanding the use case to predicates is just a bit more complexity than we need.

this logic (if isTypeAnyType(sometype) || isTypeUnknownType(sometype) or tsutils.isThenableType(checker, originalNode, sometype)) is used three times in this rule. Maybe we should move it to the function to avoid duplicated code?

Hmm; I refactored around a bit, though I still have 3 calls like this. However, I don't think something like this would add anything besides some mental overhead:

function isTypeAnyOrUnknownType(type: ts.Type): boolean {
  return isTypeAnyType(type) || isTypeUnknownType(type);
}

For this to be useful from my current perspective there would need to be a more salient name for the method that is a more useful concept than checking for any or unknown and makes the implementation obvious (since it's so short). I looked elsewhere around the codebase and any types are not universally treated the same as unknown types, e.g., I could imagine a flag on this check later, perhaps two flags (one for any and one for unknown), and depending on exactly how those flags work it might make sense to break it out then, but more likely even if you made those flags it would still not be helpful to have a function like this given you would want different errors each which explanations of the flag to use to disable that feature, ect.