Community
Participate
Working Groups
Build Identifier: Build id: I20120608-1400 The following code works in java 1.6 and Eclipse 3.7, but fails to compile in Eclipse 4.2 with compiler level set to 1.6 and autoboxing to error: The method test(int[]) is ambiguous for the type VarargPrimitiveTest import java.util.Arrays; public class VarargPrimitiveTest { public static void test(int... a) { System.out.println(Arrays.toString(a)); } public static <T> void test(Object... a) { System.out.println(Arrays.toString(a)); } public static void main(String[] args) { test(1); } } Reproducible: Always
See that this is a bug in JDK5 and JDK6 which has since been corrected in JDK7 and JDK8 (8b39). The observed behavior is the correct behavior and the code needs to be adjusted suitably. *** This bug has been marked as a duplicate of bug 346038 ***
> See that this is a bug in JDK5 and JDK6 which has since been corrected > in JDK7 and JDK8 (8b39). > The observed behavior is the correct behavior and the code needs to be > adjusted suitably While rejecting this code may be correct behaviour for JDK7, rejecting it in 1.6 compiler mode causes a maintenance hell: There is a lot of code out there in a lot of revisions that depends on this behaviour of overloading primitive and object varargs method. The same is true for annotation not seeing value arrays anymore. The quick fix even generates duplicated value() methods. It is reasonable to fix the affected code in recent versions targeted at Java 7. But not being able to use Eclipse 4.2 to fix bugs in old code, is a major drawback. When Java introduced new keywords (assert, enum), Eclipse did not require old code to be changed. Yes, I just checked, that javac 7 does reject primitive/object overloading in varargs even in 1.6 mode. This is no real problem because a JDK6 is required anyway for the classpath. But having to keep two versions of Eclipse around is a great hassle. This is not a duplicate of bug 346038 which requested consistent behaviour with Java 7. This is about breaking existing Java 6 code in compiler level 1.6.
(In reply to comment #2) > > See that this is a bug in JDK5 and JDK6 which has since been corrected > > in JDK7 and JDK8 (8b39). > > The observed behavior is the correct behavior and the code needs to be > > adjusted suitably > > While rejecting this code may be correct behaviour for JDK7, rejecting it in > 1.6 compiler mode causes a maintenance hell: There is a lot of code out there > in a lot of revisions that depends on this behaviour of overloading primitive > and object varargs method. Déjà vu all over again. I thought we had decided to make these future errors warnings, but my fault. Ayush, do you have the time to work on this - converting this into a warning for 1.6- modes ? We should not silently accept this code in 1.6 modes. > The same is true for annotation not seeing value arrays anymore. The quick fix > even generates duplicated value() methods. Please open a separate bug with a test case. TIA.
I don't believe the compliance level setting in JDT was ever intended to be used to request a mode where ECJ will behave in a bug compatible manner relative to javac: Instead it was supposed to be used in situations where the spec (not the reference implementation) got clarified (got changed) across Java SE releases. That said, we are having trouble convincing users that this stand is neither pedantic nor rigid. Build breakages are bad and on occasions in the past it has led to shrill protests even though ECJ's behavior was in the right. JDT/Core is planning to adopt a policy that "Unless it is a blatant/glaring bug that clearly violates the spec", - "Relaxing behavior changes" (i.e where code was originally not compiling with a certain version of reference compiler and now compiles with a reference compiler of the subsequent release stream) will be implemented across compliance levels and that - "Tightening behavior changes" (i.e where code was originally compiling with a certain version of reference compiler and now does NOT compile with a reference compiler of the subsequent release stream) will be implemented across matching compliance levels and that this will be warned against in earlier compliance levels. Thoughts/objections ?
(In reply to comment #4) > Thoughts/objections ? I agree that this bug needs to be fixed. Relaxing can be confusing. Assume the user has his code without errors in Eclipse and then uses 'javac' (e.g. via Ant script) to do the final build. He will have a very hard time to understand why 'javac' fails. We had similar issues with Javadoc checking inside Eclipse where we were smart and fixed real problems for/in all versions, but in the end we ended up with Javadoc errors in our official build. The only thing that really worked was to mimic even the bugs in 'javadoc'. "Tightening behavior change" If I understood your suggestion correctly, you don't plan to change the behavior but instead still accept the code and issue a warning. This is fine as long as there is an option to change this to 'Ignore' (or 'Error').
(In reply to comment #5) > "Tightening behavior change" > If I understood your suggestion correctly, you don't plan to change the > behavior but instead still accept the code and issue a warning. This is fine as > long as there is an option to change this to 'Ignore' (or 'Error'). Dani, As of today there are a couple of problems such as "Method ... has the same erasure ... as another method in type .." and "Duplicate method" that are only warnings in 1.6 but errors in 1.7. These, along with the problem for this bug report can be controlled by an option right at the Java compiler compliance preference page. Something like Warn on code that does not compile with higher compliance settings [enabled/disabled] This will be enabled by default. Is this acceptable?
(In reply to comment #6) > Dani, As of today there are a couple of problems such as "Method ... has the > same erasure ... as another method in type .." and "Duplicate method" that are > only warnings in 1.6 but errors in 1.7. (For specific code patterns such as that reported here, I meant)
(In reply to comment #6) > (In reply to comment #5) > > "Tightening behavior change" > > If I understood your suggestion correctly, you don't plan to change the > > behavior but instead still accept the code and issue a warning. This is fine as > > long as there is an option to change this to 'Ignore' (or 'Error'). > > Dani, As of today there are a couple of problems such as "Method ... has the > same erasure ... as another method in type .." and "Duplicate method" that are > only warnings in 1.6 but errors in 1.7. These, along with the problem for this > bug report can be controlled by an option right at the Java compiler compliance > preference page. Something like > > Warn on code that does not compile with higher compliance settings > [enabled/disabled] I was thinking about this and it looks to me that things could quickly get out of hand - There are places where the distance between place where we discriminate and behave differently and the place where we report the the error report could be quite vast and at the reporting site we would have lost context that we should warn. The only maintainable solution looks to be just implement the new behavior in the new modes and let that become an error/warning in the new modes with the other compliance levels behaving unchanged. Yes, we made it a warning in older compliance levels in the two cases you cite, but I don't think we can generalize that.
(In reply to comment #8) > The only maintainable solution looks to be just implement the new behavior > in the new modes and let that become an error/warning in the new modes with > the other compliance levels behaving unchanged. What do you mean with "new modes"?
(In reply to comment #6) > Warn on code that does not compile with higher compliance settings > [enabled/disabled] > > This will be enabled by default. Is this acceptable? I would use a more neutral wording: "Tolerate code that compiled with a previous version of the compiler". This would be 'false' by default i.e. in our case/bug we can issue an error like we currently do. This is important to be aligned with 'javac' which also issues the error. In other/future cases we might issue a warning (depending on what 'javac' does by default.
(In reply to comment #9) > (In reply to comment #8) > > The only maintainable solution looks to be just implement the new behavior > > in the new modes and let that become an error/warning in the new modes with > > the other compliance levels behaving unchanged. > > What do you mean with "new modes"? If we are reacting to javac changing its behavior as of JDK version x, the suggestion is not to change existing behavior for x-1, x-2 ... but to change only for x, x+1 ... The other proposal that is being considered is to make it a warning in x-1, x-2, x-3 etc - but I am not sure this is workable in the cases where the error reporting is far removed from the place where the behavior change is being made.
Out of the box we should report an *error*. Only when the user changes some option, it will either be ignored (I'm fine with that) or reported as warning.
(In reply to comment #12) > Out of the box we should report an *error*. Only when the user changes some > option, it will either be ignored (I'm fine with that) or reported as > warning. Alternatively, the user can choose to use the "broken" compiler from 3.7 or use javac <= 1.7.
(In reply to comment #8) > Yes, we made it a warning in older compliance levels in the two cases > you cite, but I don't think we can generalize that. Yeah, i realized that after writing this comment. > (In reply to comment #6) > "Tolerate code that compiled with a previous version of the compiler". > > This would be 'false' by default i.e. in our case/bug we can issue an error > like we currently do. This is important to be aligned with 'javac' which also > issues the error. In other/future cases we might issue a warning (depending on > what 'javac' does by default. I think we can use this setting to tolerate such cases for <1.7, however we will do so without giving a warning. We can include the cases mentioned in comment 6 to be also controlled by this setting, even though in those cases we are able to give a warning.
(In reply to comment #12) > Out of the box we should report an *error*. Only when the user changes some > option, it will either be ignored (I'm fine with that) or reported as warning. (In reply to comment #14) > I think we can use this setting to tolerate such cases for <1.7, however we > will do so without giving a warning. We can include the cases mentioned in > comment 6 to be also controlled by this setting, even though in those cases we > are able to give a warning. Let us make haste slowly: We can choose between alternate behaviors using a source level check. Not sure that we can choose between error/warning/ignore using source level check because behavior change and error reporting could be arbitrarily far removed: i.e ambiguous method invocation is a general error situation, and may arise in situations nothing to do with primitive types and/or varargs. We can turn the error into warning in the general situation, only when we encounter the scenario of the current problem. But at the reporting time, we are stripped of some context and it could be impossible to discern the scenario.
(In reply to comment #15) > We can turn the error into warning in the general situation, only when Sorry, should read: We CANNOT turn the error into warning in the ... > we encounter the scenario of the current problem. But at the reporting time, > we are stripped of some context and it could be impossible to discern > the scenario.
(In reply to comment #15) > We can choose between alternate behaviors using a source level check. That would be wrong because it's not a question of source compatibility: javac 1.7 also reports an error if source is set to 1.6. We can interpret "compliance" as corresponding to the javac version (as long as the defaults aren't changed). In this case we can add the already mentioned BOOLEAN option: "Tolerate code that compiled with a previous version of the compiler". All it would do is restore the wrong behavior. If you are saying you can detect and do this, then we can close this bug as WONTFIX. I would also not spend too much time to dig out old cases and bind them with this new option.
(In reply to comment #17) > If you are saying you can detect and do this, then we can close this bug as > WONTFIX. It should read: "you *can't* detect"
(In reply to comment #17) > (In reply to comment #15) > > We can choose between alternate behaviors using a source level check. > > That would be wrong because it's not a question of source compatibility: javac > 1.7 also reports an error if source is set to 1.6. We can interpret > "compliance" as corresponding to the javac version Sorry, compliance is what I meant., NOT source level. It is a case of fingers typing something the brain didn't intend. (In reply to comment #17) > All it would do is restore the wrong behavior. If you are saying you can detect > and do this, then we can close this bug as WONTFIX. I think you meant "If you are saying you cannot detect" ... Perhaps your fingers are typing something your brain didn't intend :) > I would also not spend too much time to dig out old cases and bind them with > this new option. Agreed. Returning to the issue, error reporting happens happens at the narrow end of the funnel. We can reach the narrow end by taking different courses at the wider mouth side of the funnel. Unless we are extremely careful, we would end up ignoring or turning into warnings errors which we absolutely should not. Ayush, let me know if your investigation shows otherwise.
(In reply to comment #19) > I think you meant "If you are saying you cannot detect" ... See comment 18. > Perhaps your fingers are typing something your brain didn't intend :) Yes, they sometimes do ;-)
(In reply to comment #19) > Returning to the issue, error reporting happens happens at the narrow end > of the funnel. We can reach the narrow end by taking different courses > at the wider mouth side of the funnel. Unless we are extremely careful, > we would end up ignoring or turning into warnings errors which we absolutely > should not. Ok I did some archeology and found that the 'tightening' observed in this bug is actually due to the 'intended relaxing' of similar varargs constructs in bug 346038. That basically means that in this case, the behaviour change has ripple effects starting at the wide end of the funnel, and any compliance specific change put there will also reverse the effect of bug 346038 for compliances < 1.7 For cases mentioned in comment 6, we were lucky to find and report the problem at the same place, but not so much in the current case. Should be WONTFIX for this. Even if we can somehow find a way in this case, there will be a bunch of others where we cant do anything, so the new setting will be a bad precedent and raise user's expectations for us to be bug compatible with javac. I do understand though that this is inconvenient for users. :(
(In reply to comment #21) > Should be WONTFIX for this. Even if we can somehow find a way in this case, > there will be a bunch of others where we cant do anything, so the new setting > will be a bad precedent and raise user's expectations for us to be bug > compatible with javac. I do understand though that this is inconvenient for > users. :( How about this compromise: we document this in the readme and add a system property where the old behavior can be restored for this particular case. Checking this at the right location(s) should be easy.
(In reply to comment #22) > How about this compromise: we document this in the readme and add a system > property where the old behavior can be restored for this particular case. > Checking this at the right location(s) should be easy. Sounds good to me. I would also not rewire old fixes - we will use this system property for the current bug and future ones only.
(In reply to comment #23) > (In reply to comment #22) > > > How about this compromise: we document this in the readme and add a system > > property where the old behavior can be restored for this particular case. > > Checking this at the right location(s) should be easy. > > Sounds good to me. I would also not rewire old fixes - we will use this system > property for the current bug and future ones only. I would use the property just for this single instance to make it clear. We can add new properties in the future for different cases, if really needed.
Created attachment 218230 [details] proposed fix v1.0 + regression tests This patch introduces a new setting org.eclipse.jdt.core.compiler.problem.tolerateEarlierCompiledVarargsCode = [enabled/disabled] When turned on in compliances 1.5 and 1.6, it reverses the effect of fix for bug 346038 and bug 346039. Thus, it lets the code in comment 0 to be compiled in those compliances, and the code in the 2 bugs mentioned to raise errors, thus restoring the compiler behaviour pre-Juno for these kind of varargs cases. For compliance >= 1.7, this setting does not have any effect. A batch compiler option -warn:tolerateVarargsCode is introduced as well.
(In reply to comment #25) > Created attachment 218230 [details] > proposed fix v1.0 + regression tests > > This patch introduces a new setting > org.eclipse.jdt.core.compiler.problem.tolerateEarlierCompiledVarargsCode = > [enabled/disabled] > I think the recommendation is to use a system property (environment variable) and not a full blown option. That should also cut down on the boiler plate code a whole lot.
Created attachment 218240 [details] proposed fix v2.0 + regression tests This patch uses a system property instead of a setting. This can be set in the eclipse.ini file as -vmargs -DtolerateVarargsCodeThatCompiledEarlier=true
Dani, where should this option be documented? In the incompatibilities.htm in the isv doc?
Srikanth, can you review for 3.8.1 inclusion? TIA!
(In reply to comment #28) > Dani, where should this option be documented? In the incompatibilities.htm in > the isv doc? I would add it to the migration FAQ since it is not really an incompatibility.
I find the current bug summary confusing: Eclipse 4.2 fails to compile valid Java 6 code in Java 6 compatibility mode) Is the following correct? [compiler] Eclipse 4.2 creates ambiguous varargs invocation error in 1.6 compliance mode (does not mimic JDK bug) I would also change "tolerateVarargsCodeThatCompiledEarlier" to "tolerateIllegalAmbiguousVarargsInvocation" to clarify the situation and to make it clear that this is a hack (also for someone who finds this setting in a build script, etc.).
(In reply to comment #31) > I would also change "tolerateVarargsCodeThatCompiledEarlier" to > "tolerateIllegalAmbiguousVarargsInvocation" to clarify the situation and to > make it clear that this is a hack (also for someone who finds this setting in a > build script, etc.). This suggestion looks good. I'll study the patch in a bit more detail, but at the outset a couple of comments: - Triggers an NLS tag warning, should be fixed. - We should hoist the system property consultation into a class initializer block, rather than doing it in multiple places. The compiler option check can stay where it is.
Created attachment 218319 [details] proposed fix v2.1 + regression tests CHanged the name as Markus suggested and moved the setting of the property to CompilerOptions, so that a boolean flag is set in the options map that can be consulted from anywhere else later.
Created attachment 218320 [details] patch for migration FAQ This needs to go only in R3_8_maintenance
Some more comments: (1) Are these 5 lines if (tolerateIllegalAmbiguousVarargs != null && tolerateIllegalAmbiguousVarargs.equalsIgnoreCase("true")) { //$NON-NLS-1$ this.tolerateIllegalAmbiguousVarargsInvocation = true; } else { this.tolerateIllegalAmbiguousVarargsInvocation = false; } written better as a single line: this.tolerateIllegalAmbiguousVarargsInvocation = tolerateIllegalAmbiguousVarargs != null && tolerateIllegalAmbiguousVarargs.equalsIgnoreCase("true"); (2) Is this: CompilerOptions options = this.compilerOptions(); if (options.complianceLevel < ClassFileConstants.JDK1_7 && options.tolerateIllegalAmbiguousVarargsInvocation) tiebreakingVarargsMethods = false; better written as: if (tiebreakingVarargsMethods) { CompilerOptions options = this.compilerOptions(); if (options.complianceLevel < ClassFileConstants.JDK1_7 && options.tolerateIllegalAmbiguousVarargsInvocation) tiebreakingVarargsMethods = false; } The common case scenario is for tiebreakingVarargsMethods to already be false. (3) I would swap the checks in: options.complianceLevel < ClassFileConstants.JDK1_7 && options.tolerateIllegalAmbiguousVarargsInvocation However, these are micro-issues that can be left in unless we are making changes on account of (4) below: (4) This patch does not seem to roll back the entire effect of the commit 85f48e0f08275e1f81e9995073d5c4f69bfd0707. Is this intentional ? I haven't determined what is the effect of the behavior left in yet: (a) The original fix seems to have deleted this code block: if (oneParamsLength > twoParamsLength) { // special case when autoboxing makes (int, int...) better than (Object...) but not (int...) or (Integer, int...) if (((ArrayBinding) twoParams[twoParamsLength - 1]).elementsType().id != TypeIds.T_JavaLangObject) return false; } Is the behaviour deleted something that would have to be tolerated under the system property ? What is the effect of it staying deleted and not resurrected ? (b) Calls to org.eclipse.jdt.internal.compiler.lookup.Scope.parameterCompatibilityLevel(MethodBinding, TypeBinding[], boolean) from the tail end of org.eclipse.jdt.internal.compiler.lookup.Scope.isAcceptableMethod(MethodBinding, MethodBinding) continue to pass true for the tiebreak parameter and that results in different side effects inside that method - can these be all left in and what is the effect of leaving it in ?
(0) tolerateIllegalAmbiguousVarargsInvocation doesn't have to be an instance variable at all and can be initialized in a static block. That would allow us to check it as CompilerOptions.tolerateIllegalAmbiguousVarargsInvocation eliminating the overhead walking the scope stack all the way to top to retrieve options.
*** Bug 384562 has been marked as a duplicate of this bug. ***
Created attachment 218798 [details] proposed fix v2.2 + regression tests Yup, (4) and (5) were indeed unintended omissions. Fixed both and added tests test070b_tolerate and test071_tolerate respectively. Also fixed all other points. I hadn't bothered about (0) earlier because we were anyway looking up the compiler options to get the compliance so I didn't see any incremental advantage in directly referencing the new option.
*** Bug 386361 has been marked as a duplicate of this bug. ***
*** Bug 382469 has been marked as a duplicate of this bug. ***
Sorry for the delay in reviewing this. I expect to complete this well in time for RC1.
Created attachment 219412 [details] proposed fix v2.2 + more regression tests Added tests from duplicate bugs
(In reply to comment #38) > other points. I hadn't bothered about (0) earlier because we were anyway > looking up the compiler options to get the compliance so I didn't see any > incremental advantage in directly referencing the new option. Nevertheless, it does not make sense to initialize this variable over and over. The initializer block where this is assigned is better tagged as a static block. Sorry for the delay, patch looks good. Please release for 4.3 M1.
(In reply to comment #43) > (In reply to comment #38) > > > other points. I hadn't bothered about (0) earlier because we were anyway > > looking up the compiler options to get the compliance so I didn't see any > > incremental advantage in directly referencing the new option. > > Nevertheless, it does not make sense to initialize this variable over > and over. The initializer block where this is assigned is better tagged > as a static block. > > Sorry for the delay, patch looks good. Please release for 4.3 M1. Also for 3.8.1 please.
Released in master via commit 6b3cf19570292f4957aecb9401f641fdbfac5985. Released doc update via http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=e56f4baea829e08b968447d2709266d5b72de374
Backported to 3.8.1 via commit 2b9357559972763914866304cd1f0268492229d2
The FAQ has CHKPII errors and the copyright date is not correct: GIT\ECLIPSE.PLATFORM.COMMON\BUNDLES\ORG.ECLIPSE.JDT.DOC.ISV\PORTING\3.8\ FAQ.HTML HTML-40 801 Either '<T' is an unsupported tag or replace '<' with '<'. Line: 27 824 Found '<' in text which should be replaced with '<' symbol. Line: 40 Please fix in both branches.
(In reply to comment #47) > [..] Sorry i forgot to run the validator before releasing. Fixed now
Verified for 4.3 M1 using Build id: I20120807-2000
(In reply to comment #3) > > The same is true for annotation not seeing value arrays anymore. The quick fix > > even generates duplicated value() methods. > > Please open a separate bug with a test case. TIA. Hello, without a test case, we cannot work on this. Please raise a separate defect with a small test case. TIA.
Verified for 3.8.1 using Build id: M20120809-1000
*** Bug 388358 has been marked as a duplicate of this bug. ***
The fix was missing in 'R4_2_maintenance'. Cherry-picked with http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=fcb6b9f4449034be93fe4a53d75826bae6f6b75e
(In reply to comment #53) > The fix was missing in 'R4_2_maintenance'. Cherry-picked with > http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/ > ?id=fcb6b9f4449034be93fe4a53d75826bae6f6b75e Just the FAQ entry was missing.
*** Bug 399687 has been marked as a duplicate of this bug. ***
I was retesting this issue in Kepler (4.3). It is fixed BUT you need this line in your eclipse.ini. -DtolerateIllegalAmbiguousVarargsInvocation=true The behavior does not come by default.
(In reply to Srikanth Sankaran from comment #1) > See that this is a bug in JDK5 and JDK6 which has since been corrected > in JDK7 and JDK8 (8b39). > > The observed behavior is the correct behavior and the code needs to be > adjusted suitably. For the test case in comment#0: JDK5 and JDK6 compile it. JDK7 rejects it. JDK8 will reverse course again and accept it. JLS8 is also being amended to say: // -- The previous "applicable variable arity method" terminology incorrectly hinted that, if a variable-arity method is applicable in any phase, it is applicable in and only in Phase 3. This overlooks the fact that variable arity methods can act as fixed-arity methods in Phases 1 and 2. What is relevant is the kinds of adaptations actually used to determine applicability, not the kinds of adaptations allowed by the method declaration. //-- I plan to align not only Java 8 behavior, but across all compliance levels without requiring any system properties. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=421922
