Fix Issue 20353 - -checkaction=context does not work well with const …

[MoonlightSentinel]

…numbers

[dlang-bot]

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20353	normal	-checkaction=context does not work well with const numbers

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#10535"

[MoonlightSentinel]

The test fails for release builds but succeeds when the optimize call is disabled (see last commit).
The failure is caused by an ud2 in the function prolog of testFolded(): .....
Is this a backend bug or caused by an malformed AST (which should probably abort compilation)?

[MoonlightSentinel]

The delayed optimize const-folds x < y into false in release mode, resulting in an assert(false) which aborted the test, hence the version(assert).
I'm not sure wether this is comformant to the spec:

If the first AssignExpression consists entirely of compile time constants, and evaluates to false, it is a special case; it signifies that it is unreachable code. Compile Time Function Execution (CTFE) is not attempted.

CC @RazvanN7

EDIT: The spec refers to executing the assert(false), not the condition

[MoonlightSentinel]

This should be good now

(The failures are network timeouts)

[Geod24]

I'm not really convinced by this diff TBH. From what I see, this changes behavior so that constfolded expressions do not result in assert(false) anymore, which is kept even in -release builds.

And the benefit of this is just a slightly better output. Now it seems that every asserts that would be affected by this bug could be turned into a static assert, for which we have been generating user-friendly message for a while now:

void main() {
    const x = -1L;
    const y = -2L;
    static assert(x == y);
}

pr10535.d(4): Error: static assert:  -1L == -2L is false

So, why not re-use this mechanism instead and provide the string directly from the compiler ?

[MoonlightSentinel]

I'm not really convinced by this diff TBH. From what I see, this changes behavior so that constfolded expressions do not result in assert(false) anymore, which is kept even in -release builds.

The example in the test is constfolded and treated as assert(false) (hence its excluded if testassert.d is compiled with -release). Do you have a concrete example where the behavoir differs from master?

And the benefit of this is just a slightly better output. Now it seems that every asserts that would be affected by this bug could be turned into a static assert, for which we have been generating user-friendly message for a while now:

void main() {
    const x = -1L;
    const y = -2L;
    static assert(x == y);
}

pr10535.d(4): Error: static assert:  -1L == -2L is false

So, why not re-use this mechanism instead and provide the string directly from the compiler ?

Are you suggesting to implicitly promote assert(cond) to static assert(cond) if cond is non-trivial (0, false) but can be evaluated at compile time?

[Geod24]

hence its excluded if testassert.d is compiled with -release

assert(false) are never removed, even in -release mode.
They are used to mark unreachable code.

% echo "void main () { const x = 1; const y = 42; assert(x == y); }" > pr10535.d
% dmd -release -run pr10535.d
Error: program killed by signal 4

That being said, I just tested this PR and the assert is still there for -release builds, with or without -checkaction=context, so that's good.

Are you suggesting to implicitly promote assert(cond) to static assert(cond) if cond is non-trivial (0, false) but can be evaluated at compile time?

No, as that would break the usage of "assert(0) for unreachable code". What I was suggesting is to just generate the message upfront, in DMD, when the assert is const-folded, instead of relying on druntime.

[MoonlightSentinel]

hence its excluded if testassert.d is compiled with -release

assert(false) are never removed, even in -release mode.
They are used to mark unreachable code.

% echo "void main () { const x = 1; const y = 42; assert(x == y); }" > pr10535.d
% dmd -release -run pr10535.d
Error: program killed by signal 4

That being said, I just tested this PR and the assert is still there for -release builds, with or without -checkaction=context, so that's good.

I think my previous reply was worded poorly- the test case (not the assert) is excluded when compiling with -release.

No, as that would break the usage of "assert(0) for unreachable code". What I was suggesting is to just generate the message upfront, in DMD, when the assert is const-folded, instead of relying on druntime.

Thats could be an improvement but would require special-casing depending on the operands as __d_assert_fail can format more expression than toChars used by static assert. (unless we can assume that these expressions cannot be constfolded by the fronted)

I'm not entirely sure why your example even works but dmd seems to promote x, y to enums.

[Geod24]

Thats could be an improvement but would require special-casing depending on [...]

I was thinking of just providing a template overload that accepts a string.

Can you add the following test case to your PR:

struct Foo { int a; float b; }
static immutable Foo x, y;
void main ()
{
    assert(x == y);
}

For me this still print core.exception.AssertError@pr10535.d(5): assert(x == y) failed with your branch.
Interestingly, when the struct only have a single float member, it properly prints "nan != nan" (even without your PR).

[MoonlightSentinel]

Thats could be an improvement but would require special-casing depending on [...]

I was thinking of just providing a template overload that accepts a string. Do you h

There is something missing here?

Can you add the following test case to your PR:

Done

For me this still print core.exception.AssertError@pr10535.d(5): assert(x == y) failed with your branch.

unaSemantic rewrites that assert to assert(x.a == y.a && (x.b == y.b))

Interestingly, when the struct only have a single float member, it properly prints "nan != nan" (even without your PR).

unaSemantic rewrites that assert to assert(x.b == y.b)

Shouldn't these assert generate an implicit opEquals and be rewritten into assert(x.opEquals(y))?

NVM, the spec allows this rewrite

[MoonlightSentinel]

For me this still print core.exception.AssertError@pr10535.d(5): assert(x == y) failed with your branch.

unaSemantic rewrites that assert to assert(x.a == y.a && (x.b == y.b))

Essentially this is a problem of the current implementation of checkaction=context not my PR as it is unable to handle complex expressions.

The correct approach IMHO would be to apply checkaction=context before rewriting to format the original expression instead of the lowering. This would require less special casing and produce better messages (S(0, 1) != S(1, 1) instead of maybe 0 != 1 || 1 != 1).

Handling complex expressions would also suffice to handle such == lowerings at the cost of slightly worse messages.

I tried to implement the first approach but was not successfull yet

[RazvanN7]

I think that the fix is fine. I have hit this problem in the past also where the optimizer kicks in way too early, before actual error checking is done, and by the time you want to print the error the AST has already been irreversibly altered.

[MoonlightSentinel]

This issue will probably be fixed by #11005