-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[AST, Sema] Replace HoleType with PlaceholderType #35589
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
Changes from all commits
0f7ec99
580fd40
d78d95e
e508fb3
65521b5
fbb78cd
a79e7b5
a5d0153
40122df
e4ea167
be2aeb5
1819d33
ddd4842
8539782
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,7 +138,7 @@ struct PotentialBinding { | |
|
||
static PotentialBinding forHole(TypeVariableType *typeVar, | ||
ConstraintLocator *locator) { | ||
return {HoleType::get(typeVar->getASTContext(), typeVar), | ||
return {PlaceholderType::get(typeVar->getASTContext(), typeVar), | ||
AllowedBindingKind::Exact, | ||
/*source=*/locator}; | ||
} | ||
|
@@ -279,9 +279,9 @@ struct PotentialBindings { | |
bool isPotentiallyIncomplete() const; | ||
|
||
/// If this type variable doesn't have any viable bindings, or | ||
/// if there is only one binding and it's a hole type, consider | ||
/// if there is only one binding and it's a placeholder type, consider | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree - I like the term "hole" for the constraint system because "placeholder" implies that it can be replaced, but if we bind to a placeholder in the constraint system, that means it solver could not replace it with a real type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me, I don't have super strong feelings either way! So we'd keep the type name as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that’s the idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Effectively we are just utilizing a placeholder in a solution and AST to represent places which couldn’t be inferred. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hborla @xedin Thoughts on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should leave it as hole since it would be consistent with the rest. |
||
/// this type variable to be a hole in a constraint system | ||
/// regardless of where hole type originated. | ||
/// regardless of where the placeholder type originated. | ||
bool isHole() const { | ||
if (isDirectHole()) | ||
return true; | ||
|
@@ -290,14 +290,14 @@ struct PotentialBindings { | |
return false; | ||
|
||
const auto &binding = Bindings.front(); | ||
return binding.BindingType->is<HoleType>(); | ||
return binding.BindingType->is<PlaceholderType>(); | ||
} | ||
|
||
/// Determines whether the only possible binding for this type variable | ||
/// would be a hole type. This is different from `isHole` method because | ||
/// type variable could also acquire a hole type transitively if one | ||
/// of the type variables in its subtype/equivalence chain has been | ||
/// bound to a hole type. | ||
/// would be a placeholder type. This is different from `isHole` method | ||
/// because type variable could also acquire a placeholder type transitively | ||
/// if one of the type variables in its subtype/equivalence chain has been | ||
/// bound to a placeholder type. | ||
bool isDirectHole() const; | ||
|
||
/// Determine if the bindings only constrain the type variable from above | ||
|
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.
Just a note - making unrelated whitespace changes like this sometimes complicates the review since it's easy to miss value changes...
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.
Ah, that must have been
clang-format
, I didn't make any intentional whitespace changes here. I'll revert!