[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Basile-z  changed:

   What|Removed |Added

   See Also||https://issues.dlang.org/sh
   ||ow_bug.cgi?id=22646

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #19 from github-bugzi...@puremagic.com ---
Commits pushed to master at https://github.com/dlang/dmd

https://github.com/dlang/dmd/commit/30d5c4b3028d827c285a8bec682ed1acc69e1a57
fix Issue 18115 - [REG2.078-b1] case where && is not shortcut anymore in CTFE

https://github.com/dlang/dmd/commit/206ac0c195271cda5aa766aee25045874bb94629
Merge pull request #8259 from WalterBright/fix18115

fix Issue 18115 - [REG2.078-b1] case where && is not shortcut anymore…
merged-on-behalf-of: Walter Bright 

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Basile B.  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Basile B.  changed:

   What|Removed |Added

 CC||turkey...@gmail.com

--- Comment #18 from Basile B.  ---
*** Issue 18878 has been marked as a duplicate of this issue. ***

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #17 from Walter Bright  ---
I had an idea:

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

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Walter Bright  changed:

   What|Removed |Added

   Keywords|CTFE|rejects-valid

--- Comment #16 from Walter Bright  ---
BTW, here's where the constant folding is done:

 dtemplate.d(1586)
// Optimize argument to allow CT-known length matching
farg = farg.optimize(WANTvalue, (fparam.storageClass & (STC.ref_ | STC.out_))
!= 0);
//printf("farg = %s %s\n", farg.type.toChars(), farg.toChars());
--

https://github.com/dlang/dmd/blob/master/src/dmd/dtemplate.d#L1587

I'm not giving up yet, maybe I'll think of something :-) or maybe one of you
guys will.

In any case, it's not a CTFE issue. I'll remark it as rejects-valid for the
time being.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #15 from Walter Bright  ---
Tracing this through the compiler, the trouble is more subtle.

  int test()
  {
if (test.stringof.length > 6 &&
test.stringof[$-7..$] == "1234567") {}
return 0;
  }
  enum a = test();

1. The enum declaration says evaluate test() at compile time.
2. test() has not had semantic run on it yet, so the CTFE runs semantic on it
before attempting to execute it.
3. semantic sees the == with arrays on both sides. So it attempts to
instantiate the object.__equals() template with the left and right operands of
the ==.
4. As part of argument matching to the template, the operands get constant
folded.
5. The error is generated by the constant folding.

object.__equals() was added fairly recently, hence the regression.

I don't see any obvious way out of this.

However, there is a simple workaround, as constant folding does not do any flow
analysis:

  int test()
  {
auto i = test.stringof.length;
if (test.stringof.length > 6 &&
test.stringof[i-7..i] == "1234567") {}
return 0;
  }

  enum a = test();

Here, the value of `i` is not propagated to the constant folder. The optimizer
will do that, but later. CTFE will do it, too, but CTFE respects short
circuits.

Note also that the following version:

  int test()
  {
if (test.stringof.length > 6 &&
test.stringof[$-7..$]) {}
return 0;
  }

  enum a = test();

does not issue an error. That's because no template matching is attempted.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #14 from Rainer Schuetze  ---
The problem here is not the short-circuiting of semantic analysis, but the
const-folding now done on the second operand of "&&" that predicts a runtime
error if the code is actually executed.

Maybe it's ok to consider const-folding part of the semantic analysis (and the
code in the bug report never should have compiled). Otherwise we could try to
detect dead code and mute const-folding errors in it. But that's a can of worms
that's probably better left closed.

Or is it part of the language definition that you must not compile code
"arr[-1]"?

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Martin Nowak  changed:

   What|Removed |Added

 CC||c...@dawg.eu

--- Comment #13 from Martin Nowak  ---
(In reply to Basile B. from comment #12)
> Before closing, would it be possible to gag the error if the LHS evaluates
> to false ?

We ultimately dismissed short-circuit semantics outside of static if.

https://issues.dlang.org/show_bug.cgi?id=9073
https://github.com/dlang/dmd/pull/2559
https://github.com/dlang/dmd/pull/1325

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #12 from Basile B.  ---
Before closing, would it be possible to gag the error if the LHS evaluates to
false ?

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #11 from Rainer Schuetze  ---
Short circuiting is only performed for "static if", which also works for your
test case.

The string comparison used to be converted to a runtime call, so it could not
be evaluated further (but with soe special support in CTFE). It is now a
template that can be const folded and reports the error.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #10 from Basile B.  ---
(In reply to Rainer Schuetze from comment #9)
> > Sorry, did you miss my previous answer ? Use the code from the first 
> > message.
> > The regression is obvious.
> 
> I noticed and it is consistent with what I tried to say before: the reduced
> test case 

Damn, the reduced test case is wrong. Don't comment it

> fails because it checks "test()" for length 6, but then tries to
> access "test()"[$-7].
> 
> I think the problem actually existed before (as in my variation), but is now
> also visible in array comparisons a bit earlier.

Okay let's fix the test case:

```
int test()
{
if (test.stringof.length > 6 &&
test.stringof[$-7..$] == "1234567") {}
return 0;
}

enum a = test();
```

this compiles with 2.077.1, not with 2.078-beta.1.
the "&&" RHS is clearly here to prevent the bound error and the LHS should only
be evaluated once the RHS evaluates to "true". Unless the semantic for && is
different in CTFE mode, which i don't believe to be desirable, there's a
problem

The regression is that (a && b) behaves like ((a) & (b)) for some reason.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #9 from Rainer Schuetze  ---
> Sorry, did you miss my previous answer ? Use the code from the first message.
> The regression is obvious.

I noticed and it is consistent with what I tried to say before: the reduced
test case fails because it checks "test()" for length 6, but then tries to
access "test()"[$-7].

I think the problem actually existed before (as in my variation), but is now
also visible in array comparisons a bit earlier.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #8 from Basile B.  ---
(In reply to Rainer Schuetze from comment #7)
> > Rainer, not sure what you're saying. 
> > Is it invalid code in the test case, or a compiler problem?
> 
> I'm not 100% sure. At runtime the code will always produce a RangeError, but
> the check before it causes it to never be executed. Should it still be a
> compile error?
> 
> Allowing the condition could make writing generic code simpler, but might
> also trigger the "unreachable code" warning later. 
> 
> To change it, dmd.constfold must not produce errors (unconditionally), but
> keep the expressions that *might* cause an error at runtime or CTFE. Array
> indexing and divide by zero are probably affected, too.

Sorry, did you miss my previous answer ? Use the code from the first message.
The regression is obvious.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #7 from Rainer Schuetze  ---
> Rainer, not sure what you're saying. 
> Is it invalid code in the test case, or a compiler problem?

I'm not 100% sure. At runtime the code will always produce a RangeError, but
the check before it causes it to never be executed. Should it still be a
compile error?

Allowing the condition could make writing generic code simpler, but might also
trigger the "unreachable code" warning later. 

To change it, dmd.constfold must not produce errors (unconditionally), but keep
the expressions that *might* cause an error at runtime or CTFE. Array indexing
and divide by zero are probably affected, too.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #6 from Basile B.  ---
I've failed when trying to reduce the issue. Take the first one as test case.

```
module test;

class A23456{}

string foo()
{
string result;
mixin("alias m = " ~ __MODULE__ ~ ";");
foreach (member; __traits(allMembers, m))
{
if (member.length > 5 && member[$-6..$] == "A23456")
result ~= member ~ " ";
}
return result;
}

enum e = foo();
```

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Walter Bright  changed:

   What|Removed |Added

 CC||bugzi...@digitalmars.com

--- Comment #5 from Walter Bright  ---
Rainer, not sure what you're saying. Is it invalid code in the test case, or a
compiler problem?

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #4 from Rainer Schuetze  ---
if you change the test to

int test()
{
if (test.stringof.length < 7)
return 0;
return test.stringof[$-7..$] == "1234567";
}

enum a = test();


it fails for older versions, too. That's caused by the semantic analysis
already trying to optimize expressions. I guess it must not do this in case of
errors but defer it to the runtime error.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Rainer Schuetze  changed:

   What|Removed |Added

 CC||r.sagita...@gmx.de

--- Comment #3 from Rainer Schuetze  ---
The reduced test case fails for older versions, too, but that's because you are
substracting 7 there instead of 6.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Mike Franklin  changed:

   What|Removed |Added

 CC||slavo5...@yahoo.com

--- Comment #2 from Mike Franklin  ---
I bisected with digger and found the culprit to be
https://github.com/dlang/dmd/pull/7225

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

--- Comment #1 from Basile B.  ---
The '&&' RHS  shouldn't be evaluated even in this simplified test case:

```
int test()
{
if (test.stringof.length >= 6 &&
test.stringof[$-7..$] == "1234567") {}
return 0;
}

enum a = test();
```

When trying to build commit by commit i've found that
https://github.com/dlang/dmd/commit/9a57a965647ca7eef4c3149158b9256edb543f8a is
the culprit of the regression.

--

[Issue 18115] [REG2.078-b1] case where && is not shortcut anymore in CTFE

[d-bugmail--- via Digitalmars-d-bugs]

https://issues.dlang.org/show_bug.cgi?id=18115

Basile B.  changed:

   What|Removed |Added

   Keywords||CTFE

--