ccrewrite removes non-Contract method call, resulting in NullReferenceException

[yaakov-h]

Somewhat ironically, Code Contracts has introduced a null reference exception into my code by removing a function call.

The repro case is simple: When ccrewrite removes calls to Contract methods (e.g. in ReleaseRequires mode), it will also remove any semi-inlined methods.

Code:

using System;
using System.Diagnostics.Contracts;

namespace ContractsInlining
{
    class Program
    {
        static void Main(string[] args)
        {
            object thing = null;
            var didTheThing = TryDoThing(out thing);
            Contract.Assume(didTheThing);
            Console.WriteLine("The thing: {0}", thing.ToString());
        }

        static bool TryDoThing(out object value)
        {
            value = new object();
            return true;
        }
    }
}

IL of Main:

.method private hidebysig static 
    void Main (
        string[] args
    ) cil managed 
{
    // Method begins at RVA 0x2048
    // Code size 42 (0x2a)
    .maxstack 4
    .entrypoint
    .locals init (
        [0] object thing,
        [1] bool didTheThing
    )

    IL_0000: nop
    IL_0001: ldnull
    IL_0002: stloc.0
    IL_0003: ldloca.s thing
    IL_0005: call bool ContractsInlining.Program::TryDoThing(object&)
    IL_000a: stloc.1
    IL_000b: ldloc.1
    IL_000c: ldnull
    IL_000d: ldstr "didTheThing"
    IL_0012: call void System.Diagnostics.Contracts.__ContractsRuntime::Assume(bool, string, string)
    IL_0017: nop
    IL_0018: ldstr "The thing: {0}"
    IL_001d: ldloc.0
    IL_001e: callvirt instance string [mscorlib]System.Object::ToString()
    IL_0023: call void [mscorlib]System.Console::WriteLine(string, object)
    IL_0028: nop
    IL_0029: ret
} // end of method Program::Main

IL of Main with compiler optimizations:

.method private hidebysig static 
    void Main (
        string[] args
    ) cil managed 
{
    // Method begins at RVA 0x2048
    // Code size 37 (0x25)
    .maxstack 4
    .entrypoint
    .locals init (
        [0] object thing
    )

    IL_0000: ldnull
    IL_0001: stloc.0
    IL_0002: ldloca.s thing
    IL_0004: call bool ContractsInlining.Program::TryDoThing(object&)
    IL_0009: ldnull
    IL_000a: ldstr "out thing"
    IL_000f: call void System.Diagnostics.Contracts.__ContractsRuntime::Assume(bool, string, string)
    IL_0014: ldstr "The thing: {0}"
    IL_0019: ldloc.0
    IL_001a: callvirt instance string [mscorlib]System.Object::ToString()
    IL_001f: call void [mscorlib]System.Console::WriteLine(string, object)
    IL_0024: ret
} // end of method Program::Main

As you can see, instead of storing the result of TryDoThing and re-loading it, it leaves it on the expression stack. This is equivalent to Contract.Assume(TryDoThing(out thing), null, "didTheThing").

IL of Main with compiler optimizations, ReleaseRequires and Public Surface Contracts only:

.method private hidebysig static 
    void Main (
        string[] args
    ) cil managed 
{
    // Method begins at RVA 0x2048
    // Code size 19 (0x13)
    .maxstack 4
    .entrypoint
    .locals init (
        [0] object thing
    )

    IL_0000: ldnull
    IL_0001: stloc.0
    IL_0002: ldstr "The thing: {0}"
    IL_0007: ldloc.0
    IL_0008: callvirt instance string [mscorlib]System.Object::ToString()
    IL_000d: call void [mscorlib]System.Console::WriteLine(string, object)
    IL_0012: ret
} // end of method Program::Main

Both the call to Contract.Assume and the call to TryDoThing have been eliminated by ccrewrite. This results in a NullReferenceException at runtime.

This happens in both VS2013 and VS2015. IL above is from 2015.

[ZbynekZ]

I have the same problem. In my case, the rewriting works well in VS2013, but has improperly eliminated some non-contract code in VS2015. I can provide more details if needed, but at the moment I believe the original post provides a perfect repro. If I can ask, this should be handled as one of the highest priorities.

[SergeyTeplyakov]

@yaakov-h This is a very interesting bug.

Consider following code:

Contract.Assume(SomeMethod());

What ccrewrite should do in this case?

Currently, ccrewrite will remove Assume as well as SomeMethod() method call. This behavior is consistent with Debug.Assert behavior.

Now, switching back to the original example:

var result = SomeMethod();
Contract.Assume(result);

In Debug build there would be a local variable and ccrewrite can understand this and leave SomeMethod call. But turning optimization on this code would be transformed by C# compiler to the first example.

So from ccrewrite perspective there is no way to distinguish those cases at all...

So current proposal sounds like this to me: Let ccrewrite leave all the expressions evaluated as a first argument on evaluation stack.

I do agree that in some cases this could be very beneficial, but I do believe that it could be not what other people want from the ccrewrite.

Consider following code:

Contract.Assume(CheckSomeHeavyweightState());

This check would be removed right now and for some users this could be exactly what they need (and this is exactly what is required for few of my projects).

It means that proposed change would be a breaking change and not everyone will agree that new behavior is desirable.

P.S. I do believe we need to do something with this issue, but I'm not sure that simple approach (just not remove the call to Contract.Assert) is a best way to solve it.

[SergeyTeplyakov]

BTW, as far as I remeber, ccrewrite has very weird bug, that ccrewrite will never remove complex expressions.

Consider following:

Contract.Assume(Foo());

In this case, Foo method invocation would be removed for None or Pre/Post modes.

But in this case:

Contract.Assume(Foo() || Boo());

In this case call to Contract.Assume would be removed, but invocation expressions will be in the evaluation stack.

This definitely a bug that could be very hard to tackle. For instance, you can avoid NRE in you code simply by changing your code to: Contract.Assume(didTheThing || false);.

[SergeyTeplyakov]

@ZbynekZ Maybe you've faced #191 issue. Could you confirm, please, that you're facing current issue that I believe is by design (unfortunately).

[SergeyTeplyakov]

@yaakov-h ok, It seems that this behaviour needs to be changed to proposed one.

[ZbynekZ]

@SergeyTeplyakov I think my case is #242 and not #191. The only difference I can tell was that in my case, it has worked correctly up to VS2013, and appeared in VS2015 Release builds, but I think there might be some explanation for it that we just do not know at the moment. Besides this, everything else is practically identical, and it is VS2015 that concerns me now, and with that we are all seeing the same.

I have read the analysis and it makes sense. Unfortunately, there is probably no "good" solution based just on IL, if the source code that should in end behave differently compiles to the same IL.

I think that therefore we need to look at this form the practical point of view: If we simply allow the current behavior of the rewriter and claim it as "standard", lots of code that will appear correct will end up being rewritten to incorrect code, and it would be (at least in case of my large projects) very difficult if not impossible to scan all the code for places where this danger appears, and make sure to work around it somehow.

If, on the other hand, we go with the proposal "Let ccrewrite leave all the expressions evaluated as a first argument on evaluation stack.", all code will behave correctly, and when writing new code, the developer will not have to always think about whether some of his statements are not going to disappear. The only negative thing that can happen is that an argument to Assume or Assert will get evaluated when it "should not". That may be sub-optimal because a) it has some side effect, or b) is heavy to compute. The side effect thing I think should not be an issue at all, because that's not how contracts should be written, and in the Debug build that same thing would be called anyway, so it makes even less sense to claim that the same side effect would be undesirable in Release build. The heavy-weight computation of a contract condition might be a problem sometimes, but is relatively rare, and as such I think can be avoided with extra conditional directive - I think that this is something the developer can keep an eye on, much easier than trying to figure out whether a statement that is not in any Assert or Assume would be subject to removal.

[ZbynekZ]

@SergeyTeplyakov Maybe just to sum up my argument: I think it is far better to end up with a program that has some extra code that is just evaluating a contract condition (without actually enforcing it), rather than end up with a program that has pieces of code removed, where the actual scope of the removed piece depends on an implementation detail of a compiler, and as such it is generally unpredictable.

[yaakov-h]

@SergeyTeplyakov thanks for looking into this, I'll try that workaround.

@ZbynekZ this issue (#242) occurs in both 2013 and 2015. If your problem only occurs in 2015, this may not be your problem.

[SergeyTeplyakov]

@yaakov-h, @ZbynekZ I think that current behavior needs to be changed to proposed one.

The reason for that is simple: VS2015 is doing much more optimization that VS2013.

Consider following code:

var hashSet = new HashSet<int>();
bool b = hashSet.Add(42);
Contract.Assert(b);

In VS2013 even with Release mode local b would be in the resulting IL and the code would behave correctly even when asserts are disabled. But in VS2015 in Release mode the local would be removed and the hashSet would not have 42 in it!

[SergeyTeplyakov]

@yaakov-h My proposed workaround will not work, because C# compiler is smart enough to entirely remove || false.

ccrewrite would preserve complex expressions (I do believe this is a bug, but anyway) but you need more complex non-constant expression.

[yaakov-h]

@SergeyTeplyakov The code you posted regarding the hashset adds 42 to the HashSet in all cases that I could test - Debug, Release, Debug with optimizations, and Release without optimizations.

Debug.Assert(hashSet.Add(42)); however, only adds 42 to the HashSet in Debug builds.

This makes me think that Roslyn is smart enough to tell the difference between what is and isn't part of the Debug.Assert expression - assumedly, because it uses the code, not the generated IL.

Here's the IL output of Release with optimizations from VS2015 (Update 1 RC):

        IL_0000: newobj instance void class [System.Core]System.Collections.Generic.HashSet`1<int32>::.ctor()
        IL_0005: stloc.0
        IL_0006: ldloc.0
        IL_0007: ldc.i4.s 42
        IL_0009: callvirt instance bool class [System.Core]System.Collections.Generic.HashSet`1<int32>::Add(!0)
        IL_000e: pop

I assume there's no way to match this behaviour when IL is the input, as is the case with Code Contracts?

[SergeyTeplyakov]

@yaakov-h Sorry, I've copy-pasted wrong code. I meant Contract.Assert not Debug.Assert.

Debug.Assert is conditionally compiled, so the method invocation would be removed in compile time if no DEBUG symbol is defined.

[yaakov-h]

@SergeyTeplyakov That's what I thought, though Roslyn seems a bit more selective about what it removes and what it keeps.

So with Contracts interrogating the IL, is the only way an all-or-nothing approach (keep all expressions inside Contract.Assume/Assert/etc., or remove them all), or is there any avenue that could be used to determine whether a variable was optimized out?

I don't like the idea of leaving potentially expensive checks in, but I can't see any other way.

[SergeyTeplyakov]

@yaakov-h Unfortunately I don't have any ideas, how to distinguish between those two cases.

The only way to think about it: not to remove (or warn) if non-pure method was put into Contract.Assume and warn or just not to remove this call. In this case the behavior would be consistent. The only problem with this approach - there is no simple way to tell whether expression is Pure or not (there is no way to tell if some arbitrary method is Pure if no attributes were added to it).

I don't like the idea of leaving checks on the stack, but I can't see other options right now as well...

[yaakov-h]

@SergeyTeplyakov Good point on using [Pure]. I'd also suggest, if possible, to strip out expressions with no assignment or function calls. That way, Contract.Assume(foo != null) would also disappear, but Contract.Assume(Foo() != null) would still result in Foo being called, unless marked as [Pure].

[SergeyTeplyakov]

@yaakov-h I'm thinking about long-term and short-term fixes for this issue. I really hope to release new version very soon, and maybe for this version I'll just fix this stuff to leave all expressions on the evaluation stack and will create an work item to fix this properly... What do you think?

[yaakov-h]

@SergeyTeplyakov Sounds like a decent short-term fix, as long as there's still a long-term one in the works.

[ZbynekZ]

Agreed with the recent proposals. If the rewriter manages to remove stuff that it identifies as "pure" - be it for the presence of [Pure} attribute, or other simple cases (foo != null) - then even better so.

[mmcg]

We ran into this issue as well. A couple of options:

1. Provide overloads for Contract.Assert and friends which take a Func<bool> or Expression<Func<bool>>, and deprecate the old interfaces. That'd be optimiser-proof (and future-proof), and upgrading to the new API is as simple as globally replacing Contract.Assert( with Contract.Assert(()=>. As a special bonus, ccrewrite wouldn't even need to elide the calls.
2. Possible backward compatibility hack: read the IL debug info from the PDB and elide the code that constructs the argument if it appears after the Contract.Assert and before the start of the next statement; spew warnings wherever the PDB info is unavailable or ambiguous. Even if this works (which is debatable), it's probably pretty poor form and would probably break in the future (no, I don't like it either).

Most likely I'll be implementing option 1 until we have something better :)

[mistoll]

Also ran into this issue.
I like the pure approach.