Optimize code generation for common cases of Requires<TException>

[sharwell]

Currently the rewriter produces verbose code for the following:

Contract.Requires<ArgumentNullException>(param != null, "param");

In release builds, I would expect this to be rewritten exactly as:

if (!(param != null))
  throw new ArgumentNullException("param");

[mike-barnett]

What does the rewriter produce today?

[sharwell]

It produces this:

__ContractsRuntime.Requires<ArgumentNullException>(array != null, "array", "array != null");

Which is efficient for the "fast path", but terrible for cases where the exception does get thrown (significant use of reflection APIs).

[mike-barnett]

Yes, I guess out belief was that exceptions should be, well, exceptional and so performance for the failing case was not important. Note that the form you propose would have to be contingent on the user wanting exactly that behavior: if they want to provide custom methods for handling contract failures or fail-fast behavior then the current rewriting should be retained.

We would like to get all of the VS2015 issues resolved before we start making these kinds of changes, but if you would like to implement this, then that would be great!

[SergeyTeplyakov]

I totally agree with Mike, that current behavior is correct, because runtime behavior of the failure is highly configurable.

So the call to _ContractRuntime should be preserved any way, but implementation of that method could be enhacned.

Two additional points:

1. Bug case (please not, that contract violation is not a "regular" exceptional case it is a subset of exceptional case when exception represents a bug) should be super rare.
2. Reflection is used because any exception type that accepts message could be used in this case. it is possible to add some "specializations" for different exceptions, but we should consider pros and cons of this approach (benefits vs. potential issues).

[sharwell]

I should add, I see this as a special case for the behavior when the rewriter is configured for ReleaseRequires contract checking only. I use this mode for shipping assemblies, and do not expect (or want) a contract failed event to occur. I'm also not expecting consuming code to be using Code Contracts (relevant for the bug case). Contract.Requires<TException> is essentially used at the public interface boundaries and for release it would be ideal for it to behave the same way as any other "normal" .NET module behaves for argument validation.

[mike-barnett]

Yep, that all sounds reasonable. I think it would be a great thing --- I hope my comments didn't sound negative about it. I just want to make sure that we don't preclude any of the existing behavior.

[SergeyTeplyakov]

@sharwell Just in case. I'm using following trick in this case: I'm using if-throw + Contract.EndContractBlock. In this case I would be able to turn contracts altogether and still get the desired behavior.

But we can do proposed trick.

[sharwell]

@mike-barnett I did interpret your comment according to that.

@SergeyTeplyakov I considered that approach, but it comes with its own downsides. In particular, with rewriting I can associate the contracts with a public interface and they will be automatically inserted in my internal implementation.

[SergeyTeplyakov]

@sharwell I don't think that you have downside that you mentioned. if-throw (i.e. legacy) requires are treated the same way as the regular preconditions. So you would still have them copied into internal implementation even with suggested approach.

[mike-barnett]

@sharwell Umm, just to make sure: you mean you interpreted my comments as sounding too negative? If so, I am very sorry. Your contributions to the project are fantastic!

[sharwell]

@mike-barnett No, the opposite. I understood them the way you meant from the start. :smile:

[mike-barnett]

Phew! Electronic communication is tough...

[risogolo]

As far as I remember the Generic version of the method Requires used to contained a bug. Generic method Contract.Requires is missing [Conditional("CONTRACTS_FULL")] attribute. Not sure if this was fixed.

[sharwell]

@risogolo That's not a bug. Calls to Contract.Requires<TException> must always be compiled.

[risogolo]

Not sure If I quite understand, I had an issue when I was using that Generic version of Contract.Requires, it was not never rewritten when I wanted to compile with no CC used. The code was still compiled to the body of my method or constructor. So I had to start using not generic implementation.

[federicoce-moravia]

@risogolo that's by design. The generic version is provided as a terse "if (!precondition) throw" useful at surface-level methods which are supposed to throw specific exceptions when the preconditions are not met. It's not meant to be elided when CC is off, it allows you to specify the "fail path" of your interfaces while at the same time feeding the checker information about expected input.

[risogolo]

OK then, but I had an issues with this design for example on build server, I'm not able to install CC on the build sever. Or I might not to be able on install CC to the production, then I have an issue. Do you suggest something?

[federicoce-moravia]

The production server doesn't need to know anything about CC, provided the Build server somehow managed to rewrite the assemblies before deployment. We actually have CC installed in our build agents, but I believe some work was done in the repo to make CC a NuGet package. I haven't tried it myself, but the package is available for download in the releases section. You could upload that to a private NuGet server your CI system is aware of.

[risogolo]

Once there will be an official nuget package then it will cure my issue, even a generic version will still not be rewritten, I was not aware that's by design, but since I know I can live with that ;), thx

[SergeyTeplyakov]

@risogolo ETA for publishing new nuget package - this week.

[risogolo]

Hello, by word "new" you probably meant first version of nuget package, I was not aware there is an official nuget package, even though I have found a couple of unofficial

[JohanLarsson]

Out of curiosity why: if (!(param != null)) and not: if (param == null) Also what does the stacktrace look like when throwing the way it does?

[federicoce-moravia]

Negating the contract to raise the failure is easier and less prone to bugs than transforming the expression, which can be arbitrarily complex (e.g. param == null || (param.Count >= 1 && param.Count < 8)). Granted, there are clear rules for doing so but very little gain from it.

[JohanLarsson]

Ah, that makes perfect sense. What about the stacktrace? Does it throw deep inside the cc code?

[federicoce-moravia]

For discussion's sake, here's ILSpy's output from __ContractsRuntime:

namespace System.Diagnostics.Contracts
{
    [CompilerGenerated]
    internal static class __ContractsRuntime
    {
        [DebuggerNonUserCode, ReliabilityContract(Consistency.WillNotCorruptState, Cer.MayFail)]
        internal static void Requires<TException>(bool condition, string msg, string conditionTxt) where TException : Exception
        {
            if (condition)
            {
                return;
            }
            string text = ContractHelper.RaiseContractFailedEvent(ContractFailureKind.Precondition, msg, conditionTxt, null);
            Exception ex = null;
            ConstructorInfo constructor = typeof(TException).GetConstructor(new Type[]
            {
                typeof(string),
                typeof(string)
            });
            if (constructor != null)
            {
                if (constructor.GetParameters()[0].Name == "paramName")
                {
                    ex = (constructor.Invoke(new object[]
                    {
                        msg,
                        text
                    }) as Exception);
                }
                else
                {
                    ex = (constructor.Invoke(new object[]
                    {
                        text,
                        msg
                    }) as Exception);
                }
            }
            else
            {
                constructor = typeof(TException).GetConstructor(new Type[]
                {
                    typeof(string)
                });
                if (constructor != null)
                {
                    ex = (constructor.Invoke(new object[]
                    {
                        text
                    }) as Exception);
                }
            }
            if (ex == null)
            {
                throw new ArgumentException(text, msg);
            }
            throw ex;
        }
    }
}

What about generating as many methods as distinct exception types required by the assembly, instead of a single generic method? (one can still be generated to unify the failure raising code). This would nuke the reflection perf losses, and still allow event raising (which could be further configured to be as no-op as possible).

[federicoce-moravia]

@JohanLarsson it's not too bad, two or three frames deep when dealing with the ContractFailedEvent, and only one when throwing the exception (which is gracefully hidden from view within VS due to the DebuggerNonUserCodeAttribute used in the method above).

Tangentially related to this, as far as I can tell there's no way to differentiate a Requires from a Requires<> from within the ContractFailed event handler (both use ContractFailureKind.Precondition). Perhaps there's merit in allowing the user to know the exception type the precondition was qualified with?

[jbcutting]

I agree with @sharwell that it would be ideal to have more the more optimized version of the rewritten IL for Requires<TException>. On top of the optimization gain, it would also shut FxCop up about CA1062. I know I'm not the only one wishing FxCop and CC played nicely together...

http://geekswithblogs.net/terje/archive/2010/10/14/making-static-code-analysis-and-code-contracts-work-together-or.aspx

[jimmymain]

it really would be awesome to be able to turn CA1062 back on. it's such a great check.

[Daniel-Svensson]

Since the type is known at compile time it should be quite straightforward to make all the reflection at rewrite time and end up with something similar to the code below while still preserving the same behaviour as today (almost, apart from slightly changed stack trace).

This would also change the stack trace so that the exceptions seems to come from the method using the Requires instead of from within the code contract runtime.

if (!(condition))
{
   string msg = ....;
   string conditionTxt = ...;
   string text = ContractHelper.RaiseContractFailedEvent(ContractFailureKind.Precondition, msg, conditionTxt, null);
   throw new TException(....); // text,msg order determined by same logic as in __ContractsRuntime.Requires<TException>
}

Using release requires I guess it could just skip the call to the RaiseContractFailedEvent, but then it was a while ago I read about exectly how it is supposed to work.

if (!(condition))
{
   throw new TException(....); // text,msg in order as 
}

For the original example the rewritten output then be rewritten as:

if (!(param != null))
{
   string msg = "param";
   string text = ContractHelper.RaiseContractFailedEvent(ContractFailureKind.Precondition, msg, "param != null", null);
   throw new ArgumentNullException(msg, text);
}