Enable preview=fieldwise & fix blocking bug in VariantN

[MoonlightSentinel]

The first commit removes unreachable code in VaraintN for struct without members. This bug was exposed by -preview=fieldwise wich enables early const-folding for EmptyStruct == EmptyStruct (always true).

The second commit enables the flag for phobos s.t. it stays compatible in the future.

[dlang-bot]

Thanks for your pull request and interest in making D better, @MoonlightSentinel! 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

Auto-close	Bugzilla	Severity	Description
✓	21231	normal	Unreachable warning for empty struct in VariantN with preview=fieldwise

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 + phobos#7622"

[MoonlightSentinel]

Blocked because it won't compile without -preview=fieldwise after the bugfix.

Guess we should finally have a way to silence the warning in generic code (because detecting that the code is unreachable is impossible/really akward here)

[nordlow]

Why not return 0; here?

[MoonlightSentinel]

Because it would be unreachable code - diagnosed with -preview=fieldwise.

[nordlow]

What about moving

if (*rhsPA == *zis) { return 0; }

as two separate code duplications into the else static if and else branches? I guess we can afford some code duplication here.

That should remove the warning for the

static if (is(A == struct) && A.init.tupleof.length == 0)

case right?

[nordlow]

This compiles for me without the flag -preview=fieldwise:

        static ptrdiff_t compare(A* rhsPA, A* zis, OpID selector)
        {
            static if (is(typeof(*rhsPA == *zis)))
            {
                enum isEmptyStruct = is(A == struct) && A.init.tupleof.length == 0;
                static if (!isEmptyStruct)
                {
                    if (*rhsPA == *zis)
                        return 0;
                }
                static if (isEmptyStruct)
                {
                    return 0;
                    // The check above will always succeed if A is an empty struct.
                    // Don't generate unreachable code as seen in
                    // https://issues.dlang.org/show_bug.cgi?id=21231
                }
                else static if (is(typeof(*zis < *rhsPA)))
                {
                    // Many types (such as any using the default Object opCmp)
                    // will throw on an invalid opCmp, so do it only
                    // if the caller requests it.
                    if (selector == OpID.compare)
                        return *zis < *rhsPA ? -1 : 1;
                    else
                        return ptrdiff_t.min;
                }
                else
                {
                    // Not equal, and type does not support ordering
                    // comparisons.
                    return ptrdiff_t.min;
                }
            }
            else
            {
                // Type does not support comparisons at all.
                return ptrdiff_t.min;
            }
        }

[MoonlightSentinel]

That would also need to check for user-defined opEquals inside of an empty struct.

[MoonlightSentinel]

The user-defined opEquals may have different semantics than the compiler generated one (e.g. return false
). Your proposal skips it and simply returns "equal".

Additionally, the warning disappears when the empty struct defines an opEquals (see https://run.dlang.io/is/rvo4Yt), which suggests that it breaks the early const-folding. That would trigger the missing return,... again

[nordlow]

The user-defined opEquals may have different semantics than the compiler generated one (e.g. return false
). Your proposal skips it and simply returns "equal".

What about statically checking for presence of user-defined opEquals and avoid special handling then?

Have you asked @WalterBright how to proceed with these problems?

[MoonlightSentinel]

What about statically checking for presence of user-defined opEquals and avoid special handling then?

That could work

Have you asked @WalterBright how to proceed with these problems?

No and I don't see a reason to do so. He rarely contributes to phobos and there's more imporant work only he can do (e.g. review backend pull requests)

[nordlow]

Ok, thanks.

[wilzbach]

Yup, the bright side of the lack of leadership is that we have more decision freedom here than at DMD ;-)

[nordlow]

Blocked because it won't compile without -preview=fieldwise after the bugfix.

How does this error?

[MoonlightSentinel]

How does this error?

Missing return.., assert(0), ... at the end of the function.

[nordlow]

Missing return.., assert(0), ... at the end of the function.

I believe I have a solution. See code above.

[MoonlightSentinel]

I believe I have a solution. See code above.

Nice.

But i really think the compiler should ignore "unused code" for such scenarios. Trying to find an akward workaround wastes a lot of a developers time (and probably increases compilation time) for very little benefit.

[nordlow]

Are you putting this in another PR?

[nordlow]

I have an idea. Instead of disabling unreachable code analysis, what about making its diagnostics better in conveying to the developer how he should change the code to avoid the warning.

For instance

struct S {} // line 1

void f(S s, S y) // line 3
{
    if (x == y) // line 5
        return true; // line 6
    return false; // line 7
}

could instead say something like

line 7: statement/line is unreachable, because
line 5: expression `x == y` is always true, resulting in
line 6: unconditional return here (instead)

. I realize that there may be more complicated cases to describe than this, though.

@MoonlightSentinel?

[nordlow]

I hope it's ok that I reopen this.

[MoonlightSentinel]

Well, better diagnostics for non-templated code would be nice indeed. But that doesn't help code where "unreachable" is dependent on template parameters (and somewhat on compiler flags).

Also note that the compiler should only suppress "unreachable code" warnings when there is a conditionally compiled return, throw, ..., not in general.