-
Notifications
You must be signed in to change notification settings - Fork 393
Fix Atomic::load() being incorrectly marked as mutating #8807
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
Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com>
| // This is a regression test for GitHub issue where load() was incorrectly marked as [__ref] | ||
| // instead of [constref], causing it to be considered mutating. | ||
|
|
||
| groupshared Atomic<uint> sharedNumber; |
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 don't think this test is correct. Should be:
//TEST:COMPARE_COMPUTE(filecheck-buffer=CHECK):-output-using-type
struct AtomicCounter
{
Atomic<uint> sharedNumber;
property uint value { get { return sharedNumber.load(); } }
}
//TEST_INPUT: set gBuffer = ubuffer(data=[10 0 0 0], stride=4)
StructuredBuffer<AtomicCounter> gBuffer;
//TEST_INPUT: set outputBuffer = out ubuffer(data=[0 0 0 0], stride=4)
RWStructuredBuffer<uint> outputBuffer;
[numthreads(1,1,1)]
void computeMain()
{
// CHECK: 10
outputBuffer[0] = gBuffer[0].value;
}
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.
Updated the test as suggested in commit d82ddf2.
|
@copilot fix the test. |
Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com>
aidanfnv
left a comment
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.
Looks good to me.
…#8807) - [x] Explore repository structure and understand the issue - [x] Create a test case to reproduce the issue (Atomic::load in property getter) - [x] Verify the issue exists in current code - [x] Fix the issue by changing `[__ref]` to `[constref]` in core.meta.slang line 3744 - [x] Build the code to verify fix compiles - [x] Run the test to verify the fix works - [x] Run formatting script - [x] Run CodeQL security check - [x] Final verification - all tests passing - [x] Address reviewer feedback on test ## Summary Fixed the issue where `Atomic<T>::load()` was incorrectly marked as mutating, preventing it from being called in property getters or other const contexts. **Changes made:** 1. Changed `[__ref]` to `[constref]` attribute on `Atomic<T>::load()` method in `source/slang/core.meta.slang` line 3744 2. Added regression test in `tests/language-feature/atomic-load-property.slang` to verify the fix (updated based on reviewer feedback) The fix is minimal and surgical - only changing the single attribute that was incorrect. All existing tests continue to pass. <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Atomic::load is considered mutating</issue_title> > <issue_description># Issue Description > A clear and concise description of what the bug is. > Before filing the issue, please make sure the bug can be reproduced with the latest release of Slang. > it seems that the Atomic<T>::load method is considered mutating when it shoudl be non mutating. > > # Reproducer Code > ```cpp > Atomic<float> number; > > property float value { get { return number.load(); } } > ``` > > # Expected Behavior > I expected the following code tocompile since it shouldnt mutate. > > # Actual Behavior > It gives a compiler error, which I believe is incorrect behaviour. > > # Environment > Windows > Latest slangc version 2025.19.1 > > # Additional context > Nope, this might be intended behaviour in which case can you please elucidate this?</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@csyonghe</author><body> > Looks like an error in core.meta.slang:3744, which should be: > ``` > [constref] T load(MemoryOrder order = MemoryOrder.Relaxed); > ``` > instead of > ``` > [__ref] T load(MemoryOrder order = MemoryOrder.Relaxed); > ```</body></comment_new> > </comments> > </details> Fixes shader-slang#8759 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/shader-slang/slang/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> Co-authored-by: Yong He <yonghe@outlook.com>
…#8807) - [x] Explore repository structure and understand the issue - [x] Create a test case to reproduce the issue (Atomic::load in property getter) - [x] Verify the issue exists in current code - [x] Fix the issue by changing `[__ref]` to `[constref]` in core.meta.slang line 3744 - [x] Build the code to verify fix compiles - [x] Run the test to verify the fix works - [x] Run formatting script - [x] Run CodeQL security check - [x] Final verification - all tests passing - [x] Address reviewer feedback on test ## Summary Fixed the issue where `Atomic<T>::load()` was incorrectly marked as mutating, preventing it from being called in property getters or other const contexts. **Changes made:** 1. Changed `[__ref]` to `[constref]` attribute on `Atomic<T>::load()` method in `source/slang/core.meta.slang` line 3744 2. Added regression test in `tests/language-feature/atomic-load-property.slang` to verify the fix (updated based on reviewer feedback) The fix is minimal and surgical - only changing the single attribute that was incorrect. All existing tests continue to pass. <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Atomic::load is considered mutating</issue_title> > <issue_description># Issue Description > A clear and concise description of what the bug is. > Before filing the issue, please make sure the bug can be reproduced with the latest release of Slang. > it seems that the Atomic<T>::load method is considered mutating when it shoudl be non mutating. > > # Reproducer Code > ```cpp > Atomic<float> number; > > property float value { get { return number.load(); } } > ``` > > # Expected Behavior > I expected the following code tocompile since it shouldnt mutate. > > # Actual Behavior > It gives a compiler error, which I believe is incorrect behaviour. > > # Environment > Windows > Latest slangc version 2025.19.1 > > # Additional context > Nope, this might be intended behaviour in which case can you please elucidate this?</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@csyonghe</author><body> > Looks like an error in core.meta.slang:3744, which should be: > ``` > [constref] T load(MemoryOrder order = MemoryOrder.Relaxed); > ``` > instead of > ``` > [__ref] T load(MemoryOrder order = MemoryOrder.Relaxed); > ```</body></comment_new> > </comments> > </details> Fixes shader-slang#8759 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/shader-slang/slang/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com> Co-authored-by: Yong He <yonghe@outlook.com>
[__ref]to[constref]in core.meta.slang line 3744Summary
Fixed the issue where
Atomic<T>::load()was incorrectly marked as mutating, preventing it from being called in property getters or other const contexts.Changes made:
[__ref]to[constref]attribute onAtomic<T>::load()method insource/slang/core.meta.slangline 3744tests/language-feature/atomic-load-property.slangto verify the fix (updated based on reviewer feedback)The fix is minimal and surgical - only changing the single attribute that was incorrect. All existing tests continue to pass.
Original prompt
Fixes #8759
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.