Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix inferences between alias type arguments and defaulted alias type arguments #51771
Fix inferences between alias type arguments and defaulted alias type arguments #51771
Changes from all commits
eb8ed4f
978b474
9f806ef
c27fc75
9e8c50d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wasn't even aware that the type argument list could be incomplete at this point, and I'm not sure that was ever intended. It's fine for type arguments to be missing at the lexical level, but once we're dealing with types we pretty much assume that the length of a type argument list matches the length of the corresponding type parameter list. That assumption is pretty pervasive throughout the compiler (modulo the
this
type argument that sometimes isn't present). I think it would make more sense to find the origin of the incomplete list and put the call tofillMissingTypeArguments
there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so, our old logic for giving an alias symbol instantiation an alias symbol didn't fill missing type arguments, which is why the new condition I added also didn't - it didn't seem required, since our existing usage didn't fill them. Obviously, broader usage of these unfilled lists reveals the flaw, namely inference (and probably comparison checking) not going well, since, as you said, they currently expect filled lists. Thing is, I think trafficking the unfilled lists has an advantage - it may be worth handling incomplete lists in inference/comparison checking. If we start filling them eagerly, rather than getting the user-supplied list, when we printback, we don't printback the user supplied list anymore. Meaning, even though you wrote
Component
, we print back and serializeComponent<{}, any, any>
. For complex defaults, this means we can muddy the printback unnecessarily quite a bit. So while I could fix this by passing in filled lists at the now 2 locations we supply an alias for an alias symbol instantiation, the UX is much nicer if instead we update inference and comparison checking to handle incomplete type alias argument lists, imo.