Thread: [Clirr-devel] adding methods to interfaces
Status: Alpha
Brought to you by:
lkuehne
Menu
▾
▴
clirr-devel
|
From: Simon K. <si...@ec...> - 2004-06-23 06:34:57
|
Hi,
In MethodSetCheck, the reportMethodAdded method uses:
final Severity severity =
!newClass.isInterface() &&
(newClass.isFinal() || !newMethod.isAbstract())
? Severity.INFO
: Severity.ERROR;
This means that adding a method to an interface is reported as an ERROR.
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 to
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?
-------------------------------
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 than
an ERROR (linktime problem).
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 possible
as far as I know..
Regards,
Simon
|
|
From: <lak...@t-...> - 2004-06-25 05:44:29
|
Simon Kitching wrote: >Hi, > >In MethodSetCheck, the reportMethodAdded method uses: > final Severity severity = > !newClass.isInterface() && > (newClass.isFinal() || !newMethod.isAbstract()) > ? Severity.INFO > : Severity.ERROR; > > >This means that adding a method to an interface is reported as an ERROR. >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 to >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? > > > I think we could follow the same route that I'm currently investigating for source incompatibilies. I.e. the ApiDifference should not have only one severity field, but one field for each compatibility class (linktime, runtime, source). In the meantime I think ERROR is OK - "clirr" stands for "//Client binaries must link and *run* with new //releases...". To me linktime and runtime errors are pretty much equivalent, either of them means I can't use the new version as a drop-in replacement because my application will bomb out with some exception sooner or later. >------------------------------- > >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 than >an ERROR (linktime problem). > > > See above - I think differentiating between linktime and runtime errors is not very useful from the perspecive of a library user. Let's keep it at ERROR until we have the extended API available in the ApiDifference class. >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 possible >as far as I know.. > > > Maybe it's because I've just recovered from the flu, but I don't understand your point here... There are theoretically four combinations of final and abstract. One of them (abstract method in a non-final class) creates a compatibility problem and the code reports that correctly. The remaining three cases do not create a compatibility problem. One of those remaining three cases (abstract method and final class) cannot be achieved with a correct java compiler - but that doesn't matter here, because it is still not a compatibility problem: there can't be any subclasses where the method is missing because the class is final. Reporting INFO in that case is OK, no? Cheers, Lars |
|
From: Simon K. <si...@ec...> - 2004-06-28 04:27:54
|
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 |
|
From: <lak...@t-...> - 2004-06-28 05:43:31
|
Simon Kitching wrote: >On Fri, 2004-06-25 at 17:53, Lars Kühne wrote: > > >>Simon Kitching wrote: >> >> >> >>>Hi, >>> >>>In MethodSetCheck, the reportMethodAdded method uses: >>> final Severity severity = >>> !newClass.isInterface() && >>> (newClass.isFinal() || !newMethod.isAbstract()) >>> ? Severity.INFO >>> : Severity.ERROR; >>> >>> >>>This means that adding a method to an interface is reported as an ERROR. >>>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 to >>>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? >>> >>> >>> >>> >>> >>I think we could follow the same route that I'm currently investigating >>for source incompatibilies. I.e. the ApiDifference should not have only >>one severity field, but one field for each compatibility class >>(linktime, runtime, source). >> >>In the meantime I think ERROR is OK - "clirr" stands for "//Client >>binaries must link and *run* with new //releases...". To me linktime and >>runtime errors are pretty much equivalent, either of them means I can't >>use the new version as a drop-in replacement because my application will >>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. > > > You mean "having two incompatible classes in the same classloader", right? >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. > >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. > > > Hmmm... I think the situation for commons-collections is a bit special. I like to think of binary compatibility as "binary compatible in all possible scenarios". From the Eclipse paper: "Adding a new method to an API interface that is implemented by Clients (e.g., a callback, listener, or visitor interface) breaks compatibility because hypothetical pre-existing implementations do not implement the new method." There are many libraries where users are actually expected to provide interface implementations. Think PropertyChangeListener in the JDK. Think Appender in log4j. Think all kinds of Decorators. Clirr can't find out whether the interface is meant to be implemented externally (maybe in JDK 1.5 some class metadata could be used to express that...) My current thinking is this should be flagged as an error, and clirr users who modify interfaces that are typically not implemented by a user (like in commons-collection) can somehow filter those messages. The effect is that if you introduce new methods in an interface you are forced to make a decision and actively approve the change. You cannot ignore the error (like you can ignore a warning) because it will break your build. >>>------------------------------- >>> >>>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 than >>>an ERROR (linktime problem). >>> >>> >>> >>> >>> >>See above - I think differentiating between linktime and runtime errors >>is not very useful from the perspecive of a library user. Let's keep it >>at ERROR until we have the extended API available in the ApiDifference >>class. >> >> >> >>>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 possible >>>as far as I know.. >>> >>> >>> >>> >>> >>Maybe it's because I've just recovered from the flu, but I don't >>understand your point here... >> >>There are theoretically four combinations of final and abstract. One of >>them (abstract method in a non-final class) creates a compatibility >>problem and the code reports that correctly. >> >>The remaining three cases do not create a compatibility problem. One of >>those remaining three cases (abstract method and final class) cannot be >>achieved with a correct java compiler - but that doesn't matter here, >>because it is still not a compatibility problem: there can't be any >>subclasses where the method is missing because the class is final. >>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 :-) > > > Yep, change the code as you like, I think we agree on the calculation result for the relevant three cases :-) Lars PS: will you start working on the i18n improvements you suggested or should I do it? |
|
From: Simon K. <si...@ec...> - 2004-06-28 05:53:14
|
On Mon, 2004-06-28 at 17:54, Lars K=FChne wrote: > Simon Kitching wrote: >=20 > >> > > > >The issue is that binary incompatibilities are *always* failures; just > >having two incompatible classes together in the classpath causes a > >failure. > > > > =20 > > >=20 > You mean "having two incompatible classes in the same classloader", rig= ht? Yep.=20 >=20 > >For "warning" level issues, the app *may* run or *may* cause a runtime > >exception depending on exactly how the classes are used. > > =20 > > >=20 > Hmmm... I think the situation for commons-collections is a bit special.= =20 > I like to think of binary compatibility as "binary compatible in all=20 > possible scenarios". >=20 > From the Eclipse paper: "Adding a new method to an API interface that=20 > is implemented by Clients (e.g., a callback, listener, or visitor=20 > interface) breaks compatibility because hypothetical pre-existing=20 > implementations do not implement the new method." >=20 > There are many libraries where users are actually expected to provide=20 > interface implementations. Think PropertyChangeListener in the JDK.=20 > Think Appender in log4j. Think all kinds of Decorators. Clirr can't fin= d=20 > out whether the interface is meant to be implemented externally (maybe=20 > in JDK 1.5 some class metadata could be used to express that...) >=20 > My current thinking is this should be flagged as an error, and clirr=20 > users who modify interfaces that are typically not implemented by a use= r=20 > (like in commons-collection) can somehow filter those messages. >=20 > The effect is that if you introduce new methods in an interface you are= =20 > forced to make a decision and actively approve the change. You cannot=20 > ignore the error (like you can ignore a warning) because it will break=20 > your build. Ok, I'm happy with that as long as we can document this, so when people see ERROR being reported on their interfaces they know why, and know that we know that it is "probably" an error, not "definitely" one. The new message framework (I18N) should help, as messages will now have codes that can link to further explanations. > >> > >>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. > >> > >>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? > > =20 > > >=20 > Yep, change the code as you like, I think we agree on the calculation=20 > result for the relevant three cases :-) I see: non-final + non-abstract --> info non-final + abstract --> ERROR final + non-abstract --> info final + abstract --> impossible ie the final/non-final state of the class is irrelevant. I have just committed a patch to MethodSetCheck to rewrite the "reportAdd= ed" method using if-stmts. I hope you can check this for me. > PS: will you start working on the i18n improvements you suggested or=20 > should I do it? I'm right in the middle of committing some patches now - literally. PS: I'm logged in to IRC if you wish to catch up. Of course, design discussions may well be better held by email so they are archived. Cheers, Simon |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X