Bug 323633 - [1.5][compiler] Reconciler issues mixing 1.4 projects with & 1.5 project with generics.
Summary: [1.5][compiler] Reconciler issues mixing 1.4 projects with & 1.5 project with...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.6   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.7 M2   Edit
Assignee: Srikanth Sankaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 323855 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-08-25 12:34 EDT by Thomas Watson CLA
Modified: 2010-09-14 10:22 EDT (History)
4 users (show)

See Also:
Olivier_Thomann: review+


Attachments
test projects (3.96 KB, application/zip)
2010-08-25 12:34 EDT, Thomas Watson CLA
no flags Details
Preminimary patch for TJW (6.89 KB, text/plain)
2010-08-30 04:52 EDT, Srikanth Sankaran CLA
no flags Details
Preliminary patch - Take 2. (7.09 KB, patch)
2010-08-30 06:13 EDT, Srikanth Sankaran CLA
no flags Details | Diff
Patch under test (8.58 KB, patch)
2010-09-06 11:24 EDT, Srikanth Sankaran CLA
no flags Details | Diff
Patch with regression tests (15.99 KB, patch)
2010-09-07 01:22 EDT, Srikanth Sankaran CLA
no flags Details | Diff
Test helper to reconcile all files from all projects in workspace (7.12 KB, application/octet-stream)
2010-09-08 09:03 EDT, Srikanth Sankaran CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Watson CLA 2010-08-25 12:34:44 EDT
Created attachment 177441 [details]
test projects

This is after the fix for bug305259 and is not a dup of that bug.

Build: I20100824-1210

Import the attached projects and see the strange error in the editor only for the /client/src/client/Test.java class.  It seems to think Headers is not really a Dictionary even though it extends Dictionary.
Comment 1 Srikanth Sankaran CLA 2010-08-26 03:27:05 EDT
Is it going to be possible for you to zip up
your workspace and send it - or if it is all
on open CVS repository, help with instructions
to recreate a workspace locally.

I would like to programmatically create working
copies of all code there and see if the reconciler
can handle this. That may be a better and efficient
way of handling this issue than attacking the ripples
piecemeal fashion.

I can reproduce the current problem and am looking
into it.
Comment 2 Thomas Watson CLA 2010-08-26 09:20:32 EDT
You could load the releng core.map file to get all the projects in equinox/core ...
http://dev.eclipse.org/viewsvn/index.cgi/org.eclipse.releng/maps/core.map?view=markup

The main project with generic types is org.eclipse.osgi.  Most of the other projects use 1.4 compile settings.  But it is difficult to tell which files have errors in the editors until you open each file.  In this case I was creating a new test (in org.eclipse.osgi.tests) that was using the internal type Headers from org.eclipse.osgi.
Comment 3 Srikanth Sankaran CLA 2010-08-26 09:35:29 EDT
When Dictionary is imported by 1.4 code, a binary type
is constructed by reading the library jar file. But
since it is only a 1.4 project, we throw away all
generics information and use the erased form.

But when Header references Dictionary as a super
type it claims to extend Dictionary<K, V>. In our
cache, we have Dictionary stripped of all generics
information and so the compiler is confused about the
attempt to parametrize a non-generic type and things
go south pretty quickly from there.

I see three possibilities:

(1) Retain all generic information in source types
and binary types - If they are present, don't
throw them away.

(2) Provide the same treatment to source types as we
do for binary types - i.e completely get rid of
type parameters and materialize an erased equivalent
version of source type. The original problem with
bug 305259 was that we threw away the type parameters
but they were still lingering around. The return type
was seen as S instead of Object as would be if erasure
were applied. 

The former would be unacceptable from a performance
memory foot print p.o.v.

The latter could be feasible, but it is not clear
how much work is needed.

(3) Yet another option is to provide a new preference controlled
mode where we retain generics information. So not all
and sundry need to pay the penalty.

I'll continue to investigate,
Comment 4 Srikanth Sankaran CLA 2010-08-28 22:42:17 EDT
*** Bug 323855 has been marked as a duplicate of this bug. ***
Comment 5 Srikanth Sankaran CLA 2010-08-28 22:57:37 EDT
(In reply to comment #3)

> (3) Yet another option is to provide a new preference controlled
> mode where we retain generics information. So not all
> and sundry need to pay the penalty.

Originally the question of whether generics information should
be dropped or not used to be settled by looking at the project's
source & compliance levels. If would appear, we ought to maintain
a "high water mark" compliance level for each project taking into
account the (high water mark) compliance levels of source projects
on the class path of a given project and use that to determine
whether generics should be retained or not.

Now a question: is the following a likely scenario that we want
to support :

P14 has P15 on its class path
P15 has generics in its API and some of that is used by P14
P14 uses a 1.4 JRE 
P15 uses 1.5 JRE.

Constructing source types in a properly erased form would solve
the problem completely side stepping the issue above of course.
Comment 6 Srikanth Sankaran CLA 2010-08-30 04:52:42 EDT
Created attachment 177705 [details]
Preminimary patch for TJW
Comment 7 Srikanth Sankaran CLA 2010-08-30 04:56:35 EDT
(In reply to comment #6)
> Created an attachment (id=177705) [details]
> Preminimary patch for TJW

[Inadvertently the commit button got pushed]

Preminimary patch for TJW's scenario. Haven't checked yet
with Markus's scenario. This patch hasn't been subjected
to torture tests yet.

This is based on a high water mark model.

This will not work if the two projects use two different
versions of JRE (or other library) one with generics
enabled and one without. That scenario may be a hard nut
to crack.
Comment 8 Srikanth Sankaran CLA 2010-08-30 05:49:25 EDT
(In reply to comment #3)

> (2) Provide the same treatment to source types as we
> do for binary types - i.e completely get rid of
> type parameters and materialize an erased equivalent
> version of source type.

Though there is a chicken and situation here, In order
to properly erase the type parameters, we need to able
to resolve the types referenced in/as supertypes, 
superinterfaces, method signatures, return types, thrown
exceptions, type parameter bounds etc. We may run into
inconsistencies in any of the steps there. For example
comment #3 talks about what happens when we try to resolve
the supertype (Dictionary) of Headers.

> (3) Yet another option is to provide a new preference controlled
> mode where we retain generics information. So not all
> and sundry need to pay the penalty.

The current high water mark approach does not need a user visible
preference.
Comment 9 Srikanth Sankaran CLA 2010-08-30 06:13:50 EDT
Created attachment 177711 [details]
Preliminary patch - Take 2.
Comment 10 Srikanth Sankaran CLA 2010-08-30 09:42:46 EDT
(In reply to comment #9)
> Created an attachment (id=177711) [details]
> Preliminary patch - Take 2.

Passes all JDT/Core tests, I'll release it after more testing,
writing tests, code review and etc.

(In reply to comment #2)

> The main project with generic types is org.eclipse.osgi.  Most of the other
> projects use 1.4 compile settings.  But it is difficult to tell which files
> have errors in the editors until you open each file.  In this case I was

I'll try to programatically create working copies and reconcile them.
Likewise for the JDT/UI project.
Comment 11 Markus Keller CLA 2010-08-30 10:24:30 EDT
(In reply to comment #5)
> Now a question: is the following a likely scenario that we want
> to support :
> 
> P14 has P15 on its class path

I'm not sure whether you've already solved that, but this is a valid and likely scenario that we should support. E.g. org.eclipse.pde.ui is now in this situation, since org.eclipse.jdt.junit has moved to 1.5.
Comment 12 Srikanth Sankaran CLA 2010-08-30 10:39:23 EDT
(In reply to comment #11)
> (In reply to comment #5)
> > Now a question: is the following a likely scenario that we want
> > to support :
> > 
> > P14 has P15 on its class path
> 
> I'm not sure whether you've already solved that, but this is a valid and likely
> scenario that we should support.

Yes, the current fix and the earlier fix via bug 305259 address the
case of a P14 project having a P15 project in its class path and
using some generic APIs exported by P15.

The "P14 has P15 on its class path" part was a preamble to leading
to the scenario I was worried about:

> P14 uses a 1.4 JRE 
> P15 uses 1.5 JRE.

If this were to be supported, we have to deal with two different
versions of class files one with and one without generics.
Comment 13 Markus Keller CLA 2010-08-30 10:51:57 EDT
> > P14 uses a 1.4 JRE 
> > P15 uses 1.5 JRE.

Yes, that's exactly the case with pde.ui and jdt.junit. A plug-in usually has its Execution Environment (EE) on the build path, and if the workspace is set up correctly, the J2SE-1.4 EE is mapped to a 1.4 JRE and the J2SE-1.5 maps to a 1.5 JRE.
Comment 14 Srikanth Sankaran CLA 2010-08-30 19:59:17 EDT
(In reply to comment #13)
> > > P14 uses a 1.4 JRE 
> > > P15 uses 1.5 JRE.
> 
> Yes, that's exactly the case with pde.ui and jdt.junit. A plug-in usually has
> its Execution Environment (EE) on the build path, and if the workspace is set
> up correctly, the J2SE-1.4 EE is mapped to a 1.4 JRE and the J2SE-1.5 maps to a
> 1.5 JRE.

Not only is this hard to support, I am inclined to think such a model
should NOT be supported, for this will put JDT/Core into a schizophrenic
mode that would break many many things.

If a 1.4 project has its own underlying 1.4 JRE (the term "underlying" is
used to refer to a real 1.4 JRE as opposed to a 1.5 or later JRE being used
with a 1.4 compliance setting) and 1.5 project has a 1.5 JRE and if we were
to allow that, then will be two Sets, two Maps, two Lists, two Strings and
two everything.

1.4 project method doing a 

    Map m = getMapFrom15Project();

cannot be typechecked properly, as the LHS is a 1.4 Map and RHS will
be a 1.5 Map. Binding comparisons will not work with == operator which
will be catastrophic.

See that neither javac nor eclipse compiler per se ever have to deal
with this scenario.

For the record it is worth reiterating again that, this problem does
not show up with the eclipse compiler proper, only with the reconciler.
In the compiler's case the name lookup happens by using the output location
of the prerequisite project and as mentioned earlier the compiler has
no issues letting a 1.4 project use 1.5 compiler generated class files
(After all a 1.4 project can link with a 1.5 JRE) as opposed to the
source location of the prerequisite project being used in the class path.

Olivier, Dani, what are your thoughts ?
Comment 15 Srikanth Sankaran CLA 2010-08-30 22:20:10 EDT
Olivier, why don't we always use the output folder of the
prerequisite project as a class path entry in the reconciler's
lookup/name environments instead of conjuring a compilation
unit from the model as we do ?
Comment 16 Markus Keller CLA 2010-08-31 08:21:15 EDT
(In reply to comment #15)
> Olivier, why don't we always use the output folder of the
> prerequisite project [..]

Because the prerequisite may not have been built yet. Reconcile should work even with auto-build off and all projects cleaned.

(In reply to comment #14)
> [..] two Sets, two Maps, two Lists, two Strings and two everything.

A solution for that could be to compile everything against the JRE from the project whose CU triggered the reconcile, and then throw away all problems that are caused by missing generics in sources from dependent 1.4 projects.
Comment 17 Srikanth Sankaran CLA 2010-08-31 08:45:45 EDT
(In reply to comment #16)
> (In reply to comment #15)
> > Olivier, why don't we always use the output folder of the
> > prerequisite project [..]
> 
> Because the prerequisite may not have been built yet. Reconcile should work
> even with auto-build off and all projects cleaned.

That sounds right. OK.

> (In reply to comment #14)
> > [..] two Sets, two Maps, two Lists, two Strings and two everything.
> 
> A solution for that could be to compile everything against the JRE from the
> project whose CU triggered the reconcile,

This part is what we do (have been doing) today.

> and then throw away all problems that
> are caused by missing generics in sources from dependent 1.4 projects.

Can you clarify this part ? As the comment 0 test case shows problems
can show up in the 1.4 project due to 1.5 project code's generics not
being resolved properly.

When Headers in 1.5 code extends Dictionary<K,V>, in our lookup we
discover Dictionary from 1.4 and it now looks like the code is trying
to parameterize a 1.4 (and so non-generic) type. The reconciler issues
an error for this which does not show up in the editor as the editor tab
in focus holds a 1.4 project.

More importantly having discovered a problem while resolving the super type
of Headers, the compiler now resets the supertype of Headers to Object.
This is the reason for the reconciler thinking "Headers is not
really a Dictionary even though it extends Dictionary".

We do already in effect throw away the error reported against the 1.5
project since it is hidden and if it were to become the editor tab
content a fresh reconcile would take place in a 1.5 context where
we will find a 1.5 Dictionary that can be successfully parameterized
and extended by Headers.
Comment 18 Srikanth Sankaran CLA 2010-08-31 08:54:11 EDT
(In reply to comment #16)

> A solution for that could be to compile everything against the JRE from the
> project whose CU triggered the reconcile, and then throw away all problems that
> are caused by missing generics in sources from dependent 1.4 projects.

Did you mean "throw away all problems that are caused by missing generics in
sources from prerequisite 1.5 projects" ???

If so, as the discussion in comment# 17 shows, it is not always possible
to discern what is the cause of the problem in a 1.4 project. When we see
that the super type of Headers is Object and that you are trying to assign
to a Dictionary, we don't know that the super type is set to Object as a
means of recovery from some other phase of the compiler.
Comment 19 Markus Keller CLA 2010-09-01 14:27:53 EDT
(In reply to comment #17)
> > and then throw away all problems that
> > are caused by missing generics in sources from dependent 1.4 projects.
> 
> Can you clarify this part ? As the comment 0 test case shows problems
> can show up in the 1.4 project due to 1.5 project code's generics not
> being resolved properly.

(In reply to comment #18)
> Did you mean "throw away all problems that are caused by missing generics in
> sources from prerequisite 1.5 projects" ???

I really meant to throw away the errors in the 1.4 project ("client" in the example from comment 0). But I see now that these errors can appear as a consequence of errors in the compilation of the 1.5 project ("api"), and in the end, I agree that this approach can't succeed.
Comment 20 Markus Keller CLA 2010-09-01 14:36:22 EDT
Another solution could be to compile everything against the latest JRE that shows up during the reconcile. This poses a chicken-and-egg problem since you don't know the right JRE upfront. As a straightforward solution, you could just handle this situation when it really happens, and then abort the initial reconcile and restart it with the later JRE. That's not super-efficient, but it could solve the problem for the problematic cases without affecting the "normal" case.
Comment 21 Srikanth Sankaran CLA 2010-09-01 22:15:17 EDT
(In reply to comment #20)
> As a straightforward solution, you could just
> handle this situation when it really happens, and then abort the initial
> reconcile and restart it with the later JRE.

As the earlier example showed (Headers showing up as with super class
set to java.lang.Object), we could/would be hardpressed to recognize a
problem as being the consequence of JRE mismatch (it could be a genuine
type mismatch error.) We could doing end up doing lots of spurious aborts
and restarts.

To ascertain whether we should really bend over backwards to support this
scenario, I would like to hear the reasons why a project would resist a
migration to a 1.5+ JRE. 

None of the source level, compliance level, code generation target level
need to change. Only the JRE.
Comment 22 Thomas Watson CLA 2010-09-01 23:17:13 EDT
(In reply to comment #21)
> (In reply to comment #20)
> To ascertain whether we should really bend over backwards to support this
> scenario, I would like to hear the reasons why a project would resist a
> migration to a 1.5+ JRE. 
> 
> None of the source level, compliance level, code generation target level
> need to change. Only the JRE.

This would be an awkward way to work IMO.  What would the bundle project state for its required execution environment.  For PDE to understand what JRE to set for the class path container it would have to be J2SE 1.5.

Another option is for bundles to compile against the latest OSGi/Minimum execution environment 1.2 that will be included in the forthcoming OSGi R.3 specification.  This updated EE will include the generic attributes for the types provided by the VM.  This of coarse will only work if the bundle limits itself to the APIs provided by the OSGi/Minimum 1.2 APIs.
Comment 23 Markus Keller CLA 2010-09-02 05:48:18 EDT
(In reply to comment #21)
The decision to abort and restart should only be based on the used JRE (i.e. as soon as you see a class with compliance greater than the original compliance, abort and restart).

> To ascertain whether we should really bend over backwards to support this
> scenario, I would like to hear the reasons why a project would resist a
> migration to a 1.5+ JRE. 

A 1.4 project should never compile against a 1.5 class library. You would never know what APIs are really available in 1.4. Furthermore, it's not a question of whether we can suggest a workaround for affected projects. We should not force users to change their compliance only to address a technical limitation. See also 
http://wiki.eclipse.org/index.php/Execution_Environment#I_have_prerequisites_that_require_1.5_to_run.2C_so_shouldn.27t_I_require_1.5_too.3F
Comment 24 Srikanth Sankaran CLA 2010-09-02 07:33:44 EDT
(In reply to comment #23)
> (In reply to comment #21)
> The decision to abort and restart should only be based on the used JRE (i.e. as
> soon as you see a class with compliance greater than the original compliance,
> abort and restart).

Good point.

> A 1.4 project should never compile against a 1.5 class library. You would never
> know what APIs are really available in 1.4. 

Duh. Of course. Don't know what I was thinking when I posed that question.

(In reply to comment #20)
> Another solution could be to compile everything against the latest JRE that
> shows up during the reconcile. This poses a chicken-and-egg problem since you
> don't know the right JRE upfront. 

The current patch determines up front whether generics should be retained when
a 1.5 class file is internalized on behalf of a 1.4 project. Likewise, we should
be able to determine the latest JRE upfront.

But wouldn't that approach open up the same issue of "You would never know
what APIs are really available in 1.4." albeit only in the reconciler ?
Comment 25 Markus Keller CLA 2010-09-02 08:20:05 EDT
> But wouldn't that approach open up the same issue of "You would never know
> what APIs are really available in 1.4." albeit only in the reconciler ?

Yes, but I guess the only solution for that problem would be to break up the reconciler and have independent compiler environments for each project -- probably very difficult to implement and unclear how the connection between the same classes from different JREs would look like (see "two Sets, two Maps,..").

Question is whether this would also affect content assist. If it does, and we would get too many or generic proposals in the 1.4 projects, then that's a no-go. If it doesn't, and we only get too few errors because references to 1.5 APIs would compile, then I could accept that. It's still better than outright wrong errors, and the real build will catch the remaining API compatibility issues.
Comment 26 Srikanth Sankaran CLA 2010-09-02 20:02:42 EDT
(In reply to comment #25)
> > But wouldn't that approach open up the same issue of "You would never know
> > what APIs are really available in 1.4." albeit only in the reconciler ?
> 
> Yes, but I guess the only solution for that problem would be to break up the
> reconciler and have independent compiler environments for each project --

OK, Thanks a lot Markus and Tom for the discussion. I'll investigate
both the "erase generics in source type" approach (best case scenario if
it can be made to work) and the "use the highest level JRE for reconcile
for mixed mode projects" approach.

You can use the current patch if it unblocks your problems for the moment,
but hopefully there should a fix you can test in a week's time.
Comment 27 Thomas Watson CLA 2010-09-02 22:43:19 EDT
(In reply to comment #26)
> You can use the current patch if it unblocks your problems for the moment,
> but hopefully there should a fix you can test in a week's time.

I can test the patch if needed, but this is not blocking.  Just causes some unexpected errors in the editor, otherwise the bytecode is fine.
Comment 28 Srikanth Sankaran CLA 2010-09-06 11:24:46 EDT
Created attachment 178273 [details]
Patch under test

This patch works well against all the three cases
(2 from JDT/UI and one from equinox/core.)

Under test.
Comment 29 Srikanth Sankaran CLA 2010-09-06 17:24:05 EDT
(In reply to comment #28)
> Created an attachment (id=178273) [details]
> Patch under test

All JDT/Core tests pass.

It is odd that BinaryTypeBinding would use source level to
determine whether to retain generics artifacts from a
class file while SourceTypeConverter would use compliance
level to determine the same - It is unclear why/how this
dichotomy came about - This patch leaves it as it was.

This patch continues along the lines of the fix  bug 305259 to
handle a 1.5 source type referenced during the reconcile of a
1.4 type in its own sandbox. However when deciding whether to
retain type parameters in a type be it source type or binary
type, we take into consideration the original source & compliance
settings as well the sandbox settings as needed.

Per earlier, I'll attempt to programmatically reconcile the entire
JDT/UI set of projects before releasing this patch.
Comment 30 Srikanth Sankaran CLA 2010-09-06 23:06:48 EDT
(In reply to comment #29)

> Per earlier, I'll attempt to programmatically reconcile the entire
> JDT/UI set of projects before releasing this patch.

With the following projects in the workspace and using 3.7 HEAD

org.eclipse.jdt.ui
org.eclipse.jdt.ui.tests
org.eclipse.jdt.ui.tests.refactoring
org.eclipse.jface.text.tests
org.eclipse.ltk.core.refactoring
org.eclipse.test.performance
org.eclipse.text.tests

I get reconcile errors in 40 files shown below:

JavaProjectHelper.java 
BuildpathModifierActionEnablementTest.java 
CodeFormatterTest.java 
JavaModelUtilTest.java 
ClassPathDetectorTest.java 
ContentProviderTests.java 
ContentProviderTests5.java 
MarkerResolutionTest.java 
ModifierCorrectionsQuickFixTest.java 
UnresolvedMethodsQuickFixTest.java 
SaveParticipantTest.java 
GetterSetterQuickFixTest.java 
LocalCorrectionsQuickFixTest.java 
UnresolvedVariablesQuickFixTest.java 
SerialVersionQuickFixTest.java 
JavadocQuickFixTest.java 
UnresolvedTypesQuickFixTest.java 
TypeMismatchQuickFixTests.java 
TypeParameterMismatchTest.java 
JavaLeakTest.java 
RenameSourceFolderChangeTests.java 
RenameTypeTests.java 
PushDownTests.java 
DocumentChangeTest.java 
RenameNonPrivateFieldTests.java 
ExtractClassTests.java 
ValidateEditTests.java 
MoveMembersTests.java 
RefactoringScannerTests.java 
RenamePackageTests.java 
IntroduceParameterObjectTests.java 
RenameResourceChangeTests.java 
PasteActionTest.java 
CopyTest.java 
MoveTest.java 
CopyResourcesToClipboardActionTest.java 
CopyToClipboardActionTest.java 
DeleteTest.java 
MultiMoveTest.java 
AbstractRefactoringTestSetup.java 

With the latest patch, all of the problems go away.

I'll add a regression test and after code review will release
the patch.
Comment 31 Srikanth Sankaran CLA 2010-09-07 01:22:53 EDT
Created attachment 178293 [details]
Patch with regression tests
Comment 32 Srikanth Sankaran CLA 2010-09-07 01:25:13 EDT
Olivier, Please review, TIA.
Comment 33 Olivier Thomann CLA 2010-09-08 08:32:00 EDT
Looks good.
Comment 34 Srikanth Sankaran CLA 2010-09-08 08:56:18 EDT
Released in HEAD for 3.7 M2
Comment 35 Srikanth Sankaran CLA 2010-09-08 09:03:00 EDT
Created attachment 178400 [details]
Test helper to reconcile all files from all projects in workspace

TJW: This is what was used to test that all JDT/UI
projects reconcile OK. In case you want to similarly
test with Equinox projects.
Comment 36 Thomas Watson CLA 2010-09-08 09:13:37 EDT
(In reply to comment #35)
> Created an attachment (id=178400) [details]
> Test helper to reconcile all files from all projects in workspace
> 
> TJW: This is what was used to test that all JDT/UI
> projects reconcile OK. In case you want to similarly
> test with Equinox projects.

Thanks for the hard work to address this bug.  I will pick up the next n-build and give it a try.
Comment 37 Olivier Thomann CLA 2010-09-14 10:22:52 EDT
Verified for 3.7M2 using I20100914-0100