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

Check for reentrancy in fillMissingTypeArguments and use defaults instead if so #28971

Conversation

weswigham
Copy link
Member

Fixes #28873

fillMissingTypeArguments uses the constraint to allow the default itself to reference the parent type circularly (and use defaults), however when the constraint itself references the parent type and uses defaults, ofc we can't safely renter and just need to use the base default type.

Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

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

I'm thinking we should use pushTypeResolution and popTypeResolution as we do in other places where circularities can occur. With the suggested change we'll just silently pick any or unknown.

@weswigham
Copy link
Member Author

Push/pop type resolution track circularity on a single type (and require the results be cached in a field on that type), not a list of types (and not something that's uncached), which'd be why it's not usable here. And using them wouldn't change the any/unknown part - what we do when there is a circularity is chosen by what we return when it's detected... In this case, I choose the any/unknown base default, because the error type (which is any) is an poor choice for a constraint. It's probably also worth noting that this won't apply to the top-level default - that'll always be well-formed this way, it'll just become base-defaulted where it self-references (in both the default and constraint). We wanna check if we're reentrant on a given type argument to parameter list combination - so long as the same lists don't come back up, we're fine.

@ahejlsberg
Copy link
Member

@weswigham Much simpler fix in #29144.

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

Successfully merging this pull request may close these issues.

None yet

2 participants