10

I'm using code contract (actually, learning using this).

I'm facing something weird to me... I override a method, defined in a 3rd party assembly. I want to add a Contract.Require statement like this:

public class MyClass: MyParentClass
{
    protected override void DoIt(MyParameter param)
    {
        Contract.Requires<ArgumentNullException>(param != null);

        this.ExecuteMyTask(param.Something);
    }

    protected void ExecuteMyTask(MyParameter param)
    {
        Contract.Requires<ArgumentNullException>(param != null);
        /* body of the method */
    }
}

However, I'm getting warnings like this:

Warning 1 CodeContracts: Method 'MyClass.DoIt(MyParameter)' overrides 'MyParentClass.DoIt(MyParameter))', thus cannot add Requires.

[edit] changed the code a bit to show alternatives issues [/edit]

If I remove the Contract.Requires in the DoIt method, I get another warning, telling me I have to provide unproven param != null I don't understand this warning. What is the cause, and can I solve it?

Cœur
  • 37,241
  • 25
  • 195
  • 267
Steve B
  • 36,818
  • 21
  • 101
  • 174

2 Answers2

14

You can't add extra requirements which your callers may not know about. It violates Liskov's Subtitution Principle. The point of polymorphism is that a caller should be able to treat a reference which actually refers to an instance of your derived class as if it refers to an instance of the base class.

Consider:

MyParentClass foo = GetParentClassFromSomewhere();
DoIt(null);

If that's statically determined to be valid, it's wrong for your derived class to hold up its hands and say "No! You're not meant to call DoIt with a null argument!" The aim of static analysis of contracts is that you can determine validity of calls, logic etc at compile-time... so no extra restrictions can be added at execution time, which is what happens here due to polymorphism.

A derived class can add guarantees about what it will do - what it will ensure - but it can't make any more demands from its callers for overridden methods.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • do you mean I have to use either the standard `if(param!=null) throw new ArgumentNullException(param)` or Contract.Requires but only for runtime checking ? In the later case, can I disable the warning once ? – Steve B Sep 06 '11 at 04:44
  • @Steve B: I don't know whether you can disable the warning, but fundamentally you're trying to do something you shouldn't - you're basically claiming that the client is doing something wrong when they can't know that they're not meant to pass in null. – Jon Skeet Sep 06 '11 at 05:19
  • I've updated a bit my question. If I remove the Contract.Requires from my DoIt method, I get another warning... In fact, I don't know how to ensure the param argument is not null. I understand the Liskov principle, but my method actually can't handle null parameters... that's why I use Contract.Requires. Maybe I have to use classic `if() {}` block ? – Steve B Sep 06 '11 at 07:18
  • I've just discovered that I can use `Contract.Assume(param != null)` to disable the warning. Using this method, I have to "rely" on the 3rd party base class internal quality... which I trust – Steve B Sep 06 '11 at 07:21
  • @Steve B: If your method can't handle null parameters but the base method can, it's not really adhering to the contract of the base method. Could you put the contract on the base method instead? – Jon Skeet Sep 06 '11 at 07:25
  • base class is defined in a 3rd party library which don't use Contracts... I can't change this, but I can assume the param argument won't be null (that's why I'm ok with the Contract.Assume). Maybe the question is more related to how to mix contract checked code with other code not using contracts... Anyways, thanks for your explanations, it helped me to understand a bit more how contracts works – Steve B Sep 06 '11 at 07:35
  • On a related note, technically, you should be able to weaken preconditions, for example remove one or more `Contract.Requires()` statements. This does not violate LSP and is the norm in Eiffel (see http://docs.eiffel.com/book/method/et-inheritance#Inheritance_and_contracts). However, according to section 3 of the code contracts manual (http://research.microsoft.com/en-us/projects/contracts/userdoc.pdf) this occurs rarely enough in practice for them not to support it. – akton Feb 16 '13 at 12:49
  • @Jon: Let's say your class, `Foo`, implements both `IComparable` and `IComparable`, how would you implement the former? I used to do it like that: `if (obj is Foo) return Compare((Foo)obj); throw new ArgumentException("obj is not of type Foo", "obj");` So, using Code Contracts, I think it should be something like `Contract.Requires(obj is Foo, "obj is not of type Foo.");` but it seems like this approach is wrong, too. – Şafak Gür Mar 25 '13 at 13:41
  • @Jon: But I'm making a demand in the implementation that the contract (`IComparable` interface) does not, which contradicts your answer. (And the compiler is vocal about it, too). Is it ok to just ignore the warning? – Şafak Gür Mar 25 '13 at 14:00
  • @ŞafakGür: IComparable.CompareTo documents that it will throw ArgumentException if "obj is not the same type as this instance." So if your class is sealed, that's okay. If your class *isn't* sealed, it's trickier. It's possible that IComparable has a contract applied to it in a different way, and that you could just apply the same contract. – Jon Skeet Mar 25 '13 at 14:03
  • What about abstract methods? – asavartsov Apr 16 '13 at 08:46
  • @JonSkeet I mean, what if I want to provide `Require`s for an abstract method - it's not allowed abstract methods to have body and you can't put `Require`s into overriding method. – asavartsov Apr 16 '13 at 10:41
  • @asavartsov, then you have to use the `ContractClass` attribute and create a seperate class to hold your contract logic. – toddmo Mar 11 '15 at 19:05
1

I'd like to note that you can do what Jon suggested (this answers adds upon his) but also have your contract without violating LSP.

You can do so by replacing the override keyword with new.

The base remains the base; all you did is introduce another functionality (as the keywords literally suggest).

It's not ideal for static-checking because the safety could be easily casted away (cast to base-class first, then call the method) but that's a must because otherwise it would violate LSP and you do not want to do that obviously. Better than nothing though, I'd say.

In an ideal world you could also override the method and call the new one, but C# wouldn't let you do so because the methods would have the same signatures (even tho it would make perfect sense; that's the trade-off).

MasterMastic
  • 20,711
  • 12
  • 68
  • 90
  • thanks for your feedback. I dislike the new modifier, because it can lead to confusion when dealing with large code base. And most of time, the new keyword is quite useless because working with the abstract type won't make the "new" method be visible. – Steve B Jan 22 '14 at 09:05
  • @SteveB Agreed. And for me personally I almost never use a concrete type or `var` as an identifier's type and imo that's very advisable so it's practically almost useless + confusing imo. Although if C# would have supported both new and override as mentioned, all problems are solved; but that's beyond the point. – MasterMastic Jan 22 '14 at 15:48