views:

261

answers:

5

I'm having an argument with the CodeContracts static analysis tool.

My code:

Screenshot

(ASCII version)

The tool tells me that instance.bar may be a null reference. I believe the opposite.

Who is right? How can I prove it wrong?

+2  A: 

CodeContracts is right. There is nothing stopping you from setting instance.bar = null prior to calling the BarLength() method.

Evgeny
But I'm not setting `instance.bar = null` anywhere and `instance.bar` is private, so it can't be null, right?
dtb
Yes, so I suppose you are right if that's the entire code. Its static analysis is probably not smart enough to figure out whether you ever set the value to null or not, so it assumes that you might. I don't know what lengths it goes to to check for that - if any.
Evgeny
How do I convince it that `instance.bar` is never `null`? Making `bar` read-only and adding `Contract.Ensures(bar != null);` to the constructor doesn't do the trick. `Contract.Assume(instance.bar != null);` in `BarLength()` works but looks ugly.
dtb
Adding a class invariant doesn't help as well.
dtb
Checking for null before accessing it would convince it (there's a small runtime cost to that, of course).
Noah Richards
A: 

I agree with you. instance and bar are both private, so CodeContracts should be able to know instance.bar is never set to null.

Matthew Flaschen
A: 

If you mark the field 'bar' as readonly, that should satisfy the static analyzer that the field will never be set to anything else after the ctor executes.

anelson
Marking `bar` as read-only has no effect. For some reason, the analyser doesn't seem to realise that I'm assigning `bar` to a non-null value in the constructor.
dtb
+2  A: 

Your code includes a private static initialized instance:

private static Foo instance = new Foo();

Are you assuming that this means the instance constructor will always have run before access to any static method, therefore ensuring bar has been initialized?

In the single threaded case, I think you're right.

The sequence of events would be:

  1. Call to Foo.BarLength()
  2. Static initialization of class Foo (if not already completed)
  3. Static initialization of private static member instance with instance of Foo
  4. Entry toFoo.BarLength()

However, static initialization of a class is only ever triggered once per App Domain - and IIRC there's no blocking to ensure it's completed before any other static methods are called.

So, you could have this scenario:

  1. Thread Alpha: Call to Foo.BarLength()
  2. Thread Alpha: Static initialization of class Foo (if not already completed) starts
  3. Context Switch
  4. Thread Beta: Call to Foo.BarLength()
  5. Thread Beta: No Call to static initialization of class Foo because that's already underway
  6. Thread Beta: Entry to Foo.BarLength()
  7. Thread Beta: Access to null static member instance

There's no way the Contracts analyser can know that you'd never run the code in a multithreaded way, so it has to err on the side of caution.

Bevan
Right, but as you say (step 7), `instance` would be null, not `instance.bar`. `instance.bar` is only null before it's assigned in the constructor of the Foo instance, but the instance is only stored in the field strictly after the constructor has been completed.
dtb
Anyway, it's obviously a limitation of the static analyser. Now I'm looking for the best way to convince it that I'm right. Littering my code with `Assume` statements or with checks that can never occur feels just dirty.
dtb
Yes, the CLR does ensure that the static constructor completes before anything else on that class is referenced, unless you have two classes with static constructors that reference each other: http://msdn.microsoft.com/en-us/library/aa645612.aspx
Evgeny
Thanks for the link - it shows that in the single threaded case you have to set up a circular reference; however, it says nothing about the multi-threaded case that I documented above. Unfortunately, I've been unable to find my original reference - it was an odd edge case I read about some time ago.
Bevan
+1  A: 

Update: It seems the problem is that invariants are not supported for static fields.

2nd Update: The method outlined below is currently the recommended solution.

A possible workaround is to create a property for instance that Ensures the invariants that you want to hold. (Of course, you need to Assume them for the Ensure to be proven.) Once you have done this, you can just use the property and all the invariants should be proven correctly.

Here's your example using this method:

class Foo
{
    private static readonly Foo instance = new Foo();
    private readonly string bar;

    public static Foo Instance
    // workaround for not being able to put invariants on static fields
    {
        get
        {
            Contract.Ensures(Contract.Result<Foo>() != null);
            Contract.Ensures(Contract.Result<Foo>().bar != null);

            Contract.Assume(instance.bar != null);
            return instance;
        }
    }

    public Foo()
    {
        Contract.Ensures(bar != null);
        bar = "Hello world!";
    }

    public static int BarLength()
    {
        Contract.Assert(Instance != null);
        Contract.Assert(Instance.bar != null);
        // both of these are proven ok

        return Instance.bar.Length;
    }
}
Porges
Thanks for investigating this. Feel free to post it to the MSDN forum if you're interested in it. I've stopped using the static checker for now because a code review currently produces better results for me.
dtb
I've found a posting on the forum that states that invariants are not supported on static fields. I've updated my post with a workaround.
Porges
Nice find. But `bar` is an instance field, not static!? Anyway, I've moved the check-mark to this answer because it's the best explanation so far.
dtb
I think that the problem is that `instance` is static, so any fields accessed via it don't work either.I've actually had a reply from one of the team and it seems that the solution I came up with is the (currently) recommended solution for static fields.
Porges