Re: [Clirr-devel] adding methods to interfaces

[Simon K. <si...@ec...>]

On Fri, 2004-06-25 at 17:53, Lars K=FChne wrote:
> Simon Kitching wrote:
>=20
> >Hi,
> >
> >In MethodSetCheck, the reportMethodAdded method uses:
> >      final Severity severity =3D=20
> >	!newClass.isInterface() &&=20
> >	(newClass.isFinal() || !newMethod.isAbstract())
> >                ? Severity.INFO
> >                : Severity.ERROR;
> >
> >
> >This means that adding a method to an interface is reported as an ERRO=
R.
> >However the java spec section 13.5.3 says:
> >
> ><quote>
> >Adding a method to an interface does not break compatibility with
> >pre-existing binaries
> ></quote>
> >
> >I've tested this, and this seems to be correct: there will not be a
> >*linktime* error reported under any circumstances. However if I have
> >code which implements the old interface, and I then pass this object t=
o
> >code that tries to invoke a new method, an AbstractMethodError runtime
> >exception occurs.
> >
> >So I think that this should be a WARNING, not an error. What do you
> >think?
> >
> > =20
> >
>=20
> I think we could follow the same route that I'm currently investigating=
=20
> for source incompatibilies. I.e. the ApiDifference should not have only=
=20
> one severity field, but one field for each compatibility class=20
> (linktime, runtime, source).
>=20
> In the meantime I think ERROR is OK - "clirr" stands for "//Client=20
> binaries must link and *run* with new //releases...". To me linktime an=
d=20
> runtime errors are pretty much equivalent, either of them means I can't=
=20
> use the new version as a drop-in replacement because my application wil=
l=20
> bomb out with some exception sooner or later.

The issue is that binary incompatibilities are *always* failures; just
having two incompatible classes together in the classpath causes a
failure.

For "warning" level issues, the app *may* run or *may* cause a runtime
exception depending on exactly how the classes are used.

The issue really became obvious for me when I ran CLIRR against
commons-collections. Any addition of a method to an interface was
reported as an error. But code which simply *uses* collections classes
will work fine with the new release.=20

A failure will only occur for code which *subclasses* the interfaces
present in the old release then passes a subclass instance to some code
which attempts to use a method added in the new release. This is *not*
normal for users of commons-collections. So despite the fact that about
30 "ERROR" messages were generated by clirr, the commons-collections
release is perfectly "compatible" for its normal users.

>=20
> >-------------------------------
> >
> >I'm also somewhat puzzled by the remainder of the ternary expression
> >too.
> >
> >Adding an abstract method to a class is indeed a problem, because
> >subclasses can exist which don't have that method. But like the
> >interface issue, I think this is a WARNING (runtime problem) rather th=
an
> >an ERROR (linktime problem).
> >
> > =20
> >
>=20
> See above - I think differentiating between linktime and runtime errors=
=20
> is not very useful from the perspecive of a library user. Let's keep it=
=20
> at ERROR until we have the extended API available in the ApiDifference=20
> class.
>=20
> >And why is it ok to add an abstract method to a final class? Final
> >classes are never allowed to be abstract, so this shouldn't be possibl=
e
> >as far as I know..
> >
> > =20
> >
>=20
> Maybe it's because I've just recovered from the flu, but I don't=20
> understand your point here...
>=20
> There are theoretically four combinations of final and abstract. One of=
=20
> them (abstract method in a non-final class) creates a compatibility=20
> problem and the code reports that correctly.
>=20
> The remaining three cases do not create a compatibility problem. One of=
=20
> those remaining three cases (abstract method and final class) cannot be=
=20
> achieved with a correct java compiler - but that doesn't matter here,=20
> because it is still not a compatibility problem: there can't be any=20
> subclasses where the method is missing because the class is final.=20
> Reporting INFO in that case is OK, no?

As for the interface issue, I think adding an abstract method to a class
is a WARNING because it only causes problems for *some* users of that
class. Applications which only instantiate classes which are concrete
subclasses of the changed class will continue to run fine. This is
definitely *more likely* to cause problems than the "add method to
interface" issue discussed above, but there still will be applications
for which this change does *not* cause a failure of the application when
the new lib is used with the old app binary.


Re the "ternary expression: Ok, so the fact that code reports "INFO" for
(abstract method + final class) is wrong, but you don't care because
that situation can never occur? I agree with that, but would prefer to
use somewhat clearer code to express this; unless you object I will use
if-statements to make this a bit more obvious to people like me :-)


Regards,

Simon