Contracts, subtyping, JDK-8029804

[David Holmes]

Hi Pavel,

On 14/04/2015 9:57 PM, Pavel Rappo wrote:
> Hi,
>
> I've been looking into an old doc issue [1] on incompleteness in description of
> handling incorrect arguments in BufferedWriter.write(String, int, int).
> In a nutshell it's concerned that given the following facts:
>
>      1. Writer.write(String str, int off, int len)
>         promises to throw IndexOutOfBoundsException
>         if (off < 0) or (len < 0) or (off+len < 0) or (off+len > str.length())
>
>      2. class BufferedWriter extends Writer
>
>      3. BufferedWriter.write(String s, int off, int len)
>         states [2] that in contrast to its parent it won't do anything
>         if (len < 0)
>
> behaviour of BufferedWriter.write(String s, int off, int len) is unspecified for
> other cases mentioned in its parent.
> I thought it's very natural to think that you don't need to copy-paste contracts
> into children unless they break it or you want to elaborate some details.
> Basically because of subtype relationship, general principles of OOP and Liskov
> Substitution Principle.
> i.e. in this case a line in BufferedWriter.write's javadoc
>
>           * @exception IndexOutOfBoundsException {@inheritDoc}
>
> is implied.

@throws rather than @exception but yes, unfortunately historically 
people have been unaware of the need to explicitly state in the javadoc 
that a subclass inherits the unchecked-exceptions of its parent. This is 
usually an oversight, but of course a subtype is allowed to weaken 
preconditions.

> Unfortunately, BufferedWriter.write disobeys [3] the remainder of the contract
> of its parent. So we either have to make additional notes in the javadoc like
> with `len < 0` case or go and fix the bug instead.

Yep. In this case it seems odd to allow the parents preconditions to be 
weakened but I can't say whether someone thought there was a valid 
reason for it. But once the implementation is out of sync with the 
implied/assumed contract then you are looking at a spec change 
regardless - either to make the inherited spec clear (and fix the 
implementation) or to clearly document the change in behaviour.

> I wonder if it makes sense to fix those bugs altogether and remove the initial
> `len < 0` exceptional behavior. I fully understand that it formally breaks
> backwards compatibility because of the cases where such calls were lurking in
> code and ran unnoticed. But [2] shows that it wasn't done for some specific use
> case but rather was a mistake. Thus I don't think it would be a big stretch to
> assume that this thing affects no real world applications. Otherwise people
> would have noticed that something is not being written at all.

Code that writes nothing when passed a negative 'len' is probably 
exactly what the user expects. If an exception were originally thrown 
then to me it seems likely the user would have added the "if (len > 0)" 
check themselves and so still "write nothing" in this case. So no bug 
would have been observed. If you start throwing the exception now then 
existing code may be broken.

When the code has been out of sync with the spec for an extended period 
of time the safe approach is to adapt the spec to the code, not the 
other way around.

David

> P.S. I'm very interested in an outcome of this case mainly because I've noticed
> some more similar things in java.io package (e.g. [4]).
>
> Thanks.
>
> -------------------------------------------------------------------------------
> [1] https://bugs.openjdk.java.net/browse/JDK-8029804
> [2] https://bugs.openjdk.java.net/browse/JDK-4156573
> [3] To be honest, it silently disobeys parent's guarantees, thus some
>      bizarre things are possible:
>
>          bw.write("abc", 1, Integer.MAX_VALUE); // ok
>          bw.write("abc", -1, -1); // ok
>          bw.write("abc", -1, 1); // undeclared StringIndexOutOfBoundsException
>
> [4] https://bugs.openjdk.java.net/browse/JDK-8067801
>