Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler does not detect exhaustive bool switch #24865

Closed
gafter opened this issue Feb 15, 2018 · 10 comments
Closed

Compiler does not detect exhaustive bool switch #24865

gafter opened this issue Feb 15, 2018 · 10 comments

Comments

@gafter
Copy link
Member

gafter commented Feb 15, 2018

Reported by @HaloFour in dotnet/csharplang#1054 (comment)

The compiler does not determine b to be definitely assigned unless you also have default case which assigns b:

using System;

public class C {
    public void M(bool e) {
        bool b;
        switch (e) {
            case true:
                b = true;
                break;
            case false:
                b = true;
                break;
        }
        Console.WriteLine(b); // error CS0165: Use of unassigned local variable 'b'
    }
}

That's clearly exhaustive.

@gafter gafter added this to the 16.0 milestone Feb 15, 2018
@gafter gafter self-assigned this Feb 15, 2018
@HaloFour
Copy link

The situation is the same with enums:

using System;

public enum E { Foo, Bar }

public class C {
    public void M(E e) {
        bool b;
        switch (e) {
            case E.Foo:
                b = true;
                break;
            case E.Bar:
                b = true;
                break;
        }
        Console.WriteLine(b);  // error CS0165: Use of unassigned local variable 'b'
    }
}

Yes, technically one could argue that a match against an enum can't be exhaustive given that the enums value technically could be some arbitrary integer that doesn't match to one of its members but I seem to recall a conversation on this repo somewhere where the compiler wouldn't consider those cases. I'll try to scan for such a conversation and link it here if I find it.

@HaloFour
Copy link

I might be thinking of this conversation: #11438

In which case it was explicitly decided that matching true and false of a bool would be considered exhaustive but not with enum. Still something that I think is worth exploring in the future.

@jnm2
Copy link
Contributor

jnm2 commented Feb 15, 2018

Just bringing this back up again to find out what the compiler will do. If the compiler considered (true, false) to be complete, theoretically I could access uninitialized memory depending on the runtime?

bool x = LolBool.Value;

switch (x)
{
    case true:
    case false:
        break;
    default:
        throw new Exception("lol");
}

public static class LolBool
{
    public static readonly bool Value = new LolBoolUnion(0b10000000).BoolValue;

    [StructLayout(LayoutKind.Explicit)]
    private struct LolBoolUnion
    {
        [FieldOffset(0)]
        private readonly byte byteValue;
        [FieldOffset(0)]
        public readonly bool BoolValue;

        public LolBoolUnion(byte byteValue) : this()
        {
            this.byteValue = byteValue;
        }
    }
}

Or would the implementation of the switch then change to be like if (non-zero check)?

bool x = LolBool.Value;

bool trueVariable = true;
if (x == trueVariable)
    Console.WriteLine("This does not happen because 0b10000000 != 0b00000001");

const bool trueConst = true;
if (x == trueConst)
    Console.WriteLine("But this does, because it's reduced to `if (x)` which is ultimately a non-zero check");

@tannergooding
Copy link
Member

This actually also applies to byte and sbyte switches that cover all cases, I imagine it also applies to the larger ones, but I don't expect that code would appear in the wild 😄

@jnm2
Copy link
Contributor

jnm2 commented Feb 15, 2018

@tannergooding Does it? It's possible to store more than 8 bits in a byte memory location?

I would expect this to show up in the wild in combination with interop.

@tannergooding
Copy link
Member

A byte is always 1-byte in memory. It is possible for it to be larger in register, and it is possible for additional padding bytes to surround it, but reading those padding bytes would be a bug since they could be arbitrary values.

@gafter
Copy link
Member Author

gafter commented Feb 15, 2018

The code generated for a pattern-match on a boolean constant like true or false will be like the code we produce for an if statement. We test for zero (or non-zero). We don't bother testing both of them; if one fails, the compiler "knows" the other will succeed. So sneaky bool values won't do anything "wrong".

We have previously decided that enumerating the values of types other than bool in a pattern switch won't ever be considered exhaustive. See #11438 (comment). We may reconsider this if we add range patterns, like

switch (myByte)
{
    case in 0 to 127: break;
    case in 128 to 255: break;
}

The reason for this bug (#24865) is that the compiler always considers a default case to be reachable (for compatibility with previous compilers). But we incorrectly treat the absence of a default case as a branch to the end of the switch even when the set of cases is exhaustive. We should not have done that.

@gafter
Copy link
Member Author

gafter commented Feb 16, 2018

To work around this (#24865) bug in the compiler, add when true to one of the branches of your switch. That forces the compiler to use the new pattern-matching machinery instead of the old semantic analysis for the switch. The old semantic analysis code has no concept of exhaustive and needs to be deleted.

@HaloFour
Copy link

HaloFour commented Mar 23, 2018

Would be nice to get exhaustive matching on bool? as well, as long as all three cases true, false and null are covered.

dotnet/csharplang#1410 (comment)

Update: Using when true also works here.

@gafter
Copy link
Member Author

gafter commented Mar 30, 2018

This is now fixed in the features/recursive-patterns branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants