fix issue 6226 - detect impossible cases when switching on integral t…

[ghost]

…ypes

While making testing this change I've discovered that when the switch is generated the CPU register that's tested is not cleaned up. This was not a problem until promotion was deactivated. So the promotion is still there but happens after the semantic on the switch body.

[dlang-bot]

Thanks for your pull request and interest in making D better, @StianGulpen! 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
✓	6226	enhancement	Switch with impossible cases

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#10603"

[thewilsonator]

std/math.d(7763): Error: cannot implicitly convert expression `-1` of type `int` to `immutable(ushort)`
std/math.d(7765): Error: cannot implicitly convert expression `-2` of type `int` to `immutable(ushort)`
std/math.d(7830): Error: template instance `std.math.pow!(immutable(float), immutable(ushort))` error instantiating
std/math.d(7763): Error: cannot implicitly convert expression `-1` of type `int` to `immutable(ubyte)`
std/math.d(7765): Error: cannot implicitly convert expression `-2` of type `int` to `immutable(ubyte)`
std/math.d(7831): Error: template instance `std.math.pow!(immutable(real), immutable(ubyte))` error instantiating

Hmm, look like there are a couple of phobos fixes needed first.

[ghost]

there are a couple of phobos fixes needed first.

And the buidlkite CI shows that user code would be affected too. This changes probably requires discussions. Not sure if people are ready for another disruptive deprecation.

[thewilsonator]

is it possible to deprecate the behaviour under switch while keeping it an error elsewhere?

[WalterBright]

The error messages produced indicate that these checks are backwards, and it is a mistake to move the integralPromotions from here.

[ghost]

In the first message I explained that promotion is kept only to prevent a problem with the code produced by the DMD backend. For example you can test this code when the promotion is completely removed:

string test(char a)
{
    version (none) asm
    {
        xor EAX, EAX;
    }
    switch (a)  // mov AL, byte ptr[...], but RAX is garbage, then cmp EAX result are randomly wrong
    {
        case 'a': return "a";
        case 'b': return "b";
        default: assert(false); // will take this path because of the garbages in EAX, despite of AL being correct.
    }
}

void main(string[] args)
{
    assert(test('a') == "a");
    assert(test('b') == "b");
} 

Without the BE issue the promotion would be removed. Why do you want to promote a byte to an int if the goal is to test the different values that a byte can have ?

Impossible cases are accepted because of the promotion that happened before the semantic check of the case and that is based on the promoted target type, i.e ss.condition.type.

[WalterBright]

You are indeed correct that:

"The case expressions must all evaluate to a constant value or array, or a runtime initialized const or immutable variable of integral type. They must be implicitly convertible to the type of the switch Expression."
https://dlang.org/spec/statement.html#switch-statement

I had that backwards.

[ghost]

yeah but nothing says in the specs that the type of the condition has to be promoted. Of course if it gets promoted before processing the cases that then it's obvious why the errors are not issued...

So I'm not sure if I understand. Do you agree on the fact that there's a bug ?

The bug to me is like if enum byte a = ubyte(-1); would be accepted just because we are in a switch, while everywhere else that fails.

[WalterBright]

The integral promotion of the switch value is not the source of the problem, and moving it causes failures in existing code. The correct fix would be to check if the case values could be produced by the integral promotion of the switch value.

[ghost]

In dmd.backend.cod3.doswitch(ref CodeBuilder cdb, block *b) in the statements following IfThen: It seems that the CMP instruction generated use always at least a 32 bit register as first operand. But this should be a 8, 16, 32 (or 64) bits register depending on the switch-condition type.

[WalterBright]

But this should be a 8, 16, 32 (or 64) bits register depending on the switch-condition type.

It's unnecessary to support anything but 32 or 64 bits. Just promote the switch value to an int and it'll work fine.

[ghost]

Just promote the switch value to an int and it'll work fine.

So this is what is done with the PR but this time not too early.

is it possible to deprecate the behaviour under switch while keeping it an error elsewhere?

The plan now that everybody understands why and how things are done is to keep track of the original type somewhere and use it while the CaseStatements are visited, using the implicitCastTo() result and issuing a deprecation according to the MATCH value. After the deprecation, the implicitConv will be moved like done in this PR to prevent the BE bug.

Of course as an option a special case could be added to spec, preventing the compiler change.

[WalterBright]

It needs to pass the tests, then I'll have another look.