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.

 From #1 and #2, #2 is the "correct" choice - if implemented 
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.

#1 would still allow some false positives, but they should be 
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.

The main disadvantage that I see to approach #1 is that messages 
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.
My solution #2 is the same as the one you proposed - the dummy 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 } My solution #2 would add this before the reachIf!true pass: // 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. The reason solution #2 could end up being a ton of work, is 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 } 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.) In order to apply solution #2 to this code, we need a dummy 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.
My solution #2 is the same as the one you proposed - the dummy 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. 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.
 The reason solution #2 could end up being a ton of work, is 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. -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:
 My solution #2 is the same as the one you proposed - the dummy 
 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. Regardless, you are correct solution #1 will substantially reduce 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.
 The reason solution #2 could end up being a ton of work, is 
 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:
 My solution #2 is the same as the one you proposed - the dummy 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. Regardless, you are correct solution #1 will substantially reduce the opportunities for constant 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. Do you have an opinion regarding my solution #1 - defer "not 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.
I know that solution #1 is far from perfect - it would, however, 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 solution #2 would be a royal pain to implement, though, even if 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 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