[base-controller] Fix all any usage, apply universal supertype for functions

[MajorLift]

Motivation

The ControllerMessenger pattern that is used in all of our controllers currently relies on unsafe typing, which includes any usage in key types. A more rigorous overhaul of these definitions will help us implement features with more type-safe and robust code.

Explanation

Main

• Removes all any usage from Action, Event typing in ControllerMessenger.
• Types Action['handler'] with correct universal supertype for functions.
• Fixes SelectorFunction, EventSubscriptionMap typing that was causing errors.
• Improves BaseController typing by defining NamespacedName type: the narrowest supertype of all names defined within a given namespace.
• BaseController now expects a "getState" action type and "stateChange" event type in all controllers.

Auxiliary

• Improved typing for deriveStateFromMetadata
• Rename all single-letter type param names to be more descriptive. (moved here: [base-controller] Fix single-letter type param names #2030)

Notes on the "universal function supertype"

A. (...args: never[]) => unknown

Action['handler'] extends ActionConstraint['handler'], and in general Subtype extends Supertype. Therefore, in order to assign any function to Action['handler'] without restrictions, we need ActionConstraint['handler'] to be the widest possible function i.e. the supertype of all functions.

To achieve this, the return type of this supertype function needs to be unknown (universal supertype), and the parameters need to be never (universal subtype). This is because in general (x: SuperType) => T is a subtype of (x: SubType) => T. So the params need to be the narrowest type (i.e. never) for the function type to be widened into the universal function supertype.

• There's an example of this in the TypeScript docs.
• And here's a more detailed reddit comment on this topic.

B. (...args: never) => unknown

In general, array types of arbitrary length are not assignable to tuple types that have fixed types designated to some or all of its index positions (the array is wider than the tuple since it has less constraints).

This means that there's a whole subcategory of functions that (...args: never[]) => unknown doesn't cover: functions that have a finite number of parameters that are strictly ordered and typed, making their spread argument list evaluate as a fixed-length or variadic tuple type, instead of a freely expandable (or contractable) array type.

That is, (...args: never[]) => unknown covers the cases where all of the params can be typed together as a group e.g. (...args: (string | number)[]) => T. But to cover cases like (a: string, b: number, ...rest: unknown[]) => T, we need another top type with an even narrower type for the params: (...args: never) => unknown.

C. ((...args: never) => unknown) | ((...args: never[]) => unknown)

In short, the above represents the top type that includes both of these function categories.

• | never in general is analogous to + 0, so (...args: (never[] | never)) => unknown doesn't work.
• (...args: never) => unknown doesn't work with the error message Type 'T[]' is not assignable to type 'never'.

References

• Closes [base-controller] fix: any usage #2026

Changelog

@metamask/base-controller

• CHANGED: ActionConstraint['handler'] has been fixed to remove any usage, and is now defined as the universal supertype of all functions, meaning any function can be safely assigned to it, regardless of argument types, number of arguments, or return value type.
• CHANGED : ExtractActionParameters and ExtractActionResponse have also been fixed to remove any usage, and to use the new typing of ActionConstraint['handler'].
• CHANGED: BREAKING Alters SelectorFunction type, replacing its Args generic parameter with an event payload type that is defined with a new required generic parameter Event.
• ADDED: Exports NamespacedName type, which is the narrowest supertype of all names defined within a) a given namespace if a namespace is supplied as a generic parameter, or b) any namespace in general if no generic parameter is provided.

@metamask/rate-limit-controller

• CHANGED: BREAKING RateLimitedApi['method'] is now constrained by the ActionConstraint['handler'] type from the @metamask/base-controller module.

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[mcmire]

I left some comments on the base-controller changes that I was curious about. What do you think about splitting those off into a separate PR? I feel like they could be high impact and so it'd be better to have them in their own PR so they're easier to review now and later.

[mcmire]

This is interesting. This would be a breaking change, and so I am wary of changing this. That said, I don't understand why we chose to use any here. I would be curious to hear if Mark can remember any context around this.

[MajorLift]

ActionConstraint and EventConstraint in this PR are written to be the universal supertypes of their respective categories, so I don't think we'd be adding a meaningful constraint by moving away from any. Also no tests are being broken by this change, although I guess we'll have to check whether any package outside of the monorepo is using incompatible Action or Event types.

[Gudahtt]

In general, using any in this context is not harmful in the same way that it is in other contexts. Types used in a generic constraints don't get applied to any specific variables, they're just constraints (i.e. by using any here, absolutely nothing is actually given the type any, even indirectly). For this reason I've sometimes put less effort into avoiding the use of any for generic constraints.

That said, more detailed constraints don't hurt either. And in this case, these constraints are already present in the RestrictedControllerMessenger type so this change seems like it would not affect anything functionally.

[MajorLift]

Generic constraints are also helpful for intellisense hints and self-documenting code. If finding the right constraints isn't a significant blocker for actual feature development, I think it's worth the extra effort, even if it's not strictly necessary in terms of functionality.

[MajorLift]

@mcmire On the subject of (...args: never[]) => unknown:

• [base-controller] Fix all any usage, apply universal supertype for functions #1890 (comment)
• [base-controller] Fix all any usage, apply universal supertype for functions #1890 (comment)

This is a really fun one! never[] is actually the logically correct type here, as unintuitive as that seems.

So just in empirical terms, changing the never[] to unknown[] (or even [never]) triggers a whole lot of errors all over the codebase, basically everywhere the Action type is used.

Here's why: Action['handler'] extends ActionConstraint['handler'], and in general Subtype extends Supertype. Therefore, in order to assign any function to Action['handler'] without restrictions, we need ActionConstraint['handler'] to be the widest possible function i.e. the supertype of all functions.

To achieve this, the return type of this supertype function needs to be unknown (universal supertype), and the parameters need to be never (universal subtype). This is because in general (x: SuperType) => T is a subtype of (x: SubType) => T. So the params need to be the narrowest type (i.e. never) for the function type to be widened into the universal function supertype.

• There's an example of this in the TypeScript docs.
• And here's a more detailed reddit comment on this topic.

[Gudahtt]

LGTM!

[mcmire]

Looks good!

[mcmire]

@Mrtenz Do you have any more concerns about this PR?

[MajorLift]

So it turns out that (...args: never[]) => unknown probably isn't the universal supertype for functions.

The error messages @Gudahtt linked here #1890 (comment) reflect the fact that in general, array types of arbitrary length are not assignable to tuple types with independent typing for some or all of its index positions (the array is wider than the tuple since it has less constraints e.g. the tuple has a minimum length whereas the array could be empty).

This means that there's a whole subcategory of functions that (...args: never[]) => unknown doesn't cover: functions with at least some parameters that are positionally typed, making their argument list evaluate as a fixed-length or variadic tuple type, instead of a freely expandable (or contractable) array type.

To illustrate, (...args: never[]) => unknown covers the cases where all of the params can be typed together as a group e.g. (...args: (string | number)[]) => T. But to cover cases like (a: string, b: number, ...rest: unknown[]) => T, where positional arguments are typed independently, we need another top type with an even narrower params type: (...args: never) => unknown.

In short, I think the top type for functions that includes both of these categories is: ((...args: never) => unknown) | ((...args: never[]) => unknown).

(| never in general is analogous to + 0, so (...args: (never[] | never)) => unknown doesn't work.)

With this change, CallApi['handler'] is now accepted by RestrictedControllerMessenger as extending ActionConstraint['handler'] without needing the explicit assertion, and running git checkout origin/main ./packages/rate-limit-controller/ on this PR branch no longer results in any type errors.

[mcmire]

Great sleuthing! 👏🏻

[mcmire]

Looks great!

[Mrtenz]

Looks great!

[Gudahtt]

Could you update the PR description to clarify that the action constraint change is no longer breaking? I think with this most recent update that should be the case

[MajorLift]

@Gudahtt Changelog updated!