Community
Participate
Working Groups
When going from TM-RSE version 2.0 to 3.0, we renamed a compile-time constant field of an interface (which can be seen as Delete API Field Constant + Add API Field Constant). API Tools flags the new field as "missing @since" but it should really flag the deleted constant as breaking API Change. At least if I'm reading the API Guide right: http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Interfaces says that "Delete API Field ... Breaks compatibility". Personally I'm not 100% sure if this is actually true, since the API Guide http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_fields also says in annotation "(2) Java compilers always inline the value of constant fields" so deleting a constant field from an API interface would not break binary compatibility. It definitely breaks source compatibility, but we usually dont care about that; It might also break contract compatibility, if the client expects a particular behavior from setting the constant; but even that is not necessarily the case when the constant is just renamed. So perhaps this bug is INVALID, API Tooling does the right job and the API Guide needs to be updated to talk about interface field constants specifically in http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Interfaces Anyways, here is the concrete example in code: Version 2.0: interface IHostSearchConstants { public static final int CANCELLED = 2; } Version 3.0: interface IHostSearchConstants { public static final int CANCELED = 2; } CANCELLED was renamed to CANCELED, and API Tools flags the new name as "missing @since tag" which is not really correct in this case. Perhaps the API Tools should support an "@renamed" annotation in Javadoc to handle this case?
Thinking about this again, my feeling is that compile-time constants are a bit of a corner case. On the one hand, they do not influence binary compatibility; on the other hand, addition or removal of a compile-time constant very likely goes hand-in-hand with a change in API contract or semantics, but the tooling can only guess this. Anyways, given these facts about a change in compile-time constants: * source compatibility is definitely broken * binary compatibility is definitely not broken * API contract compatibility is probably broken but not sure I would suggest that detection of any change in compile-time constants should * be flagged as a WARNING by the API Tools but not as an ERROR; * produce an @since reminder if not there in new API constant.
(In reply to comment #1) > I would suggest that detection of any change in compile-time constants should > * be flagged as a WARNING by the API Tools but not as an ERROR; API tools only reports binary incompatible changes. > * produce an @since reminder if not there in new API constant. This should already work. The renaming of the constant that you did is seen as a removal of a field and an addition of another field. If this interface can be implemented by the client, this is seen as a binary breakage as it can lead to an incompatible class change error. If this interface should not be implemented by the clients, then this should be ok as long as we are concerned only with compile-time constants.
On a second thought, I think we should rename our "binary compatibility error" to "compatibility error". Changing the value of a compile-time constant is not a binary breakage as is, but the existing code is likely broken. Imagine a case where you switch two values: from: interface I { int CONSTANT_1 = 1; int CONSTANT_2 = 2; } to: interface I { int CONSTANT_1 = 2; int CONSTANT_2 = 1; } If the user has a code like: switch(i) { case I.CONSTANT_1 : // do action for constant 1 break; case I.CONSTANT_2 : // do action for constant 2 break; } Even if this is not technically speaking a binary breakage, the user code is now broken. So I would be inclined to report this as a compatibility issue. So renaming the error message to "Compatibility issue" might be clearer since we cannot limit our detection to binary incompatible changes. Darin, any thoughts on this?
Yes - I support the change to use the term "Compatibility". It feels more user friendly and provides us with more flexibility.
(In reply to comment #3) > Rename our "binary compatibility error" to "compatibility error". -1 I'm strongly against this since it is not clearly defined. As you say yourself, change of an inlined compile-time constant "might" break compatibility but it does not necessarily. That's my whole point here -- issuing the compile-time constant changes as WARNING but not as ERROR. The term "binary compatibility" clearly and exactly defines what's being reported, by virtue of the spec on http://wiki.eclipse.org/Evolving_Java-based_APIs_2 If other (potential or actual) issues are to be reported, those reports can be named differently e.g. "potential compatibility problem" or "source compatibility error".
(In reply to comment #2) > If this interface can be implemented by the client, this is seen as a binary > breakage as it can lead to an incompatible class change error. Can you explain this? I can't see how a compile-time constant would lead to different behavior whether the interface is implemented or used.
If the tooling starts to report too many types of compatibility, then it might confuse the users more than anything else. Our goal for the API tools is to help the user to find potential problems. I don't think we should make lot of different types of compatibility since the goal of the user is to not break existing clients. As I mentioned in comment 3, changing the value doesn't prevent the existing binaries from running, but you can end up with a different runtime behavior and as is we should notify the user. This is something that should not be done. Regarding the way we should report this error, this is customizable from the preference page. So nothing prevents you from changing the default settings. You can also add a problem filter for this specific problem. When a new field is added to an interface that can be implemented by the client, this is a binary incompatible issue even if the field has a compile-time constant. Try the following test case: API code: interface I {} Client code: public class Y { public int CONST; } public class X extends Y implements I { public static void main(String[] args) { System.out.println(new X().CONST); } } You can compile all this and it runs fine. Now change I for the following code: interface I { int CONST = 4; } recompile only I. Do not recompile X or Y. Run your code again and you should have an incompatible class change error at runtime. The fact that CONST has a compile-time constant is irrelevant. This is why it is really important to define all interfaces that define constants as not implementable. I would definitely vote to report "incompatibilities" issues, i.e. things the user should not do to preserve the same runtime behavior for existing binaries. We should also work on the help contents to provide an example for each type of incompatibilities we can report. How does this sound ? Let me know if you have further concerns.
Thanks for the elaborate example. I can see now why addition or removal of compile-time constants breaks compatibility when the interface can be implemented by clients. Actually, I should have seen this from the "Evolving Java-based APIs" docs. I'll add the new @noimplement Javadoc annotation to my interfaces. I'm fine with your proposal except that I'd still vote for differentiating between Errors (definitely known to break binary compatibility) and Warnings (probably break compatibility). I'd also propose following additions to the "Evolving Java-based APIs" doc, provided that I'm getting this right: http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Interfaces Delete API Field - If field is compile-time constant and interface not implementable by clients - Binary Compatible (6) http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Classes Delete API Field - If field is compile-time constant and class is not subclassable by Client - Binary Compatible (10) Footnote (6) for interfaces and (10) for classes: While removal of a compile-time constant is binary compatible, it is not source compatible. Also, implementers of the API may easily fail to observe contract compatibility of the previous API when the former constant is no longer API. This is not an issue with renaming a compile-time constant though, which can be seen as removal + addition of a constant under the same API contract.
Regarding the original bug (see summary), I think that the API Tools should issue a warning for deleted compile-time constants, unless the interface (or class) is marked as @noimplement.
Thanks Martin for your comments. Regarding the default severity for problems, they are all set to errors. This can be customized from the preference page per project. As the settings is saved inside the project, it can be shared with team members. So using actual code, nothing prevents you from changing it to warnings. They are fined grained so you don't need to change all of them, but just the ones you want. If you cannot set the severity only the exact problem, let us know. This can be added. I'll send your comment regarding the "Evolving API" document to Jim who maintains it. Ok to close?
Ok to close when the error is actually issued for deleted compile-time constant and the @noextend / @noimplement annotation is observed to NOT flag the error in this case. I did not try this out with HEAD.
I'll check this case.
(In reply to comment #11) > Ok to close when the error is actually issued for deleted compile-time constant > and the @noextend / @noimplement annotation is observed to NOT flag the error > in this case. I did not try this out with HEAD. When you remove a constant value (by removing the final keyword or changing the compile-time constant for a non-compile time constant), you break all existing users that used this value in a switch statement. So I think we should always report it as a problem as soon as the field is visible for the client. We might want to treat it as the modification of the field constant value. We don't report a problem if the field is not visible for the client or it is visible (protected access) and there is restriction on the type (@noextend). An interface cannot have protected field, so @noimplement should not be considered. So you mean that you want this check to be added in case of removal of compile-time constant? This sounds fair as it should be consistent with changing the value.
(In reply to comment #13) > When you remove a constant value (by removing the final keyword or changing the > compile-time constant for a non-compile time constant), you break all existing > users that used this value in a switch statement. Well I'd say you POTENTIALLY break all existing users. You won't break them when the constant is just renamed or eliminated but its semantics is kept intact. You definitely break them when assigning a different value to the same constant (since this changes semantics). > So I think we should always report it as a problem as soon as the field is > visible for the client. Ok for me, because I agree that even if it's a POTENTIAL breakage only it should be flagged. The text of the flag could be "potential compatibility issue". I'm not yet sure what the impact is on the version numbering proposals ... somehow it would be nice if the user could review any potential breakage to specify whether he thinks it's actual breakage or not, such that the version numbering proposals could come up with a correct version number. > We might want to treat it as the modification of the field constant value. We > don't report a problem if the field is not visible for the client or it is > visible (protected access) and there is restriction on the type (@noextend). Ok > An interface cannot have protected field, so @noimplement should not be > considered. Ok > So you mean that you want this check to be added in case of removal of > compile-time constant? This sounds fair as it should be consistent with > changing the value. Yes, sounds good to me.
Ok, I'll implement the same check for removal of the constant value and modification of the value. Since we have flexible settings for severity through the preference page I am not inclined to add the "potential errors". API tooling is about helping the user to maintain good APIs. So if we notice something that might break existing users, we should report it. If the user believes that these errors should only be warnings, then he can change the preferences for the project. However this should not impact the versioning scheme of the corresponding bundles as this has consequence on the OSGi resolver. If the version is not changed, then you can end up with some plugins in your environment that are not compatible due to the fact that the plugin that introduced the breakage is still within the version range of dependent plugins. Darin, if we contribute again for M6, I'd like to add the change described in comment 13. Thanks, Martin for your comments.
Since we don't plan to recontribute for M6 (unless something really bad is found), I set the target for M7.
(In reply to comment #15) > However this should not impact the versioning scheme of the corresponding > bundles as this has consequence on the OSGi resolver. I'm talking about following scenario that I fell into: * Baseline of bundle is foo.bar_1.0.0.qualifier * I'm renaming a compile-time constant which does not break binary API * I'm going to foo.bar_1.1.0.qualifier --> But API Tooling thinks that I have broken API due to renaming the constant, so it issues an error and tells me that I should be doing foo.bar_2.0.0.qualifier Given that my foo.bar plugin may be very complex, I'd like to be able to rely on API Tooling come up with a correct version number proposal, so I'd like to mark "potential" breaking API changes as either breaking or non-breaking just because I checked an know what's been done.
So what you are really asking is to ignore the bundle versioning scheme based on what the users has decided to ignore. This is quite different from the original problem. I think we are right to report an error, but you want to be able to say that this is ok and don't provide report wrong @since tags values when the @since tag refers to the next version (from the user point of view). Let's create a new bug report for this issue. See bug 224456. Please add yourself to the CC list and let's continue the discussion there. I'll close this one as soon as the fix (see comment 13) is released.
Awsome. Thanks!
Released for 3.4M7. Fix is described in comment 13. Darin, please verify.
Moving to 3.4M6 as we recontribute for bug 224636.
Reopen to assign to Darin.
Fixed. Darin, please verify.
Verified.
