Issue 14835 - Constant folding should not affect front end flow analysis
Summary: Constant folding should not affect front end flow analysis
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P2 major
Assignee: No Owner
URL:
Keywords: pull
: 15166 (view as issue list)
Depends on:
Blocks:
 
Reported: 2015-07-26 17:49 UTC by Mathias LANG
Modified: 2023-10-01 16:36 UTC (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Mathias LANG 2015-07-26 17:49:06 UTC
The "statement is not reachable" warning is a major annoyance when writting generic code. It often end up being more of a disturbance than an actual help.

The reason is that it doesn't accept code that would be common at runtime.
Consider the following example:

````
bool isEven(int i)
{
   if (i % 2)
     return true;
   return false;
}

````

In normal code, requiring `return false;` to be in an `else` branch would be a major annoyance for no benefit. Now take the exact same code, but at CT:

````
bool isEven(int i)()
{
   static if (i % 2) // static or not, the warning will get triggered if i is even
     return true;
   return false;
}

````

This is a simple example, but I believe it illustrate the problem well enough.

In addition to the "put everything in a branch" problem, there are some cases where it simply forces you to move to recursion everywhere because there's no way to fix the warning, for example: https://github.com/rejectedsoftware/vibe.d/blob/38784138ad8cc8f442bd0e76a1b740040ff33fe5/source/vibe/internal/meta/typetuple.d#L101-L121

This code can only be fixed in 2 ways: either move to recursion, or use a dummy boolean parameter to confuse DMD's flow analysis (example: https://github.com/rejectedsoftware/vibe.d/blob/38784138ad8cc8f442bd0e76a1b740040ff33fe5/source/vibe/web/rest.d#L1150-L1154 ).
Comment 1 Martin Nowak 2015-10-12 23:00:17 UTC
*** Issue 15166 has been marked as a duplicate of this issue. ***
Comment 2 Steven Schveighoffer 2015-10-13 16:11:43 UTC
Why can't the "hack" variable be eliminated by enclosing the rest of the function in the else branch?

And what about this solution for the group comparison (copied from my post in Issue 15166):

private bool compare(alias Group1, alias Group2)()
{
    foreach (index, element; Group!(Group1.expand, void).expand)
    {
        static if(index == Group1.expand.length)
            return true;
        else static if (!is(Group1.expand[index] == Group2.expand[index]))
            return false;
    }
}

Neither requires recursion.
Comment 3 Mathias LANG 2015-10-13 18:35:57 UTC
Note that with this implementation you get an error (missing return) in 2.068.0, but 2.069.0-b1 fixed that.

But the point is not about the actual implementation, but the effect of this warning. It makes meta code much harder to write that it's needed, and way less natural. Will you accept a language where every `if` statement has to be followed by an `else` statement ?
Comment 4 Steven Schveighoffer 2015-10-13 20:23:56 UTC
What the compiler is doing here is giving you information that code you wrote will not get executed.

The problem is that the code in question is only one *particular* execution (or instantiation) of that code.

To your example, isEven!1 will execute the line of code that is deemed to be unreachable.

The issue here is that the compiler can't "try all possibilities" to see if it will be able to find a path to that code (halting problem).

So probably we should turn off that feature when the code has taken a branch based on a template constant or a static if. The compiler should assume the other branch could be executed for a different instantiation, and so not complain for this one.

I don't think there's a "right" way to handle this. The error in question is truly a function of optimization and folding, but the user sees things in terms of lines of code. To say a line of code may not be executed if you call it one way is an error, even though it will be executed if you call it another way, doesn't make a lot of sense. If we are going to be "helpful", we should at least be accurate.

This is coming from someone who doesn't know how the compiler is implemented, or doesn't even know how compilers are implemented. So perhaps this is too difficult a task?

BTW, I was also saying that you *could* fix the problem without dummy variables or recursion, but I agree that the compiler is being unhelpful here.
Comment 5 thomas.bockman 2015-10-24 05:51:39 UTC
This issue will block for further improvements to constant folding and value range propagation, as better folding and VRP make it noticeably worse.

I found this out by adding VRP-based constant folding for integer comparisons, and then trying to compile Phobos and vibe.d - see:
    https://github.com/D-Programming-Language/dmd/pull/5229
Comment 6 thomas.bockman 2015-10-24 08:31:32 UTC
Here's a minimal compilable (requires dmd argument -wi, rather than -w) example, for anyone trying to fix this:

module main;

import std.stdio;

void reachIf(bool x)()
{
    if(!x)
        return;
    writeln("reached"); // Warning: statement is not reachable
}

void main(string[] args) {
    reachIf!true();  // prints "reached"
    reachIf!false(); // triggers warning
}
Comment 7 Jack Stouffer 2016-05-09 15:45:51 UTC
This has come up again: https://github.com/dlang/phobos/pull/4287

This issue is a major annoyance when writing template code and almost anyone writing optimized code with static if's will run into it eventually, so I'm raising the importance of this.
Comment 8 Walter Bright 2016-10-12 03:46:18 UTC
It seems the problem lies here:

https://github.com/dlang/dmd/blob/master/src/statement.d#L453

                if (s.condition.isBool(true))
                {
                    if (s.ifbody)
                        result |= s.ifbody.blockExit(func, mustNotThrow);
                    else
                        result |= BEfallthru;
                }
                else if (s.condition.isBool(false))
                {
                    if (s.elsebody)
                        result |= s.elsebody.blockExit(func, mustNotThrow);
                    else
                        result |= BEfallthru;
                }
                else

Having it set BEfallthru as if both ifbody and elsebody could execute should work, but I wonder about its affect on existing code.
Comment 9 anonymous4 2016-10-18 09:11:31 UTC
(In reply to Walter Bright from comment #8)
> but I wonder about its affect on existing code.

Will it disable all unreachable code checks?
Try this:

int square(int num)
{
   if(true)return 0;
   return num * num;
}
Comment 10 Andrei Alexandrescu 2019-05-28 16:16:30 UTC
One simple possibility here is to automatically insert `else` whenever the `static if` branch does not fall through
Comment 11 Dlang Bot 2021-03-28 06:13:42 UTC
@WalterBright created dlang/dmd pull request #12311 "fix Issue 14835 - Constant folding should not affect front end flow a…" fixing this issue:

- fix Issue 14835 - Constant folding should not affect front end flow analysis

https://github.com/dlang/dmd/pull/12311
Comment 12 deadalnix 2021-03-29 02:05:27 UTC
(In reply to Andrei Alexandrescu from comment #10)
> One simple possibility here is to automatically insert `else` whenever the
> `static if` branch does not fall through

This is the most underrated comment of the thread ^

I actually implemented this in SDC, and it makes a HUGE difference.
Comment 13 Dlang Bot 2023-09-21 12:48:46 UTC
@adamdruppe updated dlang/dmd pull request #15568 "make D great again" fixing this issue:

- Fix 10532, 14835, 20522 by adding test cases

https://github.com/dlang/dmd/pull/15568
Comment 14 Dlang Bot 2023-10-01 16:36:36 UTC
dlang/dmd pull request #15568 "make D great again" was merged into master:

- cfd7cfd4382803e0252ebe1cf13d4c39a60209e5 by Dennis Korpel:
  Fix 10532, 14835, 20522 by adding test cases

https://github.com/dlang/dmd/pull/15568