Skip to content

🍒 [Clang] [Sema] Handle placeholders in '.*' expressions (#83103) #9038

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

ADKaster
Copy link

@ADKaster ADKaster commented Aug 2, 2024

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.


@ADKaster
Copy link
Author

ADKaster commented Aug 2, 2024

@swift-ci please test

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)
@ADKaster ADKaster force-pushed the backport-llvm-53815-to-swift-6x branch from 947eb05 to 4396144 Compare August 19, 2024 03:10
@ADKaster
Copy link
Author

Backports #9089 (4c0dc61) from stable/20230725.

Also the same commit is in next as the original upstream d23ef9e

Explanation: Prevents an assertion when trying to do overload resolution on operator .*
Scope: This change only materially affects assertion builds of llvm, such as is the default in pre-release artifacts. The original LLVM PR didn't seem to indicate any risk of changing the meaning of existing code.
Issue: llvm#53815, #8998
Risk: Low risk
Testing: CI, Local testing
Reviewer: @compnerd (for the stable PR 9089)

@ADKaster
Copy link
Author

cc @ravikandhadai

@compnerd
Copy link
Member

@swift-ci please test

@compnerd
Copy link
Member

CC: @DougGregor @airspeedswift

@DougGregor
Copy link
Member

This is safe enough to take. Approved for 6.0.

@DougGregor DougGregor merged commit 392fe16 into swiftlang:swift/release/6.0 Aug 20, 2024
3 checks passed
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.

4 participants