Reposting my bug report https://issues.dlang.org/show_bug.cgi?id=20628 here to generate some discussion.

tl;dr: D is handling in-contracts in a way that causes bad outcomes both with and without debug mode, causing bizarre logic errors and even accepting invalid syntax.

---

Background:

An in-condition is part of D's implementation of Design by Contract. As such, their use on class methods acts as an extension of Liskov's Substitution Principle, which (generalizedly) states that class methods that override other methods may take more kinds of inputs, and return less kinds of outputs, than their parent method.

That is, a method that does not take 'null' values may be overridden by one that takes 'null' values, by the logic of "at least all parent's inputs are supported."

Reversedly, a method that may return 'null' values may be overridden by one that does not return 'null' values, by the inverse logic of "at most all parent's outputs are possible."

In other words, methods may, when inherited, *loosen* their input but *tighten* their output.

D lets us formalize this:

class A {
  Object method(Object obj)
  in (obj !is null)
  // no restriction on output
  ;
}

class B : A {
  override Object method(Object obj)
  // loosen restriction on input: null is allowed
  // tighten restriction on output: null is not allowed
  out (result; result !is null)
  ;
}

For inconditions, D implements this behavior as follows:

1. Check the superclass in-contract.
2. If the superclass contract throws an exception:
2.1. Then retry with the class's own in-contract.

This seems sensible. However, in practice it causes several problems.

Let's consider two cases: debug modes, where we want to look for and find logic errors, and non-debug mode, where we just want to run correctly.

Within debug mode, D should enforce that in contracts loosen the conditions. As such, it should always execute both superclass and subclass contract and Error if superclass-in passes but subclass-in does not.

In other words:

try {
  superMethod.runInCondition();
} catch (Exception) {
  // if this throws, all is well - the input is just not accepted.
  method.runInCondition();
  // if it didn't throw, all is well - the overridden in contract loosened the condition
  return;
}
// super method accepted this input - confirm that our logic holds
try {
  method.runInCondition();
} catch (Exception) {
  throw new LogicError("in contract was tightened in subclass - this is illegitimate");
}

But what to do outside debug mode? In my opinion, the reasonable thing to do is to only run the child contract.

Why? Well, consider the case that the parent contract is more tight than the child contract. In that case, the parent may throw an informative exception, but we don't care because we ignore it anyway; here, the parent's contract provides no information.

However, consider the case that the parent contract is more loose than the child contract. In that case, the child contract has *more* information than the parent. In the debug case, we want a LogicError in this scenario. However, the second-best thing is the exception that the child provides. Again, the parent contract has nothing to add, and we should just run the child contract.

Further benefits: this will also fix weirdnesses such as

interface I { void foo(); }
class C : I { void foo() in(this.is.never.compiled) { } }

or

interface I { void foo() in(true); }
class C : I { void foo() in(false) { } }

which would then be a compiletime error or runtime error, respectively.

Programming languages should do reasonable things. No reasonable programming language, when faced with this code,

void foo(int i)
in (i > 5)
{
  assert(i > 5, "this cannot happen");
}

should ever fail with "this cannot happen".

On Monday, 13 July 2020 at 13:55:56 UTC, FeepingCreature wrote:
>
> But what to do outside debug mode? In my opinion, the reasonable thing to do is to only run the child contract.
>


I don't want any contract checking outside of debug mode. You need to check your real inputs independent of the compilation mode and the in-contract is not the right place to do it.

On the other hand, having additional checks that are only 'on' in debug mode is useful.

On Monday, 13 July 2020 at 14:29:45 UTC, Panke wrote:
> I don't want any contract checking outside of debug mode.

contracts are actually independent compiler switches you can turn on and off separate from everything else. This is just talking about two layers of checks, don't read too much into the term "debug mode".

On Monday, 13 July 2020 at 14:33:42 UTC, Adam D. Ruppe wrote:
> On Monday, 13 July 2020 at 14:29:45 UTC, Panke wrote:
>> I don't want any contract checking outside of debug mode.
>
> contracts are actually independent compiler switches you can turn on and off separate from everything else. This is just talking about two layers of checks, don't read too much into the term "debug mode".

The idea here would be that the "expensive" two-step check would be limited to `-debug`, though I guess you could just always do it that way.

My ordering of preference is "check both in debug, check child without debug" > "check both (with LogicError/LiskovError) always" > "only check child always" > "current behavior".

On Monday, 13 July 2020 at 13:55:56 UTC, FeepingCreature wrote:
> [...]
>
> Let's consider two cases: debug modes, where we want to look for and find logic errors, and non-debug mode, where we just want to run correctly.
>
> Within debug mode, D should enforce that in contracts loosen the conditions. As such, it should always execute both superclass and subclass contract and Error if superclass-in passes but subclass-in does not.

I don't believe this is actually the case; it should not throw an Error if superclass-in passes and subclass-in does not. Consider the following case:

```
class A {
    /// Postconditions: `x` must equal `2`.
    void method(int x) in (x == 2) {}
}
class B : A {
    void method(int x) in (x == 3) {}
}

auto b = new B;
b.method(3); // valid
b.method(2); // valid, since `B` is-an `A` and `A.method` guarantees that inputs that pass x == 2 are valid.

A a = b;
a.method(2); // valid, even though this passes superclass-in but not subclass-in
a.method(3); // dangerous! `A.method` does not guarantee that x = 3 is allowed!
```

This is perfectly valid code, since the signature of `A.method` guarantees that it may take any inputs that are `x == 2`. The relationship is "super-in || sub-in" (an OR-relationship). [0]

> In other words:
>
> try {
>   superMethod.runInCondition();
> } catch (Exception) {
>   // if this throws, all is well - the input is just not accepted.
>   method.runInCondition();
>   // if it didn't throw, all is well - the overridden in contract loosened the condition
>   return;
> }
> // super method accepted this input - confirm that our logic holds
> try {
>   method.runInCondition();
> } catch (Exception) {
>   throw new LogicError("in contract was tightened in subclass - this is illegitimate");
> }
> But what to do outside debug mode? In my opinion, the reasonable thing to do is to only run the child contract.

This is not possible: input that passes the `in`-contract of the superclass should also be guaranteed to be valid input, if the child contract does not pass. After all, inputs that are valid for the superclass's implementation should also be valid for the subclass's implementation.

> Why? Well, consider the case that the parent contract is more tight than the child contract. In that case, the parent may throw an informative exception, but we don't care because we ignore it anyway; here, the parent's contract provides no information.
>
> However, consider the case that the parent contract is more loose than the child contract. In that case, the child contract has *more* information than the parent. In the debug case, we want a LogicError in this scenario. However, the second-best thing is the exception that the child provides. Again, the parent contract has nothing to add, and we should just run the child contract.
>
> Further benefits: this will also fix weirdnesses such as
>
> interface I { void foo(); }
> class C : I { void foo() in(this.is.never.compiled) { } }

This is valid, but should probably emit an error, since `I.foo` has no in-contracts and will always pass, so `C.foo`'s condition will never have to be checked (like you said).

> interface I { void foo() in(true); }
> class C : I { void foo() in(false) { } }
> which would then be a compiletime error or runtime error, respectively.

This is exactly the same case as above: `I.foo` will always pass and as a result, `C.foo` does not have to be checked.

> Programming languages should do reasonable things. No reasonable programming language, when faced with this code,
>
> void foo(int i)
> in (i > 5)
> {
>   assert(i > 5, "this cannot happen");
> }
>
> should ever fail with "this cannot happen".

In fact, when in-contracts are disabled, `foo(5)` may decide to fail with "this cannot happen", crash, corrupt memory or fail with "ERROR", since continuing execution while the in-contract has failed is UB anyway!

-burt

[0] https://dlang.org/spec/contracts.html#in_out_inheritance

On Tuesday, 14 July 2020 at 10:19:51 UTC, burt wrote:
> On Monday, 13 July 2020 at 13:55:56 UTC, FeepingCreature wrote:
>> [...]
>>
>> Let's consider two cases: debug modes, where we want to look for and find logic errors, and non-debug mode, where we just want to run correctly.
>>
>> Within debug mode, D should enforce that in contracts loosen the conditions. As such, it should always execute both superclass and subclass contract and Error if superclass-in passes but subclass-in does not.
>
> I don't believe this is actually the case; it should not throw an Error if superclass-in passes and subclass-in does not. Consider the following case:
>
> [snip]

I think the disagreement here is whether an incondition should mean "a condition for the method" or "a condition that is added to the implicit disjunction of the parent inconditions."

I think the way that D works currently is bad. I'm raising a design criticism here, not a bug - I know the current behavior is per spec. But I mean, if you see

```
class B : A {
    void method(int x) in (x == 3) {}
}
```

You don't expect x to be 2. In fact, the vastly more plausible way to arrive at this code is that the parent used to check `x == 3` but was changed to check `x == 2`. The disjunctive approach gives up the chance to discover this bug, for no benefit. Why no benefit? To be frank, because this kind of example essentially never comes up in practice.

Something like 95% of inconditions in our codebase at least, are some variant of "not null". How do you relax this incondition? By not writing anything, in both proposals. You certainly don't write `in (obj is null)`.

When do you want to add an additional disjunctive check that is totally unrelated to the parent's check? Even if you're say, expanding an enum, the expanded check will simply be "is the value in the expanded enum," not "is the value one of the two new enum values that I added."

I think "restate the parent's condition plus your new values" is already what people do anyways. Might as well take advantage.

On Tuesday, 14 July 2020 at 12:05:23 UTC, FeepingCreature wrote:
> Something like 95% of inconditions in our codebase at least, are some variant of "not null". How do you relax this incondition? By not writing anything, in both proposals. You certainly don't write `in (obj is null)`.
>

Addendum: Compare the type system.

If you have

```
class A
{
  void foo(A a) { }
}
class B
{
  override void foo(B b) { }
}
```

You wouldn't expect b to be typed "B or A". No, you'd expect to get an error. Parameter types are contravariant, not extensive; inconditions should follow the same logic.

On Tuesday, 14 July 2020 at 12:05:23 UTC, FeepingCreature wrote:
> [...]
>
> I think the disagreement here is whether an incondition should mean "a condition for the method" or "a condition that is added to the implicit disjunction of the parent inconditions."
>
> I think the way that D works currently is bad. I'm raising a design criticism here, not a bug - I know the current behavior is per spec. But I mean, if you see
>
> ```
> class B : A {
>     void method(int x) in (x == 3) {}
> }
> ```
>
> You don't expect x to be 2. In fact, the vastly more plausible way to arrive at this code is that the parent used to check `x == 3` but was changed to check `x == 2`. The disjunctive approach gives up the chance to discover this bug, for no benefit. Why no benefit? To be frank, because this kind of example essentially never comes up in practice.

So should the writer of a subclass HAVE to write out the preconditions of the parent? Because that could be impossible sometimes, if the precondition includes a call to a private function or something. Or perhaps some way to explicitly call the superclass's in contract.

> Something like 95% of inconditions in our codebase at least, are some variant of "not null". How do you relax this incondition? By not writing anything, in both proposals. You certainly don't write `in (obj is null)`.

You could also write `in (true)` in the current state of affairs.

> When do you want to add an additional disjunctive check that is totally unrelated to the parent's check? Even if you're say, expanding an enum, the expanded check will simply be "is the value in the expanded enum," not "is the value one of the two new enum values that I added."
>
> I think "restate the parent's condition plus your new values" is already what people do anyways. Might as well take advantage.

Is that sufficient for all cases? What if the superclass's precondition is `input1 == input2`, and the subclass's precondition also want to allow `input1.equalsCaseInsensitive(input2)` or something, how would you write that out?

-burt

On 7/13/20 9:55 AM, FeepingCreature wrote:
> Reposting my bug report https://issues.dlang.org/show_bug.cgi?id=20628 here to generate some discussion.
> 
> tl;dr: D is handling in-contracts in a way that causes bad outcomes both with and without debug mode, causing bizarre logic errors and even accepting invalid syntax.

I think you mean, invalid code, not syntax. Invalid syntax will not pass the parser.

But I somewhat disagree. Yes, you can write bad contracts, but that is not on the compiler, and can't really be checked by the compiler. The compiler enforces the rule by ignoring what the derived class does if the parent class passes. It doesn't enforce the logic of your contract fits the rule.

However, the biggest problem I have with contract inheritance is that no contract means "ignore base contract". Why is this a problem? Because people are lazy and don't do things they don't have to.

If you do nothing, what are the chances that
  a) you know about the contract, and actively decided in(true) is the correct contract for your subtype, and also knew that not providing a contract was the equivalent of in(true) so just didn't write it.
  b) you forgot/didn't notice it.

IMO, an unstated contract should not alter the parent contract. Unstated code means "I didn't care" or "leave it the same". It's not an active decision to alter the contract drastically to "accept everything".

Possibly this means changing a base contract could cause problems with existing code. I'm OK with that, it is you changing the requirements for your users (the authors of the derived code).

However, it is tedious that one has to repeat all the super's contract if your additive contract is unrelated.

One possibility is to consider a way to say "everything super said and ..."

maybe like:

void foo(int i) in(super.in) in(i > 5) {}

-Steve

On Tuesday, 14 July 2020 at 13:37:58 UTC, Steven Schveighoffer wrote:
> If you do nothing, what are the chances that
>   a) you know about the contract, and actively decided in(true) is the correct contract for your subtype, and also knew that not providing a contract was the equivalent of in(true) so just didn't write it.

It is actually `in(false)` to inherit the parent's contract. in(true) means you accept anything and everything. kinda nuts lol

http://dpldocs.info/this-week-in-d/Blog.Posted_2019_12_02.html