Bugzilla – Full Text Bug Listing
| Summary: | [1.8][compiler] EclipseCompiler should not hard code compliance level | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Jay Arthanareeswaran <jarthana> | ||||||||||
| Component: | Core | Assignee: | Jay Arthanareeswaran <jarthana> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | anchakrk, markus.kell.r, Olivier_Thomann, sasikanth.bharadwaj, srikanth_sankaran, stephan.herrmann | ||||||||||
| Version: | 4.3 | ||||||||||||
| Target Milestone: | BETA J9 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows 7 | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 414627 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Jay Arthanareeswaran
I also see this EclipseCompiler.java. Looks like this needs to be updated too. But wasn't touched for Java 7. // Eclipse compiler supports all possible versions from version 0 to // version 6 // we don't care about the order EnumSet<SourceVersion> enumSet = EnumSet.range(SourceVersion.RELEASE_0, SourceVersion.RELEASE_6); Olivier, can you throw some light on this, esp. the piece of code I shared in comment #0 ? Is it okay to set the compliance and source level to 1.8? (In reply to comment #0) > We have the following code in EclipseCompiler.java, which is our > implementation of JavaCompiler: > > eclipseCompiler2.options.put(CompilerOptions.OPTION_Compliance, > CompilerOptions.VERSION_1_6); > eclipseCompiler2.options.put(CompilerOptions.OPTION_Source, > CompilerOptions.VERSION_1_6); > eclipseCompiler2.options.put(CompilerOptions.OPTION_TargetPlatform, > CompilerOptions.VERSION_1_6); > > There has to be a way clients, at least, JDT core tests that use this, can > pass a different compliance level than this. Via org.eclipse.jdt.internal.compiler.tool.EclipseCompiler.run(InputStream, OutputStream, OutputStream, String...) ? If I remember well, this is related to the fact that this compiler had to be able to consume 1.6 annotation processors. As is it had to be at least 1.6 compliant. Any value above 1.6 should work. (In reply to comment #3) > If I remember well, this is related to the fact that this compiler had to be > able to consume 1.6 annotation processors. As is it had to be at least 1.6 > compliant. Any value above 1.6 should work. I agree. Jay, we can fix this beginning Java 8 - since it is not material to Java 7 Jay, Anirban is looking for compiler items, assign this to him ? Anirban, please take this forward. This should take only couple of hours including testing. Let me know if you need any information on testing the fix. As Anirban just found out, SourceVersion.RELEASE_8 is available only in JDK 8 and to access the same, we need JDK 8 in the build path. I am not sure if there's another way. We should not hardcode again: See javax.lang.model.SourceVersion.latest() and javax.lang.model.SourceVersion.latestSupported() Created attachment 235027 [details]
Testcase
Patch that can be used as a test case. When this patch is applied on BETA_JAVA and org.eclipse.jdt.compiler.apt.tests.AllTests is run with b100, there should be just one failure. But now you would see an additional failure, which should go away when the fix is in place.
(In reply to comment #9) > Created attachment 235027 [details] > Testcase > > Patch that can be used as a test case. When this patch is applied on > BETA_JAVA and org.eclipse.jdt.compiler.apt.tests.AllTests is run with b100, > there should be just one failure. But now you would see an additional > failure, which should go away when the fix is in place. Forgot to mention this: Whenever you make changes to the test files or the test APT processors in compiler.apt.tests, you must recreate a certain jar by running compiler.apt.tests\apttestprocessors.jardesc. Created attachment 235055 [details]
Patch for the fix
Hello,
Jay uploaded the testcase in the previous patch.
This patch represents the code change.
Thanks
Anirban
Created attachment 235063 [details]
Patch for the fix
Hello,
Patch with dynamic assignment of latest supported source version.
Thanks
Anirban
(In reply to ANIRBAN CHAKRABORTY from comment #12) > Created attachment 235063 [details] > Patch for the fix Anirban, since you are not hard-coding the latest supported version, I think the move from JavaSE1.6 to JavaSE1.8 to the compiler.tool project is not necessary. Can you please remove that classpath change from the patch? Also please include the test case in the same patch. I thought I added this comment earlier, but it seems to be missing: Please update the comment: // Eclipse compiler supports all possible versions from version 0 to // version 6 // we don't care about the order that exists one line before the place which is changed by the patch, TIA. Created attachment 235124 [details]
Patch accommodating review comments.
Patch accommodating review comments.
Pushed to BETA_JAVA8: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=b40a58ab0876d24632a275ef1be808b26bd863a5 This change broke org.eclipse.jdt.compiler.tool.tests.AllTests. You also have to update the bundle version in MANIFEST.MF and pom.xml to 1.0.300 (for both the production and the test bundle), and you should add the 1.8/8/8.0 to the static initializer in org.eclipse.jdt.internal.compiler.tool.Options (In reply to Srikanth Sankaran from comment #8) > We should not hardcode again: See javax.lang.model.SourceVersion.latest() > and javax.lang.model.SourceVersion.latestSupported() I don't think that change was correct. Doesn't that mean a org.eclipse.jdt.compiler.tool.jar compiled today will include 1.9 when it is run with a 1.9 EE? And it would say it's not able to compile 1.8 source code if it's running on a 1.7 EE. The org.eclipse.jdt.compiler.tool bundle should stay at BREE 1.6, and the EclipseCompiler should use other means to find the latest supported releases > 6. The EnumSet.range(..) is also a bit scary, since it assumes the order of enum constants is immutable. JLS7 13.4.26 explicitly allows reordering of constants, so I wouldn't say it's safe to rely on the declaration order. We should use explicit references to SourceVersion.RELEASE_0-6 and use SourceVersion.valueOf("RELEASE_7") etc. for later versions (catch the IAE if we're running on a 1.6 EE). (In reply to Markus Keller from comment #17) > The EnumSet.range(..) is also a bit scary, since it assumes the order > of enum constants is immutable. JLS7 13.4.26 explicitly allows reordering of > constants, so I wouldn't say it's safe to rely on the declaration order. The EnumSet's javadoc has this: "The iterator returned by the iterator method traverses the elements in their natural order (the order in which the enum constants are declared). " This combined with the unmodifiable collection should be good enough, isn't it? Looks like the following code has problem too:
eclipseCompiler2.options.put(CompilerOptions.OPTION_Compliance, CompilerOptions.VERSION_1_8);
We need something like this:
String compilerVersion = null;
try {
SourceVersion.valueOf("RELEASE_8"); //$NON-NLS-1$
compilerVersion = CompilerOptions.VERSION_1_8;
} catch (IllegalArgumentException iae) {
compilerVersion = CompilerOptions.VERSION_1_6;
}
eclipseCompiler2.options.put(CompilerOptions.OPTION_Compliance, compilerVersion);
....
But even that's not going to be enough because some of the tests are hard-coding the JDK level, like this:
assertEquals("Wrong value", ClassFileConstants.JDK1_6, reader.getVersion());
(In reply to Srikanth Sankaran from comment #2) > > There has to be a way clients, at least, JDT core tests that use this, can > > pass a different compliance level than this. > > Via org.eclipse.jdt.internal.compiler.tool.EclipseCompiler.run(InputStream, > OutputStream, OutputStream, String...) ? Looks like the tests are not passing the compliance level at all and relying on the EclipseCompiler to set it to 1_6. And as I mentioned in comment #19, the always expect the code to be compiled at 1.6 :( I have reverted the changes made by commit b40a58ab0876d24632a275ef1be808b26bd863a5 to avoid the failures. I figured we can force the compliance level from the tests directly (even though we are not doing it at the moment and relying on EclipseCompiler to hard-code 1.6). We will run into this problem pretty soon when we have to add annotation processors with 1.8 support. (In reply to Jayaprakash Arthanareeswaran from comment #18) If you assume that the order of declarations in the SourceVersion enum won't change, then everything is safe. But my point is that nothing prevents the maintainers of that class from changing the declaration order (neither a Javadoc contract nor binary compatibility rules). Therefore, EnumSet.range(..) is unsafe, unless the order of declarations is guaranteed to be stable. Like the Oracle Java 7 JRE could "legally" change the order in Class#getDeclaredMethods(), this could also apply here (without breaking any rules). Or they could add RELEASE_7_JIGSAW between RELEASE_7 and RELEASE_8, and if we use a range, we would wrongly include it. Jay, what do you recommend as the way forward ? Earlier, I thought there is no way for our tests to override the 1.6 level that is set as default. But I figured there is a way the tests can override this by just adding "-1.8" to the options before invoking the compiler. So, I guess we don't need do anything here, at least for the time being. It is about time we reopen and fix this. Looks like Javac has moved ahead with the default compliance to the latest but we (ECJ through JSR 199) are still stuck at 1.6. Resolved via: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=6af4a0a291a362228d60748995c15e8fd3464cbc At the moment, the latest supported is still at 1.8, as we have no reliable way of supporting the constant 9 without moving ourselves to compliance 9. But the hard coding of the latest compliance level has been fixed along with the affected tests. |