www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Fixing spurious "statement is not reachable" in template code

reply tsbockman <thomas.bockman gmail.com> writes:
While improving the DMD front-end's constant folding:
     https://github.com/D-Programming-Language/dmd/pull/5229
I found out about DMD issue 14835:
     https://issues.dlang.org/show_bug.cgi?id=14835

Briefly:
///////////////////////////////
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
}
///////////////////////////////

This is, I think, a big problem.

Affected code is rare today, but that is only because DMD's 
constant folding and value-range-propagation is weak. The more 
improvements are made in this area, the more common erroneous 
"statement is not reachable" warnings will become.

Unfortunately, from what I can tell, this bug is just a natural 
consequence of DMD's current design; I think an ideal fix will 
not be simple.

Some possible solutions:

1. Defer "not reachable" warnings until compilation has been 
completed, and only issue the warning if *all* instantiations of 
the statement were unreachable.

2. For semantic analysis purposes, first instantiate each 
template using dummy parameters with the widest possible VRP 
ranges. Only statements found to be "not reachable" in this dummy 
run should actually generate warnings.

3. ??? I don't know the compiler internals very well, so I may be 
missing a more elegant solution.


properly, it would produce precisely the results I expect, as a 
user.

However, I think it would take a huge patch touching many 
different files and functions to implement. This seems excessive, 
assuming the only benefit is eliminating some spurious warnings.


quite rare, and also solvable by the user. Otherwise, this seems 
like a good approach:

* The size and complexity of the patch should be more reasonable.

* If all messages are deferred, it should be easy to condense the 
floods of duplicate messages generated by templates. Instead of 
getting the same message duplicated once per instantiation, you 
could just have one copy with a `x37` next to it or something.

* Likewise, if desired, messages could be sorted by file and line.


will not be printed until compilation is completed - which might 
be never, if the compiler hangs or something.

Thoughts?
Oct 24 2015
next sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 10/24/15 1:25 PM, tsbockman wrote:

 While improving the DMD front-end's constant folding:
      https://github.com/D-Programming-Language/dmd/pull/5229
 I found out about DMD issue 14835:
      https://issues.dlang.org/show_bug.cgi?id=14835

 Briefly:
 ///////////////////////////////
 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
 }
 ///////////////////////////////

 This is, I think, a big problem.


I agree


 Affected code is rare today, but that is only because DMD's constant
 folding and value-range-propagation is weak. The more improvements are
 made in this area, the more common erroneous "statement is not
 reachable" warnings will become.

 Unfortunately, from what I can tell, this bug is just a natural
 consequence of DMD's current design; I think an ideal fix will not be
 simple.

 Some possible solutions:

 1. Defer "not reachable" warnings until compilation has been completed,
 and only issue the warning if *all* instantiations of the statement were
 unreachable.


This isn't good either. One instantiation of reachIf being able to 
compile shouldn't be dependent on whether another one was ever used.


 2. For semantic analysis purposes, first instantiate each template using
 dummy parameters with the widest possible VRP ranges. Only statements
 found to be "not reachable" in this dummy run should actually generate
 warnings.


How does the compiler figure this out? This seems like the halting 
problem to me.


 3. ??? I don't know the compiler internals very well, so I may be
 missing a more elegant solution.


I think the easiest solution would be to just give up on worrying about 
unreachable code if the branch is dependent on a compile time variable 
that could legitimately change from instantiation to instantiation. In 
other words, for that instantiation, you obviously don't need to include 
the code, but you can't declare that the line of code is unreachable.

This means that this will compile:

void foo(int x)()
{
    if(x == 5)
        return;
    writeln("boo!");
}

void main()
{
    foo!5();
}

But this will not:

void main()
{
    enum x = 5;
    if(x == 5)
       return;
    writeln("boo!");
}

Even though they are effectively equivalent. But that's not a good 
reason to create errors which are obviously untrue. In the second case, 
the compiler can know definitively that the statement really will never 
be executed or compiled. In the first case, it can only be sure for that 
*instance*.

Even this could compile IMO (but a sufficiently smarter compiler may 
complain):

void foo(int x)() if(x == 5)
{
    if(x == 5)
       return;
    writeln("boo!");
}

Let's not forget that "unreachable statement" errors are really not 
errors in the sense that it will cause a crash, or corrupt memory. It 
just means some statements you wrote were wasted effort. It's OK to

-Steve
Oct 26 2015
parent reply tsbockman <thomas.bockman gmail.com> writes:
On Monday, 26 October 2015 at 12:31:37 UTC, Steven Schveighoffer 
wrote:

 Some possible solutions:

 1. Defer "not reachable" warnings until compilation has been 
 completed,
 and only issue the warning if *all* instantiations of the 
 statement were
 unreachable.


 This isn't good either. One instantiation of reachIf being able 
 to compile shouldn't be dependent on whether another one was 
 ever used.


I agree this is not ideal, however it would be much better than 
what we have currently, and the compiler implementation should be 
relatively simple.


 2. For semantic analysis purposes, first instantiate each 
 template using
 dummy parameters with the widest possible VRP ranges. Only 
 statements
 found to be "not reachable" in this dummy run should actually 
 generate
 warnings.


 How does the compiler figure this out? This seems like the 
 halting problem to me.



parameter stuff is my vague proposal for a way to actual 
implement this behavior in the compiler.

The halting problem is no more of an issue than it is for the 
compiler today. (That is to say, the halting problem leads to 
false negatives, but not false positives.)

I think the compiler currently does something like this:

void main(string[] args) {
     reachIf!true();  // prints "reached"
     reachIf!false(); // triggers warning
}

// A template will be instantiated and undergo semantic analysis
// once for each combination of CT parameters fed to it in the 
program.

// reachIf!true pass:
void reachIf(bool x)() // VRP narrows x to true
{
     if(!x) // VRP + constant folding says (!true) always == false
         return; // never reached (this doesn't always generate a 
warning)
     writeln("reached"); // always reached
}

// reachIf!false pass:
void reachIf(bool x)() // VRP narrows x to false
{
     if(!x) // VRP + constant folding says (!false) always == true
         return; // always reached
     writeln("reached"); // Warning: statement is not reachable
}



// first instantiate the template with the widest possible
// VRP ranges for the CT paramters
void reachIf(bool x)() // VRP says x could be true or false
{
     if(!x) // not constant; cannot be folded
         return; // reachable
     writeln("reached"); // reachable
}

"statement not reachable" warnings would only be generated during 
this preliminary pass; they would be suppressed in the 
reachIf!true and reachIf!false passes.


a dummy type will have to be created to apply this solution to 
cases like:

module main;

import std.stdio;

void reachIf(T...)()
{
     if(T.length != 1 || T[0].sizeof != 4)
         return;
     writeln("reached"); // Warning: statement is not reachable
}

void main(string[] args) {
     reachIf!int();  // prints "reached"
     reachIf!long(); // triggers warning

     stdin.readln();
}

(Note that this case can only be reproduced with my constant 
folding upgrade patch applied.)


AliasSeq of indeterminate length, whose elements have a sizeof 
property of indeterminate value. I strongly suspect that a fake 
type like that will require explicit support to be added all over 
the DMD code base; it would be a huge change just to eliminate 
some spurious warnings.
Oct 26 2015
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 10/26/15 1:08 PM, tsbockman wrote:

 On Monday, 26 October 2015 at 12:31:37 UTC, Steven Schveighoffer wrote:

 Some possible solutions:

 1. Defer "not reachable" warnings until compilation has been completed,
 and only issue the warning if *all* instantiations of the statement were
 unreachable.


 This isn't good either. One instantiation of reachIf being able to
 compile shouldn't be dependent on whether another one was ever used.


 I agree this is not ideal, however it would be much better than what we
 have currently, and the compiler implementation should be relatively
 simple.


 2. For semantic analysis purposes, first instantiate each template using
 dummy parameters with the widest possible VRP ranges. Only statements
 found to be "not reachable" in this dummy run should actually generate
 warnings.


 How does the compiler figure this out? This seems like the halting
 problem to me.



 stuff is my vague proposal for a way to actual implement this behavior
 in the compiler.

 The halting problem is no more of an issue than it is for the compiler
 today. (That is to say, the halting problem leads to false negatives,
 but not false positives.)


OK, as long as the default behavior isn't to reject the code, I suppose 
this is simply an optimization. But the problem I see is that only 
static ifs that reduce to static if(true) would be considered to cause a 
short-circuit (e.g. someint <= int.max). Unless the compiler can VRP 
according to the template constraint, which may be possible in simple 
circumstances, but not in all circumstances.

It just seems like it's an unnecessary layer. I think we should start 
with the simple accept all code that branches based on compile-time 
variables.



 type will have to be created to apply this solution to cases like:

 module main;

 import std.stdio;

 void reachIf(T...)()
 {
      if(T.length != 1 || T[0].sizeof != 4)
          return;
      writeln("reached"); // Warning: statement is not reachable
 }


Inherent properties of types that can be variable could be considered 
variables just like any other size_t or int. Therefore, branching based 
on that would fall under the same issue.

I think the "ton of work" is avoidable.

-Steve
Oct 27 2015
parent reply tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 27 October 2015 at 12:28:38 UTC, Steven Schveighoffer 
wrote:

 On 10/26/15 1:08 PM, tsbockman wrote:


 parameter
 stuff is my vague proposal for a way to actual implement this 
 behavior
 in the compiler.

 The halting problem is no more of an issue than it is for the 
 compiler
 today. (That is to say, the halting problem leads to false 
 negatives,
 but not false positives.)


 OK, as long as the default behavior isn't to reject the code, I 
 suppose this is simply an optimization.


Right.


 But the problem I see is that only static ifs that reduce to 
 static if(true) would be considered to cause a short-circuit 
 (e.g. someint <= int.max). Unless the compiler can VRP 
 according to the template constraint, which may be possible in 
 simple circumstances, but not in all circumstances.


I think the next piece of Lionello Lunesu's PR which I am 
updating might actually do that; I'll have to check later. 

the opportunities for constant folding.


 It just seems like it's an unnecessary layer. I think we should 
 start with the simple accept all code that branches based on 
 compile-time variables.


Can outline which specific code in the compiler you would modify 
to accomplish this?

Because at the moment, having looked at the relevant front end 
code, I don't see a way that is meaningfully simpler than my 
dummy parameter idea.



 that a dummy
 type will have to be created to apply this solution to cases 
 like:

 module main;

 import std.stdio;

 void reachIf(T...)()
 {
      if(T.length != 1 || T[0].sizeof != 4)
          return;
      writeln("reached"); // Warning: statement is not reachable
 }


 Inherent properties of types that can be variable could be 
 considered variables just like any other size_t or int. 
 Therefore, branching based on that would fall under the same 
 issue.

 I think the "ton of work" is avoidable.


I have only a fuzzy understanding of the compiler's code right 
now, but it seems to me that the way it is currently structured 
does not readily allow for simultaneously making information 
available to `static if` and CTFE, while also hiding it from dead 
code detection.

I get what you want, but if it's simple to actually make the 
compiler do it, I don't see how yet. If you do, please give me 
the names of some files or functions to study in the front end.
Oct 27 2015
parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 10/27/15 12:04 PM, tsbockman wrote:

 On Tuesday, 27 October 2015 at 12:28:38 UTC, Steven Schveighoffer wrote:

 On 10/26/15 1:08 PM, tsbockman wrote:


 stuff is my vague proposal for a way to actual implement this behavior
 in the compiler.

 The halting problem is no more of an issue than it is for the compiler
 today. (That is to say, the halting problem leads to false negatives,
 but not false positives.)


 OK, as long as the default behavior isn't to reject the code, I
 suppose this is simply an optimization.


 Right.


 But the problem I see is that only static ifs that reduce to static
 if(true) would be considered to cause a short-circuit (e.g. someint <=
 int.max). Unless the compiler can VRP according to the template
 constraint, which may be possible in simple circumstances, but not in
 all circumstances.


 I think the next piece of Lionello Lunesu's PR which I am updating might
 actually do that; I'll have to check later. Regardless, you are correct

 folding.


I'm not a compiler dev, so I'm not sure how it works or should work. I'm 
looking at this from a user standpoint.

I didn't consider that the compiler is likely already using VRP to 
determine whether a line of code is unnecessary. If it's a matter of 
just keeping track of 2 VRP ranges (the actual VRP and the VRP just for 
checking reachability) for each line of code, then perhaps it's just 
easier to extend VRP. I don't know the answer.

All I was saying is that it's not necessary to add on an additional 
layer if it's not present.

I defer to the actual devs as to what is easier.

-Steve
Oct 27 2015
prev sibling next sibling parent reply Daniel Murphy <yebbliesnospam gmail.com> writes:
On 25/10/2015 4:25 AM, tsbockman wrote:

 ///////////////////////////////
 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
 }
 ///////////////////////////////




 Thoughts?


Easy to fix:

void reachIf(bool x)()
{
      static if(!x)
          return;
      else
          writeln("reached");
}

The warning is correct, and incredibly annoying.
Oct 27 2015
next sibling parent tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 27 October 2015 at 07:50:37 UTC, Daniel Murphy wrote:

 Easy to fix:

 void reachIf(bool x)()
 {
      static if(!x)
          return;
      else
          writeln("reached");
 }

 The warning is correct, and incredibly annoying.


Yes, it is easy to work around the problem in my simple example 
code. Real world examples are not generally so simple, though - 
often, an equivalent fix would require a `static foreach(...) { 
... } else` construct to avoid adding a redundant (and possibly 
quite complex) predicate.

No, the warning is not correct.

When a "statement is not reachable" warning is generated during 
analysis of a regular function, it means that that statement can 
*never* execute, no matter what parameters are fed in.

This is a very useful warning, because why would the programmer 
deliberately write dead code? Clearly something is wrong (whether 
in the code being compiled, or the compiler itself).

On the other hand, when a "statement is not reachable" warning is 
generated based on a specific combination of compile-time 
parameters, there is likely nothing wrong with the code.

The purpose of templates is code reuse. Forcing the programmer to 
manually prune each possible instantiation by sprinkling `static 
if`s with redundant predicates everywhere hinders this goal.
Oct 27 2015
prev sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 10/27/15 3:50 AM, Daniel Murphy wrote:

 On 25/10/2015 4:25 AM, tsbockman wrote:

 ///////////////////////////////
 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
 }
 ///////////////////////////////




 Thoughts?


 Easy to fix:

 void reachIf(bool x)()
 {
       static if(!x)
           return;
       else
           writeln("reached");
 }

 The warning is correct, and incredibly annoying.


Easy to fix, but the warning is incorrect, the statement is reachable if 
you use reachIf!true.

A statement is not a compiled piece of code.

-Steve
Oct 27 2015
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 10/27/2015 01:35 PM, Steven Schveighoffer wrote:

 Easy to fix:

 void reachIf(bool x)()
 {
       static if(!x)
           return;
       else
           writeln("reached");
 }

 The warning is correct, and incredibly annoying.


 Easy to fix, but the warning is incorrect, the statement is reachable if
 you use reachIf!true.

 A statement is not a compiled piece of code.

 -Steve


Versions of the same statement in different instantiations are 
independent. Templates are just a restricted form of hygienic macros.
Oct 27 2015
next sibling parent reply tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 27 October 2015 at 13:23:51 UTC, Timon Gehr wrote:

 On 10/27/2015 01:35 PM, Steven Schveighoffer wrote:

 Easy to fix:

 void reachIf(bool x)()
 {
       static if(!x)
           return;
       else
           writeln("reached");
 }

 The warning is correct, and incredibly annoying.


 Easy to fix, but the warning is incorrect, the statement is 
 reachable if
 you use reachIf!true.

 A statement is not a compiled piece of code.

 -Steve


 Versions of the same statement in different instantiations are 
 independent. Templates are just a restricted form of hygienic 
 macros.


That's how the current implementation works, but that doesn't 
mean the warning is actually *helpful*.
Oct 27 2015
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday, 27 October 2015 at 16:05:31 UTC, tsbockman wrote:

 On Tuesday, 27 October 2015 at 13:23:51 UTC, Timon Gehr wrote:

 Versions of the same statement in different instantiations are 
 independent. Templates are just a restricted form of hygienic 
 macros.


 That's how the current implementation works, but that doesn't 
 mean the warning is actually *helpful*.


Well, arguably that's exactly what templates _are_ regardless of 
the implementation - i.e. a template is just a template to 
generate code, not really actual code in and of itself. Now, that 
being said, I'm not sure that warning about unreachable code in a 
templated function is particularly helpful - particularly if it 
forces you to jump through hoops to make the compiler shut up 
about it. Heck, if anything, I find that warning/error annoying 
in regular code, because it tends to get in the way of debugging 
while developing. It wouldn't really hurt my feelings any if we 
just removed it from the compiler entirely - though I completely 
agree that you don't really want to have unreachable code left in 
your production code.

- Jonathan M Davis
Oct 27 2015
parent tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 27 October 2015 at 17:23:21 UTC, Jonathan M Davis 
wrote:

 On Tuesday, 27 October 2015 at 16:05:31 UTC, tsbockman wrote:

 On Tuesday, 27 October 2015 at 13:23:51 UTC, Timon Gehr wrote:

 Versions of the same statement in different instantiations 
 are independent. Templates are just a restricted form of 
 hygienic macros.


 That's how the current implementation works, but that doesn't 
 mean the warning is actually *helpful*.


 Well, arguably that's exactly what templates _are_ regardless 
 of the implementation - i.e. a template is just a template to 
 generate code, not really actual code in and of itself.


That is one (valid) way of thinking about templates.

Another perspective, though, which I picked up from someone 
(Andrei Alexandrescu, I think?) in the D community, is to 
consider template parameters simply as additional function 
arguments, which happen to be evaluated at compile time. In many 
cases, the timing of their evaluation is just an implementation 
detail - a performance optimization (or de-optimization, as the 
case may be).

Thinking about templates in this way leads naturally to Steven 
Schveighoffer's comparison with function inlining, above.


 Now, that being said, I'm not sure that warning about 
 unreachable code in a templated function is particularly 
 helpful - particularly if it forces you to jump through hoops 
 to make the compiler shut up about it. Heck, if anything, I 
 find that warning/error annoying in regular code, because it 
 tends to get in the way of debugging while developing. It 
 wouldn't really hurt my feelings any if we just removed it from 
 the compiler entirely - though I completely agree that you 
 don't really want to have unreachable code left in your 
 production code.

 - Jonathan M Davis


I find the warning helpful; it just needs to be limited to cases 
in which the code is actually unreachable. Removing it entirely 
wouldn't be a disaster, but we'd have to think carefully about 
whether the payoff of improvements to VRP and constant folding 
was really worth it.


reachable" warnings until all instantiations of a template have 
been analyzed, and only issue the warning if it applies to every 
one of them?
Oct 27 2015
prev sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 10/27/15 9:23 AM, Timon Gehr wrote:

 On 10/27/2015 01:35 PM, Steven Schveighoffer wrote:

 Easy to fix:

 void reachIf(bool x)()
 {
       static if(!x)
           return;
       else
           writeln("reached");
 }

 The warning is correct, and incredibly annoying.


 Easy to fix, but the warning is incorrect, the statement is reachable if
 you use reachIf!true.

 A statement is not a compiled piece of code.


 Versions of the same statement in different instantiations are
 independent. Templates are just a restricted form of hygienic macros.


I understand how the compiler treats it. But it still doesn't make the 
warning true. In some cases, the statement is reachable, the compiler is 
unhelpfully pointing out cases where it was unreachable.

It would be if the compiler had a function like this:

int foo(bool x)
{
    if(x == 5) return 1;
    return 0;
}

And you compile this function:

void main()
{
    foo(5);
}

with -inline, the compiler complained. It's unhelpful that you are 
telling me that you are not generating code for my statement *in this 
instance*.

If you want to tell me that you would *never* generate code for my 
statement in *any* instance, that is helpful.

-Steve
Oct 27 2015
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 10/27/2015 06:35 PM, Steven Schveighoffer wrote:

 I understand how the compiler treats it.  In some cases, the statement
 is reachable, the compiler is unhelpfully pointing out cases where it
 was unreachable.


The reachable statement is not the same as the statement that is flagged 
unreachable. This is not an implementation detail and it is not 
'incorrect'. It might be changed if the consensus is that it is 
unhelpful. Personally, I usually avoid generating dead code.
Oct 27 2015
parent reply tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 27 October 2015 at 19:30:08 UTC, Timon Gehr wrote:

 On 10/27/2015 06:35 PM, Steven Schveighoffer wrote:

 I understand how the compiler treats it.  In some cases, the 
 statement
 is reachable, the compiler is unhelpfully pointing out cases 
 where it
 was unreachable.


 The reachable statement is not the same as the statement that 
 is flagged unreachable. This is not an implementation detail 
 and it is not 'incorrect'. It might be changed if the consensus 
 is that it is unhelpful. Personally, I usually avoid generating 
 dead code.


I don't think any dead code is being generated, in the cases 
where the compiler knows enough to issue the "statement is not 
reachable" warning. My testing indicates that when the compiler 
decides code is unreachable, it just removes it.

(I found this out because of a bug in the compiler:
     
http://forum.dlang.org/thread/qjmikijfluaniwnxhigp forum.dlang.org)

Any fix for issue 14835 should still allow the compiler to detect 
and remove dead code in template instantiations - but in cases 
where the code could be reached with a different set of template 
parameters, it should be removed silently, without the warning.

This would make it consistent with the behaviour of inlining, as 
Steven Schveighoffer pointed out.
Oct 27 2015
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 10/27/2015 09:18 PM, tsbockman wrote:

 On Tuesday, 27 October 2015 at 19:30:08 UTC, Timon Gehr wrote:

 On 10/27/2015 06:35 PM, Steven Schveighoffer wrote:

 I understand how the compiler treats it.  In some cases, the statement
 is reachable, the compiler is unhelpfully pointing out cases where it
 was unreachable.


 The reachable statement is not the same as the statement that is
 flagged unreachable. This is not an implementation detail and it is
 not 'incorrect'. It might be changed if the consensus is that it is
 unhelpful. Personally, I usually avoid generating dead code.


 I don't think any dead code is being generated,


This seems to be a misunderstanding. I mean generated using mixins or 
template instantiation. Sure, it will usually be removed, but why 
generate and semantically analyze it in the first place.
Oct 27 2015
parent reply tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 27 October 2015 at 21:14:26 UTC, Timon Gehr wrote:

 On 10/27/2015 09:18 PM, tsbockman wrote:

 I don't think any dead code is being generated,


 This seems to be a misunderstanding. I mean generated using 
 mixins or template instantiation. Sure, it will usually be 
 removed, but why generate and semantically analyze it in the 
 first place.


Forcing me to add `static if`s with redundant and potentially 
complex predicates just to make my code do the exact same thing 
it would have done anyway is a violation of "Don't Repeat 
Yourself", with all of the usual consequences:

* The more places the same logic is expressed, the more chances I 
have to get it wrong and cause a bug.

* Even if I get it right the first time, a redundant predicate 
could get out of sync with the rest of the code later, causing a 
bug.

* It's a waste of my time, which is more valuable than my 
computer's time.

* It clutters up the code with statements which add little 
(useful) information.
Oct 27 2015
next sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 10/27/2015 10:29 PM, tsbockman wrote:

 On Tuesday, 27 October 2015 at 21:14:26 UTC, Timon Gehr wrote:

 On 10/27/2015 09:18 PM, tsbockman wrote:

 I don't think any dead code is being generated,


 This seems to be a misunderstanding. I mean generated using mixins or
 template instantiation. Sure, it will usually be removed, but why
 generate and semantically analyze it in the first place.


 Forcing me to ...


This post is attacking a straw man. I'm not suggesting any of this.
Oct 27 2015
parent tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 27 October 2015 at 21:55:28 UTC, Timon Gehr wrote:

 On 10/27/2015 10:29 PM, tsbockman wrote:

 On Tuesday, 27 October 2015 at 21:14:26 UTC, Timon Gehr wrote:

 On 10/27/2015 09:18 PM, tsbockman wrote:

 I don't think any dead code is being generated,


 This seems to be a misunderstanding. I mean generated using 
 mixins or
 template instantiation. Sure, it will usually be removed, but 
 why
 generate and semantically analyze it in the first place.


 Forcing me to ...


 This post is attacking a straw man. I'm not suggesting any of 
 this.


Well then what *are* you suggesting? Can you describe an easy way 
for the user to avoid or suppress this warning without 
introducing redundant logic into their code?
Oct 27 2015
prev sibling parent reply Daniel Murphy <yebbliesnospam gmail.com> writes:
On 28/10/2015 8:29 AM, tsbockman wrote:

 On Tuesday, 27 October 2015 at 21:14:26 UTC, Timon Gehr wrote:

 On 10/27/2015 09:18 PM, tsbockman wrote:

 I don't think any dead code is being generated,


 This seems to be a misunderstanding. I mean generated using mixins or
 template instantiation. Sure, it will usually be removed, but why
 generate and semantically analyze it in the first place.


 Forcing me to add `static if`s with redundant and potentially complex
 predicates just to make my code do the exact same thing it would have
 done anyway is a violation of "Don't Repeat Yourself", with all of the
 usual consequences:

 * The more places the same logic is expressed, the more chances I have
 to get it wrong and cause a bug.

 * Even if I get it right the first time, a redundant predicate could get
 out of sync with the rest of the code later, causing a bug.

 * It's a waste of my time, which is more valuable than my computer's time.

 * It clutters up the code with statements which add little (useful)
 information.


I personally like the style of that code, and agree that it allows less 
repetition.  But it does this at the cost of intentionally introducing 
dead code in some instantiations.  If you enable the compiler warnings 
about dead code, they have to trigger here because it doesn't know if 
you introduced dead code intentionally or not.  As is often the case 
with warnings, if you want your code to compile with them you sometimes 
need to avoid otherwise completely valid constructs.

Here's a similar example:

bool func(T, T val)()
{
     if (val < 0)
         return true;
     return false;
}

func!(uint, 7);
func!(int, 7);

When instantiated with uint, the first return is unreachable because 
unsigned numbers cannot be negative.  When val == 7, it's also 
unreachable because 7 is not less than 0.  Which instantiations should 
give the warning?


 Another perspective, though, which I picked up from someone (Andrei
 Alexandrescu, I think?) in the D community, is to consider template
 parameters simply as additional function arguments, which happen to
 be evaluated at compile time. In many cases, the timing of their
 evaluation is just an implementation detail - a performance
 optimization (or de-optimization, as the case may be).


This is a great way to learn how to use templates, but there are limits 
to how well this simplification corresponds to reality and this is one 
of them.  Parameters inhibit optimizations and analysis in ways that 
compile-time constants do not.
Oct 27 2015
parent reply tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 28 October 2015 at 03:38:37 UTC, Daniel Murphy 
wrote:

 I personally like the style of that code, and agree that it 
 allows less repetition.  But it does this at the cost of 
 intentionally introducing dead code in some instantiations.  If 
 you enable the compiler warnings about dead code, they have to 
 trigger here because it doesn't know if you introduced dead 
 code intentionally or not.  As is often the case with warnings, 
 if you want your code to compile with them you sometimes need 
 to avoid otherwise completely valid constructs.

 Here's a similar example:

 bool func(T, T val)()
 {
     if (val < 0)
         return true;
     return false;
 }

 func!(uint, 7);
 func!(int, 7);

 When instantiated with uint, the first return is unreachable 
 because unsigned numbers cannot be negative.  When val == 7, 
 it's also unreachable because 7 is not less than 0.  Which 
 instantiations should give the warning?


I would say none, since *the template* contains no unreachable 
code, and the compiler can easily trim unreachable code from any 
*instantiation* which needs it, without bothering me about it.

I would only be interested in a warning if the compiler wasn't 
able to trim the dead code, but as far as I can tell the only 
case in which the compiler doesn't trim it, is the case where it 
doesn't realize it's unreachable - in which case it can't warn 
me, either.


 Another perspective, though, which I picked up from someone


 (Andrei

 Alexandrescu, I think?) in the D community, is to consider


 template

 parameters simply as additional function arguments, which


 happen to

 be evaluated at compile time. In many cases, the timing of


 their

 evaluation is just an implementation detail - a performance
 optimization (or de-optimization, as the case may be).


 This is a great way to learn how to use templates, but there 
 are limits to how well this simplification corresponds to 
 reality and this is one of them.  Parameters inhibit 
 optimizations and analysis in ways that compile-time constants 
 do not.


It's not intended as a simplification for people who can't handle 
the true complexity of templates - the difference is 
philosophical. It's a recognition of the fundamental unity of 
run-time and compile-time computation, the same idea which 
motivates CTFE.

If most people actually *want* these warnings, then great - 
there's no bug.
But, if most find the warnings conflict with how they want to use 
templates, as I do - why not just change it?

The "reality" of D templates is whatever the D community chooses 
to make it, subject to technical feasibility.
Oct 27 2015
parent reply Daniel Murphy <yebbliesnospam gmail.com> writes:
On 28/10/2015 4:29 PM, tsbockman wrote:

 I would say none, since *the template* contains no unreachable code, and
 the compiler can easily trim unreachable code from any *instantiation*
 which needs it, without bothering me about it.


If it's unreachable or not depends on what the template is instantiated 
with, there is no clear concept of unreachable code without knowing the 
template parameters.

bool func(T)(T value) if (isUnsigned!T)
{
     if (value < 0)
         return true;
     return false;
}

Here the first return is definitely dead code for any instantiation, but 
to know this the compiler would have to reverse-engineer properties from 
the template constraints, which is not generally possible.


 I would only be interested in a warning if the compiler wasn't able to
 trim the dead code, but as far as I can tell the only case in which the
 compiler doesn't trim it, is the case where it doesn't realize it's
 unreachable - in which case it can't warn me, either.


Well of course...


 It's not intended as a simplification for people who can't handle the
 true complexity of templates - the difference is philosophical. It's a
 recognition of the fundamental unity of run-time and compile-time
 computation, the same idea which motivates CTFE.


IIRC it's intended to avoid scaring people off reading TDPL by avoiding 
the work 'template'.


 If most people actually *want* these warnings, then great - there's no bug.
 But, if most find the warnings conflict with how they want to use
 templates, as I do - why not just change it?


I don't want these warnings, so I don't generally build with warnings 
enabled.


 The "reality" of D templates is whatever the D community chooses to make
 it, subject to technical feasibility.


As one of the core compiler devs, I'm saying it sounds infeasible.  I 
don't think either of your suggested solutions are implementable. 
Templates just do not work that way.


 1. Defer "not reachable" warnings until compilation has been
 completed, and only issue the warning if *all* instantiations of the
 statement were unreachable.


The exact set of instantiations depends on the current module being 
compiled, so module A can still get an unreachable code warning even if 
in an instantiation from module B the code is reachable.


 2. For semantic analysis purposes, first instantiate each template
 using dummy parameters with the widest possible VRP ranges. Only
 statements found to be "not reachable" in this dummy run should
 actually generate warnings.


Will not work with compile-time introspection.

In some trivial cases code can be found to be unreachable without doing 
semantic analysis, and therefore can be done before template 
instantiation.  Being limited, I doubt this is of much value.
Oct 28 2015
next sibling parent tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 28 October 2015 at 10:02:01 UTC, Daniel Murphy 
wrote:

 If it's unreachable or not depends on what the template is 
 instantiated with, there is no clear concept of unreachable 
 code without knowing the template parameters.


If a statement in a template is reachable in at least one 
possible instantiation of that template, it is not "unreachable". 
What is unclear about this as a concept?

Just because implementing this definition perfectly in the 
compiler is difficult, doesn't mean the concept is unclear.


 bool func(T)(T value) if (isUnsigned!T)
 {
     if (value < 0)
         return true;
     return false;
 }

 Here the first return is definitely dead code for any 
 instantiation, but to know this the compiler would have to 
 reverse-engineer properties from the template constraints, 
 which is not generally possible.


Detection of all dead code will never be "generally possible", 
with or without templates involved - if it was you could 
trivially solve the halting problem.

If false negatives are unacceptable, then we should remove the 
warning entirely, as achieving that goal is mathematically 
impossible on a normal computer.


 [snip]

 As one of the core compiler devs, I'm saying it sounds 
 infeasible.  I don't think either of your suggested solutions 
 are implementable. Templates just do not work that way.


 1. Defer "not reachable" warnings until compilation has been
 completed, and only issue the warning if *all* instantiations


 of the

 statement were unreachable.


 The exact set of instantiations depends on the current module 
 being compiled, so module A can still get an unreachable code 
 warning even if in an instantiation from module B the code is 
 reachable.



significantly reduce the frequency of false positives. As I said 
above, perfection is not the goal, since it is not even 
theoretically possible due to the halting problem.


 2. For semantic analysis purposes, first instantiate each


 template

 using dummy parameters with the widest possible VRP ranges.


 Only

 statements found to be "not reachable" in this dummy run


 should

 actually generate warnings.


 Will not work with compile-time introspection.


I'm not sure precisely what you're referring to here. I know 

it is possible.


 In some trivial cases code can be found to be unreachable 
 without doing semantic analysis, and therefore can be done 
 before template instantiation.  Being limited, I doubt this is 
 of much value.


Perhaps the warning should just be removed then.
Oct 28 2015
prev sibling parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 10/28/15 6:02 AM, Daniel Murphy wrote:

 On 28/10/2015 4:29 PM, tsbockman wrote:

 I would say none, since *the template* contains no unreachable code, and
 the compiler can easily trim unreachable code from any *instantiation*
 which needs it, without bothering me about it.


 If it's unreachable or not depends on what the template is instantiated
 with, there is no clear concept of unreachable code without knowing the
 template parameters.


You're not thinking from the user's point of view. It's a statement. I 
wrote it because I want it to work in certain instantiations. The 
compiler is incorrectly complaining that it can't be reached, because I 
can reach it by calling it a different way (easy to prove).

The warning should either be eliminated, or fixed so it's accurate. This 
reminds me of people adding empty try/catch clauses to Java to shut the 
compiler up about the checked exceptions.


 bool func(T)(T value) if (isUnsigned!T)
 {
      if (value < 0)
          return true;
      return false;
 }

 Here the first return is definitely dead code for any instantiation, but
 to know this the compiler would have to reverse-engineer properties from
 the template constraints, which is not generally possible.


I'm OK with the compiler giving up and not warning here. This warning is 
an optimization, and a *warning*, not an error.


 If most people actually *want* these warnings, then great - there's no
 bug.
 But, if most find the warnings conflict with how they want to use
 templates, as I do - why not just change it?


 I don't want these warnings, so I don't generally build with warnings
 enabled.


As will others who do want warnings, but don't want to deal with the 
compiler making false accusations :)


 The "reality" of D templates is whatever the D community chooses to make
 it, subject to technical feasibility.


 As one of the core compiler devs, I'm saying it sounds infeasible.  I
 don't think either of your suggested solutions are implementable.
 Templates just do not work that way.


What about disabling unreachability detection if a static branch (either 
explicit or implied by const folding) depends on a template variable?

If we can't fix this, I think we're better off without the warning.

-Steve
Oct 29 2015
prev sibling next sibling parent reply deadalnix <deadalnix gmail.com> writes:
On Saturday, 24 October 2015 at 17:25:23 UTC, tsbockman wrote:

 While improving the DMD front-end's constant folding:
     https://github.com/D-Programming-Language/dmd/pull/5229
 I found out about DMD issue 14835:
     https://issues.dlang.org/show_bug.cgi?id=14835

 Briefly:
 ///////////////////////////////
 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
 }
 ///////////////////////////////

 This is, I think, a big problem.


It is. This is the optimizer leaking into semantic. As far as 
language definition is concerned, this is a runtime if and 
changing it to a compile time one is an optimization.

If this was a static if, then generating a error is reasonable, 
but with a regular if, it isn't.
Oct 27 2015
next sibling parent Timon Gehr <timon.gehr gmx.ch> writes:
On 10/27/2015 10:33 PM, deadalnix wrote:

 On Saturday, 24 October 2015 at 17:25:23 UTC, tsbockman wrote:

 While improving the DMD front-end's constant folding:
     https://github.com/D-Programming-Language/dmd/pull/5229
 I found out about DMD issue 14835:
     https://issues.dlang.org/show_bug.cgi?id=14835

 Briefly:
 ///////////////////////////////
 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
 }
 ///////////////////////////////

 This is, I think, a big problem.


 It is. This is the optimizer leaking into semantic. As far as language
 definition is concerned, this is a runtime if and changing it to a
 compile time one is an optimization.

 If this was a static if, then generating a error is reasonable, but with
 a regular if, it isn't.


if statements with constant conditions perhaps shouldn't be eliminated 
in the frontend, at least for reachability analysis within templated 
functions.
Oct 27 2015
prev sibling parent reply tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 27 October 2015 at 21:33:04 UTC, deadalnix wrote:

 If this was a static if, then generating a error is reasonable, 
 but with a regular if, it isn't.


As Daniel Murphy pointed out earlier, a surefire way to *prevent* 
the warning is to use `static if`; code hidden by a `static if` 
will never trigger it. I believe this is correct, and by design.

(But not all control flow statements have static equivalents, so 
this solution can only be applied to some code. Even if we had 
`static switch`, `static foreach`, `static goto`, etc., I doubt 
that forcing the user to segregate all compile-time logic from 
the run-time logic in that way is desirable.)

Whether the logic is explicitly `static` (compile time) or not, 
the warning should be issued if and only if the flagged code is 
unreachable with all possible input combinations - including both 
compile-time and run-time.
Oct 27 2015
parent reply Daniel Murphy <yebbliesnospam gmail.com> writes:
On 28/10/2015 4:02 PM, tsbockman wrote:

 (But not all control flow statements have static equivalents, so this
 solution can only be applied to some code. Even if we had `static
 switch`, `static foreach`, `static goto`, etc., I doubt that forcing the
 user to segregate all compile-time logic from the run-time logic in that
 way is desirable.)


Nobody is forcing anyone to do this.  Warnings are opt-in.


 Whether the logic is explicitly `static` (compile time) or not, the
 warning should be issued if and only if the flagged code is unreachable
 with all possible input combinations - including both compile-time and
 run-time.


In D's compilation model it is not possible to know all possible 
instantiations at compilation time.
Oct 28 2015
parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Wednesday, 28 October 2015 at 10:03:44 UTC, Daniel Murphy 
wrote:

 On 28/10/2015 4:02 PM, tsbockman wrote:

 (But not all control flow statements have static equivalents, 
 so this
 solution can only be applied to some code. Even if we had 
 `static
 switch`, `static foreach`, `static goto`, etc., I doubt that 
 forcing the
 user to segregate all compile-time logic from the run-time 
 logic in that
 way is desirable.)


 Nobody is forcing anyone to do this.  Warnings are opt-in.


That's true, but it's generally good practice to compile with 
warnings enabled, and it's bad practice to compile with warnings 
and not fix them. So, ultimately, a warning isn't all that 
different from an error. If push comes to shove, then you can 
compile -wi instead of -w and leave the warning in, or you can 
choose to not compile with warnings at all, so it's not quite the 
same as an error, but it's effectively the same if you're 
following what's generally considered good programming practices 
(which is part of why I really wish that Walter had never given 
in and added warnings to the compiler).

- Jonathan M Davis
Oct 28 2015
prev sibling parent tsbockman <thomas.bockman gmail.com> writes:
On Saturday, 24 October 2015 at 17:25:23 UTC, tsbockman wrote:

 While improving the DMD front-end's constant folding:
     https://github.com/D-Programming-Language/dmd/pull/5229
 I found out about DMD issue 14835:
     https://issues.dlang.org/show_bug.cgi?id=14835


Walter has proposed a simple fix for this issue. It seems like a 
good idea to me:
     https://github.com/dlang/dmd/pull/12311
But, I thought it would be good to get more eyes on this since we 
all found it to be such a difficult problem in the past.

What does everyone else think?
Mar 28 2021