Fix issue 22175 - require ref for void AA update callbacks#22687
Fix issue 22175 - require ref for void AA update callbacks#22687divyansharma001 wants to merge 1 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request and interest in making D better, @divyansharma001! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes issue 22175 by requiring void-returning AA .update callbacks to accept their value parameter by ref, preventing silently ineffective mutations on copies.
Changes:
- Updated the
update(K, V, C, U)template constraint inobject.dto reject void callbacks that accept rvalues - Updated doc comment for the
updatecallback parameter - Added regression guard unittest and adjusted existing tests to use
reffor void-returning callbacks - Added a fail-compilation test
fail22175.d
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| druntime/src/object.d | Template constraint tightened; docs, existing tests, and new regression test updated |
| compiler/test/fail_compilation/fail22175.d | New fail-compilation test asserting the by-value void callback is rejected |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int[int] aa; | ||
| // Regression guard: this callback shape must never compile. | ||
| static assert( | ||
| is(typeof(aa.update(1, () => 10, delegate void(int x) { x += 1; }))), |
There was a problem hiding this comment.
The static assert here is written as a positive assertion — it passes only when the expression is true, i.e., when the offending code does compile. Because the goal of this test is to verify that this callback shape is rejected, the condition should be negated: !is(typeof(...)). As written, the test will fail (triggering the error message) precisely when the fix is working correctly, which is the opposite of the intended behaviour. The expected TEST_OUTPUT in the comment on line 6 appears to rely on this assertion failing, which confirms the inversion. A correct regression guard would use static assert(!is(typeof(...)), ...) so the test only fails when the fix is accidentally reverted.
| is(typeof(aa.update(1, () => 10, delegate void(int x) { x += 1; }))), | |
| !is(typeof(aa.update(1, () => 10, delegate void(int x) { x += 1; }))), |
Fixes #22175
The update function for associative arrays currently accepts void-returning callbacks that take the value parameter by copy. This silently discards any mutations the user makes, which is a bug trap.
This change tightens the template constraint so that if the update callback returns void, it must accept its parameter by ref. Non-void-returning callbacks are unaffected, since they communicate the updated value through the return value.
Changes
druntime/src/object.d: Added && !is(typeof(update(V.init)) == void) to the template constraint for the update function. Updated the doc comment. Fixed two existing unit tests that used void+by-value callbacks, and added a regression unittest with three static asserts.
compiler/test/fail_compilation/fail22175.d: New fail_compilation test verifying that a void-returning, by-value callback is rejected at compile time.