REALLY huge bug in simple logic

internal class MBSToolingLensLocalHelper
    {
        public MBSToolingLens GetToolingLensStateFromNames(string p_state, string s_state)
        {
            bool pStateSet = ConvertToBool(p_state);
            bool sStateSet = ConvertToBool(s_state);
            if (!pStateSet && !sStateSet)
            {
                return MBSToolingLens.MBSTL_P_Off_S_Off;
            }
            if (!pStateSet && sStateSet)
            {
                return MBSToolingLens.MBSTL_P_Off_S_On;
            }
            if (pStateSet && !sStateSet)
            {
                return MBSToolingLens.MBSTL_P_On_S_Off;
            }
            return MBSToolingLens.MBSTL_P_On_S_On;
        }

        private bool ConvertToBool(string state)
        {
            switch (state.ToLower())
            {
                case "on":
                    return true;
                case "off":
                    return false;
                default:
                    throw new ArgumentException(string.Format("Invalid/unsupported state {0}",state));
            }
        }
    }


Really simple code, really simple logic , but the resharper marks with dots variables marked here with red and claims that "Expression is always true" , whic is of course completely wrong, since first "if" only check the case when BOTH false , this does not cover the cases when one of them true.


JetBrains ReSharper 8.2.3 C# Edition
Build 8.2.3000.5176 on 2014-10-09T17:08:24

4 comments
Comment actions Permalink

Hmm, not a bug...

The clauses are

if (! a && !b) { return ...; }

if (! a && b) { return ...; }

if (a && ! b) { return ...; }


As the left clause of the && is the same, the first two if's can be merged to:

if (! a) {

     if (! b) { return ...; }

     if (b) { return ...; }
}

Obviously on "if (b)" b is always true.


Now for the third if, looking above at the merge, "if (! a)" never reaches it's end, so on the third "if" a is always true.

0
Comment actions Permalink

Of course you are correct if you fix/change this this way.
However this is NOT the way resharper suggest to fix it - if you accept it's suggestion it just removes one of the part of the condition resulting in wrong check.
This was all my point , sorry if it not explained clear enough.

0
Comment actions Permalink

It is no bug. The expressions are logical identical. It only looks little strange for a human reader.
This is because of the returns.

if (! a && !b) { return ...; }

=> a or b must be true
if (! a && b) { return ...; }

=> a is not false
if (a && ! b) { return ...; }

=> b is not false

When first clause (!pStateSet && !sStateSet) does not return then either pStateSet or sStateSet must be true after that line.

The second clause checks for (!pStateSet && sStateSet) and as I said one line before one of the two must be true here already.
If pStateSet is true then the clause fails and sStateSet is not checked.
If the pStateSet is false then we have to check sStateSet
But as said above it MUST be true when pStateSet is false, so no check needed anymore.
=> this is why resharper found it logically able to remove it.

The third clause (pStateSet && !sStateSet) is same as second clause vice versa.
So one of them must be true because of first clause.
If pStateSet where false in the second clause then it would have returned there.
So pStateSet must be true here.
=> this is why resharper found it logically able to remove it.

If you don't want to remove it then ignore it.
You may even add a Resharper comment to hide the warning.

0
Comment actions Permalink

Xm.. after looking on it a bit more it lookds like you are right , funny how I missed this for a first time. Sorry.

0

Please sign in to leave a comment.