Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 312889

Summary:

Missing compatibility problem: "overriding" a field

Product:

[Eclipse Project] PDE

Reporter:

Tristan de Inés <tristan_de_ines>

Component:

API Tools

Assignee:

PDE API Tools Inbox <pde-apitools-inbox>

Status:

CLOSED WONTFIX

QA Contact:

Severity:

normal

Priority:

CC:

curtis.windatt.public, darin.eclipse, Michael_Rennie, Olivier_Thomann, Vikas.Chandra

Version:

3.6

Target Milestone:

---

Hardware:

OS:

Windows 7

Whiteboard:

stalebug

Attachments:

Description	Flags
Attempted "quick fix" - prob. faulty or incomplete	none
Implemented 3 new binary compatibility problems described in Javaspec 13.4.8	none
Updated patch with some regression tests	none

Description Tristan de Inés

2010-05-14 07:27:24 EDT

Build Identifier: I20100429-1549

Hi,

I studied the (somewhat informal) Java Spec on binary compatibility
(http://java.sun.com/docs/books/jls/third_edition/html/binaryComp.html)
and extracted 28 concrete binary compatibility problems. Then I created two plug-in projects to provoke these problems (the first to use as the baseline, the second to implement the binary compatibility conflicts.

The API Tools correctly detects and reports all problems except one;
adding a field f to a class c can lead to binary compatibility breakage if a superclass of c contains a field f' of the same type and name as f and one of the following is true:
- the visibility of f is lower than f' (leads to IllegalAccessError at runtime)
- f is static != f' is static (leads to IncompatibleClassChangeError at runtime)

This is described in Java Spec:
"13.4.8 Field Declarations
Widely distributed programs should not expose any fields to their clients. Apart from the binary compatibility issues discussed below, this is generally good software engineering practice. Adding a field to a class may break compatibility with pre-existing binaries that are not recompiled.

Assume a reference to a field f with qualifying type T. Assume further that f is in fact an instance (respectively static) field declared in a superclass of T, S, and that the type of f is X. If a new field of type X with the same name as f is added to a subclass of S that is a superclass of T or T itself, then a linkage error may occur. Such a linkage error will occur only if, in addition to the above, either one of the following conditions hold:

    * The new field is less accessible than the old one.
    * The new field is a static (respectively instance) field."


I tried implementing this myself (for fun & exercise) by:
- adding a new FIELD_REDUCED_ACCESS constant to IDelta
- creating a new Delta in ClassFileComparator.reportFieldAddition(...)
- marking FIELD_REDUCED_ACCESS as not compatible in DeltaProcessor.isClassCompatible(...)

But I guess something is missing, as I can't get a problem marker to appear for this. I'll add a code attachment.

Reproducible: Always

Comment 1 Tristan de Inés

2010-05-14 07:37:58 EDT

Created attachment 168525 [details]
Attempted "quick fix" - prob. faulty or incomplete

Comment 2 Olivier Thomann

2010-05-14 18:30:24 EDT

Please try to provide patch as a patch format. This helps to apply it.
Thanks.

Comment 3 Tristan de Inés

2010-05-17 08:18:32 EDT

Created attachment 168716 [details]
Implemented 3 new binary compatibility problems described in Javaspec 13.4.8

Implemented JavaSpec 13.4.8 binary compatibility problem in this patch:
3 new api binary compatibility problems introduced:

- shadowing a field with reduced access in subclass 
(CLASS_ADDED_FIELD_REDUCED_ACCESS)

- shadowing a field where new field is static and old super-field is instance
(CLASS_ADDED_FIELD_NON_STATIC_TO_STATIC)

- shadowing a field where new field is instance and old super-field is static
(CLASS_ADDED_FIELD_STATIC_TO_NON_STATIC)

Problem markers appear correctly now. Note that I only implemented this in the core "org.eclipse.pde.api.tools". I didn't make any changes to ui.

--

P.S: creating a new compatibility problem in API Tools is pretty hard. While implementing the detection in "ClassFileComparator" is straightforward, the way API Tools checks if a delta is an issue by involving lots of constants and creating obscure ids is rather unintuitive. To create a new API problem you have to:

1. Implement comparison and create delta in "ClassFileComparator"
2. create new compatibility problem constant in "IDelta"
3. add switch-cases for constants created in "IDelta" in "ApiProblemFactory", that return String descriptions of the compatibility constants.
4. Add text string descriptions in "problemmessages.properties"
5. create concatenated change-type + problem-kind constants in "IApiProblemTypes"
6. add constant field created in "IApiProblemTypes" to "allCompatibilityKeys"
7. add switch-case for delta compatiblity of new problem to "DeltaProcessor"
8. add String name of new "IDelta" constants to "Util" class.

Many of these steps deal with String-descriptions of the compatibility problem. But these are still required as the string-description is used as id for the preferences to read if the problem is an Error, Warning or should be Ignored. This was why no markers appeared in my earlier implementation. The check was there, but there was no string-description to read from the properties, so the problem was ignored.

I guess that implementing new API problems isn't something that is meant to be extended or done often.

Comment 4 Olivier Thomann

2010-05-17 14:05:12 EDT

Thanks for the patch.
Tagging for 3.6.1 inclusion.

It is too late at this time for 3.6.

Comment 5 Olivier Thomann

2010-05-18 12:44:31 EDT

Created attachment 168967 [details]
Updated patch with some regression tests

Updated patch. The first patch was missing a return statement.
Will continue to work on this for 3.6.1. Many more regression tests are needed.

Comment 6 Darin Wright

2010-08-11 10:52:13 EDT

I think we should move this to 3.7 as the problem is not a regression.

Comment 7 Curtis Windatt

2011-01-31 12:40:51 EST

Olivier, is this something you are still considering for 3.7?

Comment 8 Olivier Thomann

2011-02-09 09:14:45 EST

(In reply to comment #7)
> Olivier, is this something you are still considering for 3.7?
This would be nice. I don't have time right now to polish the posted patch.

Comment 9 Olivier Thomann

2011-02-14 11:20:59 EST

Reassign to inbox. I don't have time for this :-(.

Comment 10 Eclipse Genie

2019-09-16 14:55:11 EDT

This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.