Bug 360164 - Compile error in XSDImpl
Summary: Compile error in XSDImpl
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.8   Edit
Hardware: PC All
: P3 critical (vote)
Target Milestone: 3.8 M3   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 360317
  Show dependency tree
 
Reported: 2011-10-06 15:53 EDT by Carl Anderson CLA
Modified: 2011-10-24 04:33 EDT (History)
9 users (show)

See Also:
srikanth_sankaran: review+
srikanth_sankaran: review+


Attachments
team project set for the 15 projects requried for build wst.xsd.core (2.28 KB, text/plain)
2011-10-06 21:19 EDT, David Williams CLA
no flags Details
test & proposed fix (4.99 KB, patch)
2011-10-08 08:55 EDT, Stephan Herrmann CLA
no flags Details | Diff
Plausible fix (8.29 KB, patch)
2011-10-14 03:53 EDT, Srikanth Sankaran CLA
no flags Details | Diff
tests & combined fix v3 (13.21 KB, patch)
2011-10-18 10:29 EDT, Stephan Herrmann CLA
no flags Details | Diff
Additional fixes & tests (4.59 KB, text/plain)
2011-10-20 04:25 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 Carl Anderson CLA 2011-10-06 15:53:24 EDT
In org.eclipse.wst.xsd.core, in org.eclipse.wst.xsd.contentmodel.internal.XSDImpl, we are seeing a Java Compile error:

Compiler Report
Compiler: Eclipse Compiler for Java(TM) Version: 0.C13, 3.8.0 M3

Number of source files: 17 Number of classfiles: 37

Problems: 1 (Errors: 1 Warnings: 0 ) 

Source File: org/eclipse/wst/xsd/contentmodel/internal/XSDImpl.java 
1. ERROR: IsClassPathCorrect
The type java.lang.Enum cannot be resolved. It is indirectly referenced from required .class files 

XSDImpl.java : 

1 : /*******************************************************************************
Comment 1 Carl Anderson CLA 2011-10-06 15:57:12 EDT
This appears to be due to that plugin's usage of EMF.  Currently, EMF uses Java 1.5, but org.eclipse.wst.xsd.core only requires Java 1.4.
Comment 2 Carl Anderson CLA 2011-10-06 16:01:45 EDT
Digging even further, this appears to be caused by changes to the JDT.  If we set up a development environment using Eclipse 3.7.1 to compile, and target the same driver, this error does not appear.

I believe that this is related to a JDT bug fix post-M2... possibly bug 355838 ?
Comment 3 Carl Anderson CLA 2011-10-06 17:48:04 EDT
Moving down to JDT for their investigation/comment.

The class in question is not using EMF directly.  It should not require Java 1.5.  Either JDT is correctly displaying an incompatibility that has existed and this is EMF 2.8 M2a's fault for not being compatible with Java 1.4-based plugins, or (more likely) this is JDT detecting an incompatibility that does not truly exist.

(Note that the plugin in question has not changed and has been tested at runtime with EMF 2.8 M2 and previous EMF versions.)
Comment 4 Stephan Herrmann CLA 2011-10-06 19:17:22 EDT
(In reply to comment #3)
> Moving down to JDT for their investigation/comment.

Let's see, I guess we need a bit more information here, before we understand
the question.
 
> The class in question is not using EMF directly.

Am I assuming right that "the class in question" is XSDImpl?
For those who don't know the repository structure of wst by heart,
after some chasing I found this class here:

http://dev.eclipse.org/viewcvs/viewvc.cgi/sourceediting/plugins/org.eclipse.wst.xsd.core/src-contentmodel/org/eclipse/wst/xsd/contentmodel/internal/XSDImpl.java?view=markup&root=WebTools_Project

Is that the correct version?

However, the first thing I see there is:

import org.eclipse.emf.common.notify.Adapter;
import org.eclipse.emf.common.notify.Notifier;
import org.eclipse.emf.common.notify.impl.AdapterFactoryImpl;
import org.eclipse.emf.common.util.URI;
import org.eclipse.emf.ecore.EObject;
import org.eclipse.emf.ecore.resource.ResourceSet;
import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl;
import org.eclipse.emf.ecore.resource.impl.URIConverterImpl;

Is that the class that "is not using EMF directly"?

So, what exactly would you like us to investigate? :)


BTW: from the project page
http://www.eclipse.org/projects/project.php?id=webtools.webservices
I get forwarded to
http://dev.eclipse.org/viewcvs/viewvc.cgi/webservices/?root=Webtools_Project
which is a dead link
it must be
http://dev.eclipse.org/viewcvs/viewvc.cgi/webservices/?root=WebTools_Project
Comment 5 David Williams CLA 2011-10-06 21:17:21 EDT
I think Carl meant "The class in question is not using >Enum< directly."

I personally am not able to reproduce this in a fresh install ... but, I'm sure there is some permutation that would, since it happened in our batch build. 

For background, I asked a bug be opened when we "accidentally" used 0.C13, 3.8.0 M3 in the WTP Batch build, instead of 3.7.1 as we'd intended. So, thought we might be discovering an "early bug". 

I'll describe how I tried to reproduce ... maybe someone else can figure out an "alternative install" that might? 

I started with Eclipse SDK I20111004-2000, then added "emf-xsd" to the PDE runtime target, such as their "M2a" from 
http://www.eclipse.org/downloads/download.php?file=/modeling/emf/emf/downloads/drops/2.8.0/S201109231242/emf-xsd-SDK-2.8.0M2a.zip

Next, I imported all the projects needed for "wst.xsd.core" (was NOT easy ... I'll attach a team project set). 

And ... I don't see the error there. Maybe it'd make a difference if "emf-xsd" was "unzipped" into an install? I know EMFs M2a was kind of "special", in some way.
Comment 6 David Williams CLA 2011-10-06 21:19:01 EDT
Created attachment 204725 [details]
team project set for the 15 projects requried for build wst.xsd.core
Comment 7 Carl Anderson CLA 2011-10-06 21:37:23 EDT
Sorry, yes, it does not use Enum directly.

One other thing that might help in reproducing- this error does not appear until I have a Java 1.4 JRE installed in my development environment.  It does not have to be configured as the Execution Environment- once I installed it as a JRE, this error was reproduced.

With the same PDE target, but with Eclipse 3.7.1 ad the development environment, this error does not appear.
Comment 8 David Williams CLA 2011-10-06 22:04:59 EDT
> 
> One other thing that might help in reproducing- this error does not appear
> until I have a Java 1.4 JRE installed in my development environment. 

Yep, that did it. In my "fresh install" I forgot to add all my JREs back to JRE runtimes. Once I did, the error showed up in my fresh, "minimal" workspace.
Comment 9 Stephan Herrmann CLA 2011-10-07 03:55:11 EDT
One more question: what is the goal behind specifying a 1.4 execution
environment and compiling against other plug-ins that require 1.5?
Why would this be a useful exercise?

Note to the JDT/Core team: bug 349326 influences the order in which
types are resolved (need some super-types early). This *might* be the 
butterfly that caused the storm in wst.xsd.core.
Comment 10 David Williams CLA 2011-10-07 08:20:42 EDT
(In reply to comment #9)
> One more question: what is the goal behind specifying a 1.4 execution
> environment and compiling against other plug-ins that require 1.5?
> Why would this be a useful exercise?

Partially (mostly?) its just considered good architecture ... see 
http://wiki.eclipse.org/Execution_Environments#I_have_prerequisites_that_require_1.5_to_run.2C_so_shouldn.27t_I_require_1.5_too.3F 

where it says (and I think I might have written that section, so let us know if you disagree and think that's not true, as it has been the advised for a long time)
"... You should seek to use the smallest BREE possible. There's several reasons to use a BREE smaller than your prerequisites. One is just sound architecture. Things might change in the future, so you want to code your bundle based on your needs. Maybe that prerequisite will be refactored out next release. Similarly, you do not (normally) have control over your prerequisites BREEs. They might lower theirs, they might raise it ... no reason to hard code some assumption in your bundle, based on your prerequisites. "

Second, there could be real cases where the pre-req is optional, etc., such that the bundle can run in 1.4 with perhaps reduced function, even if has more function when running on 1.5. 

Third, I know EMF went to great pains so that even though _they_ moved to 1.5, not to require all their clients to do so ... I forget details, but something like marked their bundles to produce 1.4 compliant code and restricted themselves so that there was some 1.5 things they could do, but not just any thing in 1.5. If that was no longer possible, it'd have a huge ripple effect. 
[As far as I know, this is still true ... that EMF does this "use 1.5, compile to 1.4) ... but I will add Ed and Kenn, in case something they changed could be related to that we are seeing]. 

But, we do this all over the place (have 1.4 bundles that pre-req EMF) so not sure why it is just this one bundle that's having trouble. 

Suggestions/clarifications welcome. but, if its a new limitation, even if should have always been a limitation, it will have a big ripple effect.

HTH
Comment 11 Olivier Thomann CLA 2011-10-07 08:42:52 EDT
As far as I can say, the type org.eclipse.xsd.XSDContentTypeCategory is an enum
type. So when trying to resolve its hierarchy, the type java.lang.Enum cannot
be found.
The reference is in
org.eclipse.wst.xsd.contentmodel.internal.XSDImpl.ElementDeclarationBaseImpl#getContentType().

I wonder how this actually worked before. David, could you please how you
expect this to compile using a 1.4 system library on the classpath of the
corresponding project ?

When using 1.5 but being able to run on top of 1.4, java.lang.Enum must never
be used. This is one of the limitation that has always been there. Now the
question remains why this compiles fine using 3.7.1. I'll investigate.
Comment 12 Olivier Thomann CLA 2011-10-07 09:28:03 EDT
It doesn't compile with 3.8, because the fix for bug 349326 checks if the local defined in:
XSDContentTypeCategory category = (XSDComplexTypeDefinition) td.getContentTypeCategory();
is an instance of AutoCloseable/Closeable and therefore it resolves its type hierarchy. Doing so, it tries to resolve java.lang.Enum and it failed.

This being said, I don't expect org.eclipse.xsd to contain 1.5 type if it has to be used in projects that require 1.4 only. If that type is used differently, even with 3.7.1, its type hierarchy might be resolved.

I would say that xsd should not use 1.5 types and it should be fixed on their side.
Comment 13 Olivier Thomann CLA 2011-10-07 10:37:49 EDT
I quickly checked org.eclipse.xsd and it is not set up to be compiled to target 1.4 (target platform=jsr14). This being said, I would say it is a mistake to refer to this bundle in a bundle that is supposed to use 1.4 only. I don't see how this would work at runtime.
Could you please clarify that point ?

So even if it looks like a regression in JDT, I believe this is actually catching a wrong setup in the org.eclipse.wst.xsd.core project or org.eclipse.xsd must be changed not to use any of the 1.5 types.

Did you try to run the code (compiled using 3.7.1) from this method org.eclipse.wst.xsd.contentmodel.internal.XSDImpl.ElementDeclarationBaseImpl#getContentType() on a 1.4 VM ?
Comment 14 Ed Merks CLA 2011-10-07 12:12:14 EDT
Yes, that doesn't make sense to me.  Of course folks can depend on an older version of EMF and XSD that doesn't require Java 5, but when you depend on a newer version that depends on Java 5.0 and compile against it, by transitivity you depend on Java 5.0.
Comment 15 Olivier Thomann CLA 2011-10-07 12:23:48 EDT
(In reply to comment #14)
> Yes, that doesn't make sense to me.  Of course folks can depend on an older
> version of EMF and XSD that doesn't require Java 5, but when you depend on a
> newer version that depends on Java 5.0 and compile against it, by transitivity
> you depend on Java 5.0.
Exactly my point. If a version of org.eclipse.xsd depends on 1.5 and it not compiled to target 1.4, then that version should never be used in a bundle that targets 1.4.
So org.eclipse.wst.xsd.core should either refer to a version of XSD that doesn't use 1.5, or it should be set to run on top of 1.5, or org.eclipse.xsd must provide a new version that runs on top of 1.4.
Right now I don't see anything wrong in JDT and I would move that bug back to wst.

David, what do you think ? Are any of these options applicable for you ?
Comment 16 David Williams CLA 2011-10-07 15:19:54 EDT
(In reply to comment #15)
> (In reply to comment #14)

> David, what do you think ? Are any of these options applicable for you ?

Could be ... but, that'd mean I've been under the wrong impression for a while. (no surprise there). A few comments and questions come to mind ... no judgments here ... just comments and questions. 

First and foremost, I _thought_ emf (and xsd) used Java 1.5 in their source but still "compiled to" 1.4. But, apparently not. I've always thought "compiling time is different than runtime" but ... apparently not so much. 

Second, I thought the BREE setting was supposed to be about your own specific bundle ... not all of your prereqs. If we have to take into account all pre-reqs, then the BREE is more like a "highest common denominator" rather than a minimum for one bundle alone. That sounds like a big change (mistake?) in my thinking.

Third, Keep in mind, we have no interest in running xsd.core on 1.4 but ... a) did not want to preclude others from doing so if they wanted, and their use of it was sufficient for their needs (they might not use the function that ultimately uses Enum). and b) I thought it was supposed to be architecturally more sound, since otherwise, if prereqs change, in future, the BREE might have to change, even though our code did not change. Seems odd ... but, again, probably just a mistaken impression on my part. 

Fourth, you say "... the local defined in ... [is checked if it ] is an instance of AutoCloseable/Closeable and therefore it resolves its type
hierarchy." Does his just impact only "VM Level" issues ... or does it mean that now, to guarantee compilation, someone needs a complete "target" even for other bundles their pre-reqs might need when they compile, even though their own code does not?  Sounds complicated. (A VM Level is one thing ... a whole, huge stack of bundles one doesn't care about is another thing). Again ... maybe that's always the way it "should have been" but seems like a change that would effect many people. 

Fourth, sounds really complicated if someone specifies a version range, such as 
Require-Bundle: org.eclipse.core.resources; bundle-version="3.1.0,4.0.0"
when "core resources" changed their BREE in there somewhere, from 1.4 to 1.5 ... so, now, to be exactly correct, if those wide ranges were correct, they could no longer compile against the version that has pre-req'd the higher BREE?
[I just picked core.resources for illustration ... similar for emf or xsd].  

So ... I was thinking of lots more complicated questions ... but ... I'm boring even myself ... so, I think time to "call in" people who might know better than I ... Tom and John ... and see if they can better describe "what the BREE should be" especially "how the BREE should be related to compile time" -- I have a feeling it is not "spec'd for compile time" :)  

I should emphasize ... we in WTP have no need for 1.4 ... if the advice is to change all our BREEs to 1.5 (simply to get "highest common denominator", because we use EMF and things like core.resources) then that's fine by me. We just opened this bug to give you "early warning" of a change in behavior ... and now has turned into "educating David" :)
Comment 17 David Williams CLA 2011-10-07 15:34:40 EDT
One quick suggestion ... you say, due to bug 349326 you check " ... the local defined in ... [is checked if it ] is an instance of AutoCloseable/Closeable and therefore it resolves its type hierarchy."

Would it be better to write your code so that 
 if <type hierachy can not be resolved>
 then assume "false" (not AutoCloseable/Closeable)

and continue as before? 

Not sure if that's still speaks to "correctness" of our bundle's set-up ... but, would maintain old behavior. 

But, guess the first question is the conceptual (or spec) question if a bundle's BREE must be set to its highest common denominator of pre-reqs ... or, lowest level, as determined only by itself.
Comment 18 Olivier Thomann CLA 2011-10-07 15:40:56 EDT
(In reply to comment #17)
> Would it be better to write your code so that 
>  if <type hierachy can not be resolved>
>  then assume "false" (not AutoCloseable/Closeable)
> 
> and continue as before? 
This is what I am investigating. I'll make more experiment with this today and talk to Stephan next week.
Comment 19 Olivier Thomann CLA 2011-10-07 17:17:59 EDT
I think there is room for improvement on the compiler side. I'll leave the bug here for now and we will investigate if we can fallback to the previous behavior in case one type in the hierarchy cannot be properly resolved.
Comment 20 David Williams CLA 2011-10-08 01:41:38 EDT
(In reply to comment #19)
> I think there is room for improvement on the compiler side. I'll leave the bug
> here for now and we will investigate if we can fallback to the previous
> behavior in case one type in the hierarchy cannot be properly resolved.

Thanks, for considering. The more I think about it, the more I think each bundle should only have to specify the VM that it, itself needs. What convinces me is the "OSGi Ideal" of being able to "mix and match" bundles. As long as we use API, there's no reason our "current bundle" could not be used with an "old" version of EMF running on 1.4 as long as it fits in version pre-req ranges. 

But, admit ... cases involving "Enum" are a little problematic. Guess that could be argued to be part of their (org.eclipse.xsd) API now and we should up our minimum pre-req. But still ... in general, should not be required to "up" compile level ... as far as I can tell ... just because the current version of a pre-req does. Thanks for the discussion/education.
Comment 21 Stephan Herrmann CLA 2011-10-08 08:55:55 EDT
Created attachment 204799 [details]
test & proposed fix

OK, here's an isolated test case and a small fix which we could apply to
make XSDImpl compile again against a 1.4 EE, if we want that.

Aside from the scenario in this bug that fix might lead also a direction
towards resolving bug 148844: with a similar approach we could also 
ignore binary methods that refer to missing types. I'll take a look at that.


The jar file that didn't make it into the patch is compiled from these
classes:

package p360164;

public class Provider {
	public MyEnum getE() {
		return MyEnum.TWO;
	}
}
----------
package p360164;

public enum MyEnum {
	ONE, TWO;
}
Comment 22 Stephan Herrmann CLA 2011-10-08 09:14:50 EDT
More thoughts on the conceptual side:

I would naively assume that bundles don't lower their required BREE during
evolution but only raise it. If that is a sound assumption, I'd suggest
the following strategy:

- *Build* your bundles against the lowest possible versions of all 
  dependencies. During this build use the lowest BREE that satisfies all.
  Only if this build succeeds will clients ever be able to use the
  low BREE.

- *Cross-check* your bundles against the highest supported versions of
  dependencies to see if any API changes break you bundle.
  For this compilation you may have to temporarily set a higher BREE.
  Only if this compilation succeeds will clients be able to use your bundle
  with newer versions of dependencies.

I don't think there's a way to get both guarantees in just one build.


One thing is special about BREE: you can't simply neglect your re-reqs
BREE and let every bundle pick its favored BREE, because unlike regular
OSGi bundles, you can only have one JRE at runtime. Thus every BREE
specification should be considered as a reexported dependency, no?
Comment 23 David Williams CLA 2011-10-08 10:29:09 EDT
(In reply to comment #22)
> More thoughts on the conceptual side:
> 

All good observations. 

I have heard of some groups that do this sort of  "multiple builds" to check all the assumptions inherent in OSGi range assumptions, the focus there was not on BREE levels .... but, seems that would automatically happen, at least using PDE build. See
http://dev.eclipse.org/mhonarc/lists/cross-project-issues-dev/msg06601.html
Seems like a good idea ... too bad most of us have a hard time just building once :) 

While a different issue than the bug here, for casual readers, I'd also like to document other known cases where the compile-time BREE has to be different than the true minimum runtime BREE ... this case had more to do with class libraries sometimes in JRE, sometimes provided externally ... and that there are ways to specify both a compile-time bree and a (lower) run-time bree. see http://wiki.eclipse.org/Execution_Environments#Compiling_against_more_than_is_required. 

I hate to "add to your work" but it would be nice to turn this into an optional "feature" for the JDT compiler ... allow a compiler directive such as "strictTypeResolution" that could be "featured" as doing a better job of checking assumptions ... but which might find issues users didn't care about so could be turned off (or ... turned into "warning" instead of "error") since I think many such violations would not often effect resulting byte codes ... just tell you some of your assumptions might not be quite right. (I'd suggest using the warning by default to better match current behavior). But, again, this was opened hoping to provide you with an early warning of a change in behavior ... so feel free to handle/track that deluxe solution separately if you'd like.
Comment 24 Thomas Watson CLA 2011-10-10 09:09:38 EDT
(In reply to comment #20)
> Thanks, for considering. The more I think about it, the more I think each
> bundle should only have to specify the VM that it, itself needs. What convinces
> me is the "OSGi Ideal" of being able to "mix and match" bundles. As long as we
> use API, there's no reason our "current bundle" could not be used with an "old"
> version of EMF running on 1.4 as long as it fits in version pre-req ranges. 

I agree with David, Each bundle should be responsible for determining what its own BREE is.  If a client bundle X has BREE J2SE-1.4 and requires some bundle Y at a range of [1.0,2.0) and bundle Y increases its BREE from J2SE-1.4 to J2SE-1.5 between bundle versions 1.1->1.2 then does that mean the binary bundle X is no longer valid when the new version of bundle y version 1.2 is installed?

> 
> But, admit ... cases involving "Enum" are a little problematic. Guess that
> could be argued to be part of their (org.eclipse.xsd) API now and we should up
> our minimum pre-req. But still ... in general, should not be required to "up"
> compile level ... as far as I can tell ... just because the current version of
> a pre-req does. Thanks for the discussion/education.

But if the client is not using Enum I don't see why it is required simply because the API of xsd now includes an Enum type.

(In reply to comment #22)
> 
> One thing is special about BREE: you can't simply neglect your re-reqs
> BREE and let every bundle pick its favored BREE, because unlike regular
> OSGi bundles, you can only have one JRE at runtime.

I disagree.  First of all, with singleton bundles you can force only one version to be resolved at a time.  Many eclipse bundles are singletons (I don't know about xsd), but this is besides the point.  Clients usually place a range on their constraints to disallow major version increases (e.g [1.0, 2.0)).  We have never thought of an increase in BREE to be considered a breaking change that requires a major version increase.  I hope we do not start thinking that it is a breaking change requiring a major version increase.  If increasing some bundle's BREE forces all bundles that transitively depend on it to also increase their BREE then to me that is a breaking change that would force a major version increase.  As David has stated this is going to be very disruptive.

> Thus every BREE specification should be considered as a reexported 
> dependency, no?

I don't think of it this way.  A bundle does not provide the classes from the VM.  Depending on some bundle that requires BREE JavaSE-1.6 from a bundle that has a BREE of j2SE-1.5 does not automatically grant the bundle access to all classes from the package javax.lang.model.
Comment 25 David Williams CLA 2011-10-10 20:29:21 EDT
>> But, admit ... cases involving "Enum" are a little problematic. ...
> 
> But if the client is not using Enum I don't see why it is required simply
> because the API of xsd now includes an Enum type.

Yes, forget what I said. I was thinking we were subclassing a type that declared itself as 'enum' but we are not subclassing it ... we simply "get" an instance of one, check a few things, and move on ... so, yes, whether we "get" the one from an old 1.4 based version or the newer 1.5 based version should not matter to us. (that is, to our byte codes, at compile time). 

So, it is nice that the compiler finds potential runtime problems for us :) ...  and a warning would be just fine if not great! ... but, it would not be right for the compiler to prevent us from keeping the 1.4 bree. 

(And, while we are talking about "bree's and jre levels here ... I am guessing that strict "runtime resolution checking" would effect assumptions about other pre-req libraries too ... which might be a more difficult issue to solve than merely changing BREE level.). 

Feel free to correct my thinking or descriptions if I've mis-characterized anything. Appreciate the help.
Comment 26 Stephan Herrmann CLA 2011-10-13 13:32:26 EDT
Mh, it seems my compiler POV made me jump at conclusions that don't fit OSGi.

So the request is: the compiler must be able to cope with references of a
type that cannot be fully resolved. Here: references of an enum type while
super-class Enum cannot be found.

IMHO this makes it indeed very similar to bug 148844 which requests to 
gracefully handle a type where some methods mention types in their signatures
that are not directly visible to the project being compiled.

The immediate issue in this bug arises when all of the resolve phase never
touched the missing super-class, and only during analyseCode the new check 
triggered the undesired error message.

I think we could release this patch, but it will not be a full answer to
the requirement (which has never been implemented in full by the compiler).
E.g., if you take a variable of the type that has now become an enum and try
to invoke toString() you should see the same error even with 3.7:

The type java.lang.Enum cannot be resolved. It is indirectly referenced from
required .class files 

This is caused by the broken link to the super-class Object.
Should the compiler gracefully handle this too?
To what degree *should* we then cope with missing classes?

When adjusting the compiler to these requirements will this still be the
correct semantics outside OSGi settings, or should we introduce an
explicit OSGi mode for any changes we do here?
Comment 27 Stephan Herrmann CLA 2011-10-13 13:39:10 EDT
Ayush, do you think the patch is safe for fixing the regression at hand?

I guess the general picture deserves investigation in another bug.
Comment 28 Stephan Herrmann CLA 2011-10-13 13:51:50 EDT
When investigating full independence of a pre-req's BREE we may have to
ponder contrived examples like this:
* a class A changes its super from B (in version 1) to C (in version 2)
  assume C is provided by J2SE-1.5
* both B and C define a method m
* a client depends on A version 1 and invokes m (which is provided by B)
* at compile time we unexpectedly find A version 2, but still use J2SE-1.4.
  So C cannot be resolved, hence we don't find m, even though in all 
  situations at runtime A will have m, either from B or from C.
What is the compiler expected to answer?

At compliance 1.3 the dependency on class B would be hardcoded in the byte 
code, but starting with 1.4 this could actually work at runtime, but how can
it be compiled with A version 2 (which needs C) and J2SE-1.4 (which doesn't
provide C)?
Comment 29 Olivier Thomann CLA 2011-10-13 14:52:33 EDT
I think this problem should be carefully assessed. It has to be clear that even if we release that patch, the problem is not completely solved. It will only be hidden.
Allowing a switch statement on an enum constant inside a project which is set to be 1.4 compliant looks wrong to me. Switching on enum constants is not allowed in 1.4. This violates the JLS.
Now we need to clearly explain what should be accepted and what should not be accepted.
If org.eclipse.xsd requires 1.5 types (like enum types) and is exposing enum types as API type, then I believe that consumers of that bundle should require 1.5 as well.
Accepting some usage of 1.5 code (compiled to target 1.4) inside 1.4 projects is one thing. When it is about accepting switching on enum constants, I believe this goes too far. This should not be accepted.
I agree that changing the BREE might not require a major version change, but switching to 1.5 and then adding a enum type as an API type makes it unusable the way it is used by wst.xsd code.
Referring to a generic type is different as it can be seen as a "raw" type and used as is. This can be handled. The case described here should not be handled.
Comment 30 Stephan Herrmann CLA 2011-10-13 16:43:02 EDT
(In reply to comment #29)
> Allowing a switch statement on an enum constant inside a project which is set
> to be 1.4 compliant looks wrong to me. Switching on enum constants is not
> allowed in 1.4. This violates the JLS.

The type XSDContentTypeCategory is both a Java5-enum and a pre-5 emulation
using ints. XSDImpl uses getValue() to perform an int-based switch.
This is why I created bug 360317 as a separate issue: XSDImpl isn't affected
by that question, but if we fix this bug the other bug will become relevant.

Sorry if the test case was confusing, it is not a 1:1 extract from XSDImpl.

> Now we need to clearly explain what should be accepted and what should not be
> accepted.

Indeed.
Comment 31 Srikanth Sankaran CLA 2011-10-14 02:25:45 EDT
(In reply to comment #29)

> Allowing a switch statement on an enum constant inside a project which is set
> to be 1.4 compliant looks wrong to me. Switching on enum constants is not
> allowed in 1.4. This violates the JLS.

Yes, agreed. Stephan, is the submitter's code dependent on this behavior ?
I am coming late to the party and haven't fully assimilated the fine
points of the long discussion, but I agree with the approach outlined here
in earlier comments and embodied in the patch posted, to wit that if the
connect supertype operation leads us into minefield, recognize that, wriggle
out of there, declare the type is not an autocloseable entity and move on.

> Referring to a generic type is different as it can be seen as a "raw" type and
> used as is. This can be handled. The case described here should not be handled.

Yes.

(In reply to comment #30)
> (In reply to comment #29)

[...]

> The type XSDContentTypeCategory is both a Java5-enum and a pre-5 emulation
> using ints. XSDImpl uses getValue() to perform an int-based switch.

This I take to read that the submitter's code is not expecting to switch
on the enum directly and expect things to work. 

> This is why I created bug 360317 as a separate issue: XSDImpl isn't affected
> by that question, but if we fix this bug the other bug will become relevant.

This looks like a plain oversight on the part of the compiler. There should
have a call to org.eclipse.jdt.internal.compiler.problem.ProblemReporter.incorrectSwitchType(Expression, TypeBinding) from the appropriate place when the switch expression
is an enum at level <= 1.4

> Sorry if the test case was confusing, it is not a 1:1 extract from XSDImpl.
> 
> > Now we need to clearly explain what should be accepted and what should not be
> > accepted.

I'll post a patch shortly that I have been playing with. Stephan, please
experiment with that and let me know if there are any more open issues that
I have failed to take note of.
Comment 32 Srikanth Sankaran CLA 2011-10-14 02:28:44 EDT
(In reply to comment #31)

> > This is why I created bug 360317 as a separate issue: XSDImpl isn't affected
> > by that question, but if we fix this bug the other bug will become relevant.

BTW, how did you stumble upon this scenario for the bug 360317 ? If it is
by thought experiment/imagination/extrapolation from code inspection, we
also need to ask ourselves what other operations on enums should have been
forbidden at 1.4 level under mixed mode set up and are not in practice.
Comment 33 Ed Merks CLA 2011-10-14 02:42:25 EDT
FYI

EMFs generated enums followed the type safe enum pattern. They currently and have always implemented this interface:

package org.eclipse.emf.common.util;


/**
 * An interface implemented by the enumerators of a type-safe enum.
 */
public interface Enumerator
{
  /**
   * Returns the name of the enumerator.
   * @return the name.
   */
  String getName();

  /**
   * Returns the <code>int</code>value of the enumerator.
   * @return the value.
   */
  int getValue();
  
  /**
   * Returns the literal value of the enumerator.
   * @return the literal.
   */
  String getLiteral();
}


Here's how they look in Java 5.0

public enum GenPropertyKind implements Enumerator
{
  EDITABLE_LITERAL(0, "Editable", "Editable"),
  READONLY_LITERAL(1, "Readonly", "Readonly"),
  NONE_LITERAL(2, "None", "None");
  public static final int EDITABLE = 0;
  public static final int READONLY = 1;
  public static final int NONE = 2;

  private static final GenPropertyKind[] VALUES_ARRAY =
    new GenPropertyKind[]
    {
      EDITABLE_LITERAL,
      READONLY_LITERAL,
      NONE_LITERAL,
    };

  public static final List<GenPropertyKind> VALUES = Collections.unmodifiableList(Arrays.asList(VALUES_ARRAY));

  public static GenPropertyKind get(String literal)
  {
    for (int i = 0; i < VALUES_ARRAY.length; ++i)
    {
      GenPropertyKind result = VALUES_ARRAY[i];
      if (result.toString().equals(literal))
      {
        return result;
      }
    }
    return null;
  }

  public static GenPropertyKind getByName(String name)
  {
    for (int i = 0; i < VALUES_ARRAY.length; ++i)
    {
      GenPropertyKind result = VALUES_ARRAY[i];
      if (result.getName().equals(name))
      {
        return result;
      }
    }
    return null;
  }

  public static GenPropertyKind get(int value)
  {
    switch (value)
    {
      case EDITABLE: return EDITABLE_LITERAL;
      case READONLY: return READONLY_LITERAL;
      case NONE: return NONE_LITERAL;
    }
    return null;
  }

  private final int value;

  private final String name;

  private final String literal;

  private GenPropertyKind(int value, String name, String literal)
  {
    this.value = value;
    this.name = name;
    this.literal = literal;
  }

  public int getValue()
  {
    return value;
  }

  public String getName()
  {
    return name;
  }

  public String getLiteral()
  {
    return literal;
  }

  public String toString()
  {
    return literal;
  }
}


Here's how they looked before

public final class GenPropertyKind extends AbstractEnumerator
{
  public static final int EDITABLE = 0;

  public static final int READONLY = 1;

  public static final int NONE = 2;

  public static final GenPropertyKind EDITABLE_LITERAL = new GenPropertyKind(EDITABLE, "Editable", "Editable");

  public static final GenPropertyKind READONLY_LITERAL = new GenPropertyKind(READONLY, "Readonly", "Readonly");

  public static final GenPropertyKind NONE_LITERAL = new GenPropertyKind(NONE, "None", "None");

  private static final GenPropertyKind[] VALUES_ARRAY =
    new GenPropertyKind[]
    {
      EDITABLE_LITERAL,
      READONLY_LITERAL,
      NONE_LITERAL,
    };

  public static final List VALUES = Collections.unmodifiableList(Arrays.asList(VALUES_ARRAY));

  public static GenPropertyKind get(String literal)
  {
    for (int i = 0; i < VALUES_ARRAY.length; ++i)
    {
      GenPropertyKind result = VALUES_ARRAY[i];
      if (result.toString().equals(literal))
      {
        return result;
      }
    }
    return null;
  }

  public static GenPropertyKind getByName(String name)
  {
    for (int i = 0; i < VALUES_ARRAY.length; ++i)
    {
      GenPropertyKind result = VALUES_ARRAY[i];
      if (result.getName().equals(name))
      {
        return result;
      }
    }
    return null;
  }

  public static GenPropertyKind get(int value)
  {
    switch (value)
    {
      case EDITABLE: return EDITABLE_LITERAL;
      case READONLY: return READONLY_LITERAL;
      case NONE: return NONE_LITERAL;
    }
    return null;	
  }


  private GenPropertyKind(int value, String name, String literal)
  {
    super(value, name, literal);
  }

} //GenPropertyKind
Comment 34 Srikanth Sankaran CLA 2011-10-14 03:53:48 EDT
Created attachment 205181 [details]
Plausible fix

Plausible cumulative patch for bug 360164 and bug 360317.

Passes all tests. Stephan, let me know if there are still open issues
or we have managed to push the problem under a suitably large rug :)
Comment 35 Srikanth Sankaran CLA 2011-10-14 03:56:00 EDT
(In reply to comment #34)
> Created attachment 205181 [details]
> Plausible fix
> 
> Plausible cumulative patch for bug 360164 and bug 360317.
> 
> Passes all tests. Stephan, let me know if there are still open issues
> or we have managed to push the problem under a suitably large rug :)

Patch is missing test BTW.
Comment 36 Ayushman Jain CLA 2011-10-14 06:08:34 EDT
(In reply to comment #34)
> Created attachment 205181 [details] [diff]
> Plausible fix

A minor observation. The other switch related warnings read "cannot switch on....Only convertible int values....are permitted".
So maybe the latter "Only convertible int values are permitted" can be added to the new error msg as well, for the sake of uniformity.
Comment 37 Stephan Herrmann CLA 2011-10-18 10:29:57 EDT
Created attachment 205424 [details]
tests & combined fix v3

This patch consolidates the previous patches with
- dedicated tests for legal/illegal use of the enum type,
- better wording for the new diagnostic.
To be released shortly.
Comment 38 Stephan Herrmann CLA 2011-10-18 10:36:52 EDT
Released as commit 2d89f0516f5e5910bcd18015e8090ed0805dbb4e for 3.8 M3.
Comment 39 Srikanth Sankaran CLA 2011-10-18 11:21:13 EDT
Just so as not to leave the review flag dangling, patch has been reviewed by
both Ayush and Srikanth, so updating with +1, Thanks for the fix Stephan.
Comment 40 Srikanth Sankaran CLA 2011-10-20 04:25:17 EDT
Created attachment 205590 [details]
Additional fixes & tests

Olivier pointed out another scenario that continues to fail even after
the resolution of the bug.

Turns out https://bugs.eclipse.org/bugs/show_bug.cgi?id=349326 introduced
supertype walks in three places of which we built an escape hatch only
for one. This additional patch takes care of the other two places.

Thanks to Olivier and to Ayush for putting together a junit.

Tests are running. I'll update after they finish.
Comment 41 Srikanth Sankaran CLA 2011-10-20 06:12:02 EDT
(In reply to comment #40)

> Tests are running. I'll update after they finish.

Ayush, all tests pass, Can you please release the patch as I am having some
issues with git. Stephan has reviewed the patch already. TIA.
Comment 42 Ayushman Jain CLA 2011-10-20 08:41:26 EDT
Released last patch in HEAD for 3.8M3 via commit 7cc08fd6458164ad4911bfd35d3157c5667751d0
Comment 43 Srikanth Sankaran CLA 2011-10-24 04:33:05 EDT
Verified for 3.8 M3 using build id: N20111022-2000. Olivier also
independently ran into this problem on his set of sources and
could verify that this issue is fixed by the patch in comment# 40.