Make conditional operators on objects rvalues - #8989
Joel Kiptoo (Kiptoo-Deus) wants to merge 1 commit into
Conversation
CheckVectorConditional converts the operands of a conditional operator to rvalues, except when both are objects of the same type, where it returned early. The result was then classified as an lvalue, so '(c ? a : b) = buf' with resources hit an assert in debug builds and was silently dropped in release builds, and calling a method on such a conditional asserted as well. Convert the operands to rvalues in that case too, so the assignment gets the usual 'expression is not assignable' error. Also check rightObjectKind instead of leftObjectKind twice. Fixes microsoft#8579
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused implementation matches existing conditional semantics and includes appropriate regression coverage and release documentation.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes conditional expressions on HLSL object/resource types being incorrectly classified as lvalues.
Changes:
- Converts object operands to rvalues and corrects the right-operand object check.
- Adds regression coverage for assignment rejection and valid resource usage.
- Documents the user-visible fix.
| File | Description |
|---|---|
tools/clang/lib/Sema/SemaHLSL.cpp |
Corrects object conditional classification. |
tools/clang/test/SemaHLSL/conditional-object-not-assignable.hlsl |
Adds regression tests. |
docs/ReleaseNotes.md |
Records the compiler bug fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
I've seen this in a few PRs. This sort of commentary doesn't really belong in the PR description when we get to merging it. FWIW, I'm pretty sure lit is supported on MacOS, so I'd encourage you to figure out how to properly test this end to end on your local config. |
Did you try just running the |
Chris B (llvm-beanz)
left a comment
There was a problem hiding this comment.
This looks correct for HLSL 2021, but may need to be revised for newer versions of HLSL as we work though the proposal to adopt C++ ternary operator semantics:
| if (leftType == rightType) { | ||
| // As for the other types, the result is not an lvalue, so it can't be | ||
| // assigned to. | ||
| if (LHS.get()->isLValue()) |
There was a problem hiding this comment.
I believe we want to gate this behavior on the language version, and preserve this older behavior, according to this document:
https://hlsl-tc57.github.io/tc57/proposal/0017/
See if you can add a condition here to ensure that the language version is at least 202x.
Joshua Batista (bob80905)
left a comment
There was a problem hiding this comment.
Talked offline, gating on language version is a later concern. LGTM
|
Thanks both, and fair point. I hadn't tried |
Fixes #8579.
HLSLExternalSource::CheckVectorConditionalconverts the operands of?:to rvalues, but when both operands are objects of the same type (e.g. twoRWByteAddressBuffers) it returned early without doing that. The operands stayed lvalues, so C++ classification treated the conditional as an lvalue while the expression was built as an rvalue:(true ? a : b) = gBuf1;(the case from the issue) asserts withgetValueKind() == VK_LValuein debug builds and is silently dropped in release builds.(false ? a : b).Store(4, 2);, hits the same assert.The object case now converts both operands to rvalues like the other branches, so assigning to the conditional gets the same
expression is not assignableerror DXC already reports for scalars, vectors and matrices, and the method call compiles. I also fixed the check just above it, which testedleftObjectKindtwice instead of left and right (harmless because of theleftType == rightTypecheck that follows).Testing (macOS arm64, Release build with assertions):
SemaHLSL/conditional-object-not-assignable.hlsl(-verify): the resource anduintassignments are errors, and copying a conditional of resources into a local and callingStoreon one still compile.RWByteAddressBuffer c = true ? gBuf0 : gBuf1;produces identical DXIL before and after.(false ? gBuf0 : gBuf1).Store(4, 2)now compiles and stores through the handle foru1.