Liskov Substitution principle - strengthening preconditions

Question

I am a bit confused as for what it really means. In the related questions (Is this a violation of the Liskov Substitution Principle?), it was said that the example clearly violates LSP.

But I wonder, if there is no new exception thrown, would it still be violation? Isn't it simply polymorphism then? I.e:

public class Task
{
     public Status Status { get; set; }

     public virtual void Close()
     {
         Status = Status.Closed;
     }
}

public class ProjectTask : Task
{
     public override void Close()
     {
          if (Status == Status.Started) 
          {
              base.Close(); 
          }
     }
}

Answer 1

It depends.

For validating the LSP, you need to know the precise contract of the Close function. If the code looks like this

public class Task
{
     // after a call to this method, the status must become "Closed"
     public virtual void Close()
     //...
}

then a derived class which ignores this comment violates the LSP. If, however, the code looks like this

public class Task
{
     // tries to close the the task 
     // (check Status afterwards to find out if it has worked)
     public virtual void Close()
     //...
}

then ProjectTask does not violate the LSP.

However, in case there is no comment, a function name like Close gives IMHO a the caller a pretty clear expectation of setting the status to "Closed", and if the function then does not work that way, it would be at least a violation of the "Principle of least astonishment".

Note also that some programming languages like Eiffel have in-built language supports for contracts, so one does not necessarily need to rely on comments. See this Wikipedia article for a list.

Answer 2

You can't decide whether some code violates the LSP by the code itself. You need to know the contract that each method is to fulfill.

In the example, there is no explicit contract given, so we have to guess what the intended contract of the Close() method might be.

Looking at the base class implementation of te Close() method, the only effect of that method is, that afterwards the Status is Status.Closed. My best guess of a contract for this method reads:

Do whatever is necessary to make the Status become Status.Closed.

But that's just a plausible guess. Nobody can be sure about that if it isn't explicitly written down.

Let's take my guess for granted.

Does the overridden Close() method also fulfill that contract? There are two possibilities that after running this method we have Status.Closed:

• We already had Status.Closed before calling the method.
• We had Status.Started. Then we call the base implementation, setting the field to Status.Closed.
• In all other cases we end up with a different status.

If Status only has the two possible values Closed and Started (e.g. a 2-value enum), everything is fine, there's no LSP violation, because we always get Status.Closed after the Close() method.

But probably there are more possible Status values, ending up in a Status not being Status.Closed, thus violating the contract.

---

The OP asked about the famous sentence "wherever I am using the base class, its derived class can be used".

So I'd like to elaborate on that.

I read it as "wherever I am using the base class within its contract, its derived class can be used, without violating that contract.

So it's not only about not producing compile errors or running without throwing errors, it's about doing what the contract demands.

And it only applies to situations where I ask the class to do something that's within its intended range of operations. So we need not care about abuse situations (e.g. where preconditions aren't met).

---

After re-reading your question, I think I should add a paragraph on polymorphism in that context.

Polymorphism means that for instances of different classes, the same method call results in different implementations being run. So polymorphism technically allows you to override our Close() method with one that instead e.g. opens a stream. Technically, that's possible, but it's a bad use of polymorphism. And one principle about good and bad uses of polymorphism is the LSP.

Answer 3

Liskov Substitution Principle is all about contracts. It consists of preconditions (conditions that must hold true so the corresponding behavior could run), postconditions (conditions that must hold true so that behavior could be considered to finish its job), invariants (conditions that must hold true before, during and after the corresponding method execution) and history constraint (in my opinion it's a subset of invariant, so you'd better check wikipedia). In a question you linked to an implied contract of the Task class looks like the following:

• Precondition: there is none
• Postcondition: Status is closed
• Invariant: can't see any

So if one the child classes doesn't close the task, it is considered a violation of LSP within some certain contract.

But if you explicitly postulate your contract to be like "Close the task only if it is started", then you're fine. You can do it in your code -- an example of that is this accepted answer. But very often you can't -- so you could use plain comments.

Basically, every time you think about LSP violation, you should already be familiar with the contract. There is no such thing as just "LSP violation", only "LSP violation within some contract".

Answer 4

Yes, still a violation (probably)

Some client of Task relies on "after Task::Close(), Status is now Closed", and then breaks when it encounters a ProjectTask. You might currently not have any such clients, but then the postcondition of Task::Close() would have to be "Status is in a valid but unspecified state", which is basically pointless.

The much more natural thing is for Task::Close() to have the postcondition "Status is Closed", which precludes the implementation in ProjectTask from being valid.

This is a major problem with void DoStuff() methods: all you have are some side-effects, so you have things relying on those side-effects. bool TryClose() has the meaning "Close() if you can, and tell me about it"

Answer 5

As Ralf and others have mentioned, you haven't actually implemented or enforced any contracts on your code, other than by the assumed "common sense" convention that Close() should leave the object in a closed state, and other than the comments that were added to the subclass.

In my opinion, the example you've provided (I know it's copied from a related post) has a design flaw to declare the Close() method as virtual on the base Task class - this is just inviting others to subclass Task and change the behaviour, even though you've provided a default implementation which observes the contract.

And worse, since Status isn't encapsulated at all, state is publicly mutable, so any contracts around Close are fairly meaningless as state can be randomly assigned externally in any event.

So if your class hierarchy doesn't require polymorphic behaviour of Close, I would simply remove the virtual keyword on Task.Close:

// Encapsulate status, to control state transition
public Status Status { get; private set; }

public void Close()
{
    Status = Status.Closed;
}

(and do the same for any other state transitions)

If however you DO require polymorphic behaviour (i.e. if subclasses need to provide custom implementations of Close), then I would convert your base Task class to an interface, and then enforce any pre and post conditions through Code Contracts, as follows:

[ContractClass(typeof(TaskContracts))]
public interface ITask
{
    Status Status { get; } // No externally accessible set

    void Close();
    // Other transition methods here.
}

With the corresponding contracts:

[ContractClassFor(typeof(ITask))]
public class TaskContracts : ITask
{
    public Status Status { get; }

    public void Close()
    {
        Contract.Requires(Status != Status.Closed, "Already Closed!");
        Contract.Ensures(Status == Status.Closed, "Must close Task on Completion!");
    }
}

The benefit of this approach is that the interface usage contract is clear (and enforceable!), and unlike the virtual Close() which could be bypassed, subclasses can provide any implementation they like, provided that the contract is met.

Answer 6

Yes, it still is a violation of the LSP.

In the base Task class, after Close() has been invoked the Status is Closed.
In the derived ProjectTask class, after invoking Close() the Status may or may not be Closed.

Thus the postcondition (Status is Closed) is no longer fulfilled in the ProjectTask class.
Or in other words: A client only knowing about Task may rely on the fact that Status is Closed after invoking Close(). If you give him a ProjectTask "disguised" as a Task (which you are permitted to do), and he invokes Close() the result is different (Status might not be Closed).