[Clang] [Sema] Handle placeholders in '.*' expressions (#83103) #9089
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When analysing whether we should handle a binary expression as an overloaded operator call or a builtin operator, we were calling `checkPlaceholderForOverload()`, which takes care of any placeholders that are not overload sets—which would usually make sense since those need to be handled as part of overload resolution. Unfortunately, we were also doing that for `.*`, which is not overloadable, and then proceeding to create a builtin operator anyway, which would crash if the RHS happened to be an unresolved overload set (due hitting an assertion in `CreateBuiltinBinOp()`—specifically, in one of its callees—in the `.*` case that makes sure its arguments aren’t placeholders). This pr instead makes it so we check for *all* placeholders early if the operator is `.*`. It’s worth noting that, 1. In the `.*` case, we now additionally also check for *any* placeholders (not just non-overload-sets) in the LHS; this shouldn’t make a difference, however—at least I couldn’t think of a way to trigger the assertion with an overload set as the LHS of `.*`; it is worth noting that the assertion in question would also complain if the LHS happened to be of placeholder type, though. 2. There is another case in which we also don’t perform overload resolution—namely `=` if the LHS is not of class or enumeration type after handling non-overload-set placeholders—as in the `.*` case, but similarly to 1., I first couldn’t think of a way of getting this case to crash, and secondly, `CreateBuiltinBinOp()` doesn’t seem to care about placeholders in the LHS or RHS in the `=` case (from what I can tell, it, or rather one of its callees, only checks that the LHS is not a pseudo-object type, but those will have already been handled by the call to `checkPlaceholderForOverload()` by the time we get to this function), so I don’t think this case suffers from the same problem. This fixes llvm#53815. --------- Co-authored-by: Aaron Ballman <[email protected]> (cherry picked from commit d23ef9e)
|
@swift-ci please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When analysing whether we should handle a binary expression as an overloaded operator call or a builtin operator, we were calling
checkPlaceholderForOverload(), which takes care of any placeholders that are not overload sets—which would usually make sense since those need to be handled as part of overload resolution.Unfortunately, we were also doing that for
.*, which is not overloadable, and then proceeding to create a builtin operator anyway, which would crash if the RHS happened to be an unresolved overload set (due hitting an assertion inCreateBuiltinBinOp()—specifically, in one of its callees—in the.*case that makes sure its arguments aren’t placeholders).This pr instead makes it so we check for all placeholders early if the operator is
.*.It’s worth noting that,
.*case, we now additionally also check for any placeholders (not just non-overload-sets) in the LHS; this shouldn’t make a difference, however—at least I couldn’t think of a way to trigger the assertion with an overload set as the LHS of.*; it is worth noting that the assertion in question would also complain if the LHS happened to be of placeholder type, though.=if the LHS is not of class or enumeration type after handling non-overload-set placeholders—as in the.*case, but similarly to 1., I first couldn’t think of a way of getting this case to crash, and secondly,CreateBuiltinBinOp()doesn’t seem to care about placeholders in the LHS or RHS in the=case (from what I can tell, it, or rather one of its callees, only checks that the LHS is not a pseudo-object type, but those will have already been handled by the call tocheckPlaceholderForOverload()by the time we get to this function), so I don’t think this case suffers from the same problem.This fixes llvm#53815.
Co-authored-by: Aaron Ballman [email protected]
(cherry picked from commit d23ef9e)