fix #21630: enum and alias ignored on runtime foreach loop variables

[tgehr]

Fixes #21630 . (Additionally adds tests for the existing checks in the analysis of ForeachStatement.)

[dlang-bot]

Thanks for your pull request and interest in making D better, @tgehr! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#21631"

[ntrel]

Looks like this breaks static RangeForeach:

compilable/json.d(136): Error: cannot declare `enum` loop variables for non-unrolled foreach
static foreach(enum i; 0..3)
                       ^

[tgehr]

oops

[tgehr]

Closing in favor of #21633

[tgehr]

I guess alternatively could remove the storage classes during the static foreach lowering.

[tgehr]

With the new patch the problem should be gone.

[Bolpat]

With the new patch the problem should be gone.

I don’t know if your implementation already does that, but it would be really nice if the indicator pointed at the offending keyword (alias or enum) and if there are several, one error message suffices.

Also, maybe give the error a supplement that says “use static foreach instead”. It’s very likely what the programmer intended.

When your PR gets merged, you’re absolutely free to close mine.

[tgehr]

Now -vcg-ast tests fail because it removes the storage class from the output.

[tgehr]

Hopefully the -vgc-ast issue is addressed too now.

With the new patch the problem should be gone.

I don’t know if your implementation already does that, but it would be really nice if the indicator pointed at the offending keyword (alias or enum) and if there are several, one error message suffices.

Also, maybe give the error a supplement that says “use static foreach instead”. It’s very likely what the programmer intended.
...

Ideally yes, though I think this is a different issue. Note that there is already this check, that works the same way:
https://github.com/dlang/dmd/blob/stable/compiler/src/dmd/statementsem.d#L879-L889

Feel free to open a separate enhancement request to address the diagnostics quality. IIRC when I originally added the message, columns were not even tracked yet in the compiler.

[tgehr]

Still failures related to json and vcg-ast, not sure how to reproduce. I cannot really spend a lot of time on this right now, I had thought this is a quick fix.

[tgehr]

The failure seems unrelated:

// https://issues.dlang.org/show_bug.cgi?id=3004
/*
REQUIRED_ARGS: -ignore -v
TRANSFORM_OUTPUT: remove_lines("^(predefs|binary|version|config|DFLAG|parse|import|\(imported|semantic|entry|library|function  object|function  core|\s*$)")
TEST_OUTPUT:
---
pragma    GNU_attribute (__error)
pragma    GNU_attribute (__error)
code      test3004
function  test3004.test
---
*/

extern(C) int printf(char*, ...);

pragma(GNU_attribute, flatten)
void test() { printf("Hello GNU world!\n".dup.ptr); }

pragma(GNU_attribute, flatten);

Test 'compilable/test3004.d' failed: 
expected:
----
pragma    GNU_attribute (__error)
pragma    GNU_attribute (__error)
code      test3004
function  test3004.test
----
actual:
----
pragma    GNU_attribute (__error)
pragma    GNU_attribute (__error)
inline scan test3004
code      test3004
function  test3004.test
----
diff:
----
 pragma    GNU_attribute (__error)
 pragma    GNU_attribute (__error)
+inline scan test3004
 code      test3004
 function  test3004.test
----

Not sure what inline scan test3004 means.

[tgehr]

@dkorpel @MoonlightSentinel It seems you added the output transformation and expected output. Any idea why an unexpected inline scan test3004 might appear here?

[rainers]

@dkorpel @MoonlightSentinel It seems you added the output transformation and expected output. Any idea why an unexpected inline scan test3004 might appear here?

The error is indeed unrelated and happens on stable, too: it needs the commit c9030ed currently only on master. There were some follow-up changes, too, but I think it can be cherry picked to stable.

AFAICT @dkorpel will merge master to stable before the next release, so you can also rebase this on master to avoid the issue.

[ibuclaw]

@dkorpel @MoonlightSentinel It seems you added the output transformation and expected output. Any idea why an unexpected inline scan test3004 might appear here?

The error is indeed unrelated and happens on stable, too: it needs the commit c9030ed currently only on master. There were some follow-up changes, too, but I think it can be cherry picked to stable.

AFAICT @dkorpel will merge master to stable before the next release, so you can also rebase this on master to avoid the issue.

Failure was introduced by e33c664. So a merge went wrong.

[tgehr]

Thanks! Rebased to master.

[tgehr]

Is master broken too?

It seems for buildkite the first error is:

std/array.d(1138): Error: template instance `_d_newarrayU!E` template `_d_newarrayU` is not defined, did you mean _d_newarrayT?
                ret = _d_newarrayU!E(size, isShared);

And for DAutoTest there seem to be parser errors related to => function syntax.

In both cases it seems unlikely that my pull request broke them.

[ibuclaw]

Is master broken too?

It seems for buildkite the first error is:

std/array.d(1138): Error: template instance `_d_newarrayU!E` template `_d_newarrayU` is not defined, did you mean _d_newarrayT?
                ret = _d_newarrayU!E(size, isShared);

And for DAutoTest there seem to be parser errors related to => function syntax.

In both cases it seems unlikely that my pull request broke them.

Did you rebase against current head?

c397436

All I see in master are what look like timed out failures in Azure and GHA/FreeBSD.

[ibuclaw]

The chance of failure caused by this PR is not zero.

std/array.d(1138): Error: template instance `_d_newarrayU!E` template `_d_newarrayU` is not defined, did you mean _d_newarrayT?
                ret = _d_newarrayU!E(size, isShared);
                      ^
std/array.d(1000): Error: template instance `std.array.arrayAllocImpl!(false, ulong[], ulong)` error instantiating
    return arrayAllocImpl!(false, T, ST)(sizes);
                                        ^
std/array.d(126):        instantiated from here: `uninitializedArray!(ulong[], const(ulong))`
        auto result = (() @trusted => uninitializedArray!(Unqual!E[])(length))();
                                                                     ^
std/meta.d(964):        instantiated from here: `array!(Result)`
        static foreach (size_t i, el; iter.array)
                                          ^
std/range/package.d(1100):        instantiated from here: `aliasSeqOf!(Result(0LU, 2LU))`
                        Result(staticMap!(saveI, aliasSeqOf!(R.length.iota)));
                                                 ^
std/bitmanip.d(2655):        instantiated from here: `chain!(Result, FilterResult!(__lambda_L2661_C26, Result))`
        return chain(
                    ^
std/meta.d(964): Error: CTFE failed because of previous errors in `array`
        static foreach (size_t i, el; iter.array)
                                          ^
std/meta.d(964): Error: CTFE failed because of previous errors in `array`
        static foreach (size_t i, el; iter.array)
                                          ^

And

std/path.d(1679):        instantiated from here: `chain!(ByCodeUnitImpl, OnlyResult!char, ByCodeUnitImpl)`
        return chain(r1[0 .. pos].byUTF!CR, sep, r2.byUTF!CR);
                    ^
std/path.d(1478):        instantiated from here: `chainPath!(char[], const(char)[])`
        auto r = chainPath(buf[0 .. pos], segment);
                          ^
std/path.d(1496):        instantiated from here: `buildPath!(const(char)[][])`
    return buildPath!(typeof(paths))(paths);
                                    ^
std/file.d(140):        instantiated from here: `buildPath!char`
        fileName = text(buildPath(tempDir(), base), thisProcessID);
                                 ^
std/path.d(1480): Error: invalid `foreach` aggregate `r` of type `Result`
        foreach (c; r)
        ^
std/path.d(1480):        `foreach` works with input ranges (implementing `front` and `popFront`), aggregates implementing `opApply`, or the result of an aggregate's `.tupleof` property
std/path.d(1480):        https://dlang.org/phobos/std_range_primitives.html#isInputRange

[ibuclaw]

I noticed that the last buildkite run used stable as the target branch when cloning Phobos.

Related PR dlang/phobos#10819

Not convinced that it introduced something, as _d_newarrayU is publicly imported into every module.

dmd/druntime/src/object.d

Line 4795 in bc6dd9d

public import core.internal.array.construction : _d_newarrayU;

[ibuclaw]

I noticed that the last buildkite run used stable as the target branch when cloning Phobos.

Related PR dlang/phobos#10819

Not convinced that it introduced something, as _d_newarrayU is publicly imported into every module.

Ah, it does matter, because #21525