fix Issue 22328: Reject assignment to enum array elements #22401
Conversation
|
Thanks for your pull request and interest in making D better, @gorsing! 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. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#22401" |
| void testArrayLiteralLvalue() | ||
| { | ||
| // https://github.com/dlang/dmd/pull/16784 | ||
| // Test that this array literal is *not* put on the stack because | ||
| // it gets its address taken | ||
| static int* getPtr(int i) => &[1, 2, 3][i]; | ||
| int* x = getPtr(1); | ||
| int* y = getPtr(1); | ||
| assert(x != y); | ||
| } |
There was a problem hiding this comment.
why is this test removed?
There was a problem hiding this comment.
Thanks for your time @thewilsonator.
This test relied on taking the address of an indexed array literal (&[1, 2, 3][i]). My changes correctly classify indexed array literals as rvalues (to prevent accidental modification, as per the issue). Since they are now rvalues, taking their address is no longer valid D code, causing this test to fail compilation. I removed it because the construct it tests is now illegal.
I have updated my regression test fail_compilation/fail22328.d to include this case, ensuring that taking the address of such literals is explicitly rejected.
Example
/*
TEST_OUTPUT:
---
fail_compilation/fail22328.d(17): Error: cannot modify the content of array literal `[1, 2, 3]`
fail_compilation/fail22328.d(17): Error: cannot modify the content of array literal `[1, 2, 3]`
fail_compilation/fail22328.d(17): Error: cannot modify expression `[1, 2, 3][2]` because it is not an lvalue
fail_compilation/fail22328.d(18): Error: cannot modify the content of array literal `[10, 20]`
fail_compilation/fail22328.d(18): Error: cannot modify the content of array literal `[10, 20]`
fail_compilation/fail22328.d(18): Error: cannot modify expression `[10, 20][0]` because it is not an lvalue
fail_compilation/fail22328.d(20): Error: cannot modify the content of array literal `[1, 2, 3]`
fail_compilation/fail22328.d(22): Error: cannot take address of expression `[1, 2, 3][0]` because it is not an lvalue
---
*/
void main() {
enum ARR = [1, 2, 3];
ARR[2] = 4;
[10, 20][0] = 30;
ARR[0..2] = [4, 5];
auto p = &ARR[0];
}
| enum int[] array = [0]; | ||
| auto array = [0]; | ||
| // PostExp | ||
| assert(array[0]++ == 0); | ||
| assert(array[0]-- == 0); | ||
| assert(array[0]-- == 1); | ||
| // PreExp | ||
| assert(++array[0] == 1); | ||
| assert(--array[0] == -1); | ||
| assert(--array[0] == 0); | ||
| // BinAssignExp | ||
| assert((array[0] += 3) == 3); | ||
| } | ||
|
|
||
| // https://issues.dlang.org/show_bug.cgi?id=21312 | ||
| void check21312() | ||
| { | ||
| auto p = &[123][0]; |
There was a problem hiding this comment.
why is the test altered?
There was a problem hiding this comment.
Regarding check21312 (&[123][0]): This construct tries to take the address of an indexed array literal. With my changes, indexed array literals are correctly classified as rvalues to prevent the mutation issue (Issue 22328). Therefore, taking their address is no longer valid code. I had to alter the test because the original syntax is now a compile-time error.
Regarding enum -> auto change: The original test used an enum array (manifest constant). Modifying an element of an enum array (array[0]++) compiles to modifying a temporary literal, which is exactly what this PR forbids. To keep checking that the post/pre-increment operators work correctly on valid L-values, I changed the declaration from enum to auto.
There was a problem hiding this comment.
Additionally, this change aligns the compiler behavior with the D Language Specification regarding Manifest Constants, which explicitly states:
"Manifest constants are not lvalues, meaning their address cannot be taken."
Previously, the compiler incorrectly allowed taking the address of indexed array literals (which enums lower to), violating this rule.
|
cc @dkorpel since you wrote the PRs referenced here |
|
Per the current spec, while manifest constants are rvalues, indexing an array literal really is lvalue. It is actually handy to call C APIs like Your current implementation seems to spam several lines of error message for each assignment. Could you limit it to one message per assignment? |
4 participants
Problem:
Since DMD 2.095.1, attempting to modify an element of an enum array (manifest constant) compiles silently without error, but performs no modification at runtime. The compiler was incorrectly treating the indexed array literal as an lvalue.
Example:
#22328
https://dlang.org/spec/enum.html#manifest_constants