-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Sema] Allow PlaceholderType Originator to be TypeRepr not just PlaceholderTypeRepr #75463
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
Conversation
@swift-ci Please smoke test. |
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.
I left a comment inline, feel free to land this once addressed.
@@ -1071,12 +1071,12 @@ Type ConstraintSystem::replaceInferableTypesWithTypeVars( | |||
return openUnboundGenericType(unbound->getDecl(), unbound->getParent(), | |||
locator, /*isTypeResolution=*/false); | |||
} else if (auto *placeholderTy = type->getAs<PlaceholderType>()) { | |||
if (auto *placeholderRepr = placeholderTy->getOriginator() | |||
.dyn_cast<PlaceholderTypeRepr *>()) { |
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.
This change needs an additional check for PlaceholderTypeRepr
as is we only want to open _
type reprs and not the invalid reprs.
@@ -1172,7 +1172,7 @@ void TypeChecker::notePlaceholderReplacementTypes(Type writtenType, | |||
} | |||
|
|||
if (auto *origRepr = | |||
placeholder->getOriginator().dyn_cast<PlaceholderTypeRepr *>()) { | |||
placeholder->getOriginator().dyn_cast<TypeRepr *>()) { |
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.
We should add an assert here that that makes sure that origRepr
was PlaceholderTypeRepr
.
Let's hold off on this for a bit because original change is getting reverted due to source compatibility impact. |
Fixed feedback, but won't merge here until/unless the previous stuff works out. |
Re-applied in #75526 |
No functional change. In addition to the Originator change, I realized I failed to move the
resolveType()
inConcreteTypeSpecialization
back to where it should be in the previous PR so that is included as well.This is a follow-on to PR #74909.