Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate assignability rules for ComponentLike and friends #470

Merged
merged 4 commits into from
Nov 23, 2022

Conversation

dfreeman
Copy link
Member

Background

Now that we've overhauled [Invoke] to have a simpler internal representation, we're better positioned to ensure that various ComponentLike values property reflects the real-world subtyping relationship between their types.

As a concrete example, a value with the bare ComponentLike type represents a component that can be invoked with no args, no blocks, and no modifiers. Given a definition like this:

class MyComponent extends Component<{
  Args: { name?: string };
  Element: HTMLImageElement;
  Blocks: { default: [] };
}> {}

The value MyComponent should be assignable to ComponentLike, because it is legal to invoke without passing any args, blocks or modifiers.

If, however, the name arg were not optional, then MyComponent should no longer be assignable to ComponentLike, as it would no longer be legal to invoke without any arguments.

The goal of this PR is to capture this and similar intuitions about the subtyping relationship between ComponentLike, HelperLike and ModifierLike values in the type system, and to put tests in place to ensure we maintain those relationships moving forward.

Details

The steps taken for this change also had a couple of side effects that were nice in their own right.

First, landing #447 meant we no longer need the EmptyObject type, because now when an entity accepts no named args, we just don't include a parameter for them in [Invoke] at all, so we no longer need to worry about triggering EPC for {} cases.

Removing EmptyObject also, in an ouroboric way, enabled us to clean up the last place where we did create a parameter unnecessarily for named args in InvokableArgs. This site was already EPC-enabled even without EmptyObject because of the NamedArgs wrapper, but it's nice not to have the parameter at all.

These two changes together ensure that the pairwise subtyping relationship between Args and Blocks works as expected, leaving only Element.

Internally we've used the same null marker as the public signature type does to represent the element of a component that never spreads its ...attributes. This doesn't work for assignability purposes, as we want a component that does spread ...attributes on a particular element type to be usable in a context where we aren't going to bother passing attributes or modifiers. Accordingly, we now use unknown internally for this scenario.

Now that we don't have to always have a slot for named
args even when the type is empty, we no longer need
`EmptyObject` to trigger EPC.
@dfreeman dfreeman added the breaking A breaking change label Nov 22, 2022
@dfreeman dfreeman linked an issue Nov 22, 2022 that may be closed by this pull request
Now that we no longer have to contend with `EmptyObject`, we can
safely determine when we're working with `{}` and produce more
accurate `[Invoke]` types.
This will enable us to have correct assignability/variance
rules for `ComponentLike` w.r.t `Element`
These tests ensure `HelperLike`, `ModifierLike` and `ComponentLike`
all have appropriate variance relative to their signatures'
constituent parts.
@dfreeman dfreeman merged commit 11a69e8 into main Nov 23, 2022
@dfreeman dfreeman deleted the assignability branch November 23, 2022 08:52
Copy link
Member

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retroactive review: this looks great overall and I think the changes all broadly make sense. I left some questions inline about removing rather than updating certain tests, but my one slightly bigger concern here is with the nullunknown transition for our internal representation on Element. The motivation made sense; is our error reporting going to be confusing, though?

@dfreeman
Copy link
Member Author

my one slightly bigger concern here is with the nullunknown transition for our internal representation on Element. The motivation made sense; is our error reporting going to be confusing, though?

Our error reporting is already pretty confusing in the "you didn't define an Element in your signature" scenario, both when trying to pass ...attributes to such a component and when trying to use ...attributes in such a component's template.

I'm inclined to say that subbing unknown for null in those error messages is at worst a lateral move on the "how nonsensical is this error message" scale, and that ultimately the right answer is that we should consider adding some special handling for these in our diagnostics/augmentation module.

@chriskrycho
Copy link
Member

Makes sense to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Meta issue: function helper resolution bugs
2 participants