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.

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

[Refactor] This "get the name one way if it's computed or another if it's static" need comes up a lot in rules. We should have a shared utility for it. getStaticStringValue should do the trick, or getStaticValue. One of those 😄.

There is already a function getStaticStringValue in eslint-plugin/src/util but it's about finding what the source representation of a value would be, e.g., turning a regex into /this/ or null into the string null, etc.

To avoid confusion with that I've broken out a getMemberName function you can look at and see if it might be worth breaking out into a shared utility

Yeah I think unions should be allowed. It's a nice convenience and intentionally part of the Promise aggregator design to allow this.

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

[Refactor] Every one of the uses for this function does >=1 === check. How about using a Set instead to make it more clear what's happening?

By the way, these functions are already included in several public releases. Shouldn't we keep the already published signatures to avoid breaking changes? Perhaps we can add this new functionality as overload/union? Although these packages are not documented yet, not sure, just asking 😄

Predicates are more composable than sets and offer easier ergonomics; for sets; (v) => symbolNameSet.has(v) is just as readable as symbolNameSet, and for looking up a single value (symbolName) => symbolName === 'asdf' is in my opinion much easier to interpret than new Set(['foo']) which offers no context for what the set is for, so you end up needing either a comment or breaking it out into a constant. Finally, predicates can be more consistently applied; e.g., we already have predicates for isBuiltinSymbolLikeRecurser, so switching between predicates and sets for arguments requires constantly deciding which ones "tend to be used like sets". Furthermore, predicates portray the intent of the argument more clearly: a set has many functions besides just contains-checking, including iteration. Providing a predicate makes it clear the only part the library function needs is the yes/no answer on each element.

For @auvred 's comment; happy to add the overload/union; adds a little clutter though, so will wait for more discussion since I'm not familiar enough with the project to have any opinion for backwards compatibility vs tidiness for those helpers

backwards compatibility

Yup, this'll need to retain backwards compatibility. I haven't heard of anybody telling us they use it, but a Sourcegraph query for TypeScript files referring to it shows a few public repos that do.

(v) => symbolNameSet.has(v) is just as readable as symbolNameSet

I think this is subjective 😄 I find the latter more intuitive. In my mind I'm kind of implicitly applying the context from "is built-in symbol like" to the last parameter. But that's just me.

offers no context for what the set is for, so you end up needing either a comment or breaking it out into a constant

This is equally applicable to both a predicate and a set. For both, a descriptive parameter name like allowedSymbolNames or isAllowedSymbolName fixes the context issue.

predicates portray the intent of the argument more clearly
a set has many functions besides ...

I suppose Pick<Set<String>, 'has'> would be the most precise thing to take in. All we really want in any of this is to know whether a name is what we're looking for.

One flaw of functions is it leaks the implementation detail of "this gets called up to once per symbol name", so folks can do weird things like:

isBuiltinSymbolLike(program, type, (name) => {
  someRegistry.add(name);
  if (someAttribute(name)) {
    context.report({ data: { name }, messageId: "someMessage", node: someNode });
  }
});

Just pointing this out as a counterpoint to the idea that a predicate adds more practical or semantic safety. I don't think we have to worry about anybody doing such an odd thing.

Anyway, the issue of whether we use a set or predicate is not particularly important to me. It's a small implementation detail and if you feel strongly about it I don't mind just shrugging and seeing what happens.

+1 in general to pushing back on my stylistic suggestions 😄 I like this!

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

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

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

[Non-Actionable] Every time I come back to this PR I get confused as to how such an important-looking file has such major gaps in unit test coverage... it's #6701, but for this package.

Filed #8358 to track adding in unit tests separately.