'Expression is always true' incorrect

Given the following code:

Snippet
class MyControl : Control
{
    Control _foo;

    public void DoIt()
    {
        if (_foo != null)
        {
            CaptureMouse();
            Contract.Assert(_foo != null);
        }
    }

    protected override void OnGotMouseCapture(MouseEventArgs e)
    {
        base.OnGotMouseCapture(e);
        _foo = this;
    }
}


ReSharper (5.1.1708.23) will flag the Contract.Assert expression as "expression is always true". Yet that's not the case - CaptureMouse may result in OnGotMouseCapture being called, which will set the flag.

Strangely, if I switch to Debug.Assert the warning goes away.
6 comments
Comment actions Permalink

Hello Scott,

Will CaptureMouse -> OnGotMouseCapture set _foo to null?

Andrey Serebryansky
Support Engineer
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"

Given the following code:

Snippetclass MyControl : Control
{
Control _foo;
public void DoIt()
{
if (_foo != null)
{
CaptureMouse();
Contract.Assert(_foo != null);
}
}
protected override void OnGotMouseCapture(MouseEventArgs e)
{
base.OnGotMouseCapture(e);
_foo = this;
}
}
ReSharper (5.1.1708.23) will flag the Contract.Assert expression as
"expression is always true". Yet that's not the case - CaptureMouse
may result in OnGotMouseCapture being called, which will set the flag.

Strangely, if I switch to Debug.Assert the warning goes away.

---
Original message URL:
http://www.jetbrains.net/devnet/message/5265527#5265527



0
Comment actions Permalink

Sorry I had a bug in the code. Change the _foo = this to _foo = null. Same result.

The point I was making is that ReSharper has no way of knowing what might happen when CaptureMouse is called. Because it dives into native code at some point, which in turn may or may not assign mouse capture, ReSharper cannot know if OnGotMouseCapture will actually be called or not. So even though we've entered the first block testing _foo != null to be true, after CaptureMouse (or any other method that ReSharper cannot follow the call trace of) then testing for the same expression again isn't necessarily redundant.

0
Comment actions Permalink

Yes, you're right.

But anyway this is that kind of heuristics we won't like to break.
Consider the followinf code:

{code}
if (_foo != null && SomeOtherCoolChecks())
  _foo.DoSomething();
{code}

if we suppose that ANY external call could reset the "_foo" to null, then we'll get a lot of false positives in this case.
Our investigations shows that "stable" variables are much often the case than modified externally.

Anyway, external modification of the field is a side-effect, and thus bad practice

0
Comment actions Permalink

I see your point, though I am getting false positives now that are cluttering my code. What I'm proposing is that if you cannot know if there are side effects or not, then it doesn't help to flag something as "expression is always true". Given that it's only a suggestion from ReSharper which will make the code simpler and possibly faster, but not necessarily more correct, wouldn't it be safer to assume there are side effects and err on the side of caution? This suggestion is effectively telling engineers it's safe to remove the duplicate expression test, when it's possible it will not.

In a WPF app side effects are very common - setting a property which in turn calls a handler which then does something with an internal field.. I don't think it's a bad practice so much as the pitfalls of normal programming.

As a side note - do you know why it would show this behavior with Contract.Assert but not Debug.Assert? I wonder what the difference is. Trace.Assert does show it, which makes me think it has to do with the debug-only attribute affecting the tests.

0
Comment actions Permalink

Your point on "expression is always true/false" warning is reasonable. I'll think how we can avoid this case.....

As for special Debug.Assert behavour - this method is marked by "AssertionMethod" annotation, and our analysis engine suppress warnings for it's parameters. Definitely we should add such annotations for contract methods

0
Comment actions Permalink

I've created high-priority request for the next version of ReSharper:
http://youtrack.jetbrains.net/issue/RSRP-182707

0

Please sign in to leave a comment.