Bugzilla – Full Text Bug Listing
| Summary: | [1.8][compiler] Illegal combination of modifiers on interface methods produces confusing diagnostics | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Srikanth Sankaran <srikanth_sankaran> | ||||||||||
| Component: | Core | Assignee: | Manoj N Palat <manoj.palat> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | jarthana, manju656, manoj.palat, markus.kell.r, stephan.herrmann | ||||||||||
| Version: | 4.3 | Flags: | jarthana:
review+
|
||||||||||
| Target Milestone: | BETA J8 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows 7 | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 380190, 410170 | ||||||||||||
| Attachments: |
|
||||||||||||
Manoj, please take a look. Manoj, when working on this could you please also handle the following cases:
default abstract void foo();
default abstract void foo() {}
Currently the compiler complains, e.g., about an abstract method with a body, but it should first of all report the illegal modifier combination:
" It is a compile-time error if a method is declared with more than one of the modifiers abstract, default, or static. "
Please coordinate with Jay's work on static methods in an interface.
Thanks
Created attachment 227313 [details] Proposed Patch Fixes the original (synchronized) issue. Srikanth: >>While fixing this, also see if we need a Java8 version of this message: Since DefaultMethods >= 1.8, felt that a Java8 version is not necessary since the error message gets printed only for DefaultMethods. For all other methods, error message remains the same (a testcase added at MethodVerify for the same as well). Stephan: Will post another patch for the "more than one" modifiers problem soon. Please see https://bugs.eclipse.org/bugs/show_bug.cgi?id=399780#c8 and https://bugs.eclipse.org/bugs/show_bug.cgi?id=399780#c9 Manoj, I'll ask Jay to absorb the changes for the part about "synchronized not being allowed any more" into bug 399780 and we will retain the present bug only for the elimination of message confusion. I changed the title accordingly. *** Bug 406245 has been marked as a duplicate of this bug. *** Please ensure additional concerns expressed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=406245 are also addressed. Created attachment 232421 [details]
Proposed Patch
- Added a new error message to inform only one of the allowed (abstract, default or static) modifiers.
- Added a new error message for combination of abstract and stritcfp.
- Added a new "allowed modifiers message for >=1.8" replacing the default method error message.
(In reply to comment #8) > Created attachment 232421 [details] > Proposed Patch > > - Added a new error message to inform only one of the allowed (abstract, > default or static) modifiers. > - Added a new error message for combination of abstract and stritcfp. > - Added a new "allowed modifiers message for >=1.8" replacing the default > method error message. Please note that this is a WIP patch to unblock UI team. Manju: you can take this patch to start on your bug. Only pending work in the bug is re-arranging the warning message as per Markus comment in bug 217966 comment 2 and correcting the error messages of failing test cases. Created attachment 232426 [details]
Proposed Patch - All changes
(In reply to comment #10) > Created attachment 232426 [details] > Proposed Patch - All changes Review comments: 1. "Illegal modifiers for the interface method {0}; strictfp is not permitted for abstract interface method {0}" can be shorter. How about this: "strictfp is not permitted for abstract interface methods" 2. Consider renaming IllegalAbstractStrictfpModifierCombinationForInterfaceMethod to something shorter (IllegalModifierForAbstractInterfaceMethod ?) 3. Similarly IllegalModifierCombinationForInterfaceMethod -> IllegalModifiersForInterfaceMethod ? (In reply to comment #11) Jay: I differ with the following suggestions. The reasons are inlined. Let me know if these need to be changed. > 1. "Illegal modifiers for the interface method {0}; strictfp is not > permitted for abstract interface method {0}" can be shorter. How about this: > > "strictfp is not permitted for abstract interface methods" Similar error messages (search for strictfp) have method name included in the error message. So the suggested error message may not be specific and as per the other existing messages. > 2. Consider renaming > IllegalAbstractStrictfpModifierCombinationForInterfaceMethod to something > shorter (IllegalModifierForAbstractInterfaceMethod ?) > > 3. Similarly IllegalModifierCombinationForInterfaceMethod -> > IllegalModifiersForInterfaceMethod ? For the above, the suggested name does not look appropriate since the modifiers are legal, Only the combination of the modifiers are not legal, and hence the original names. (In reply to comment #12) > Similar error messages (search for strictfp) have method name included in > the error message. So the suggested error message may not be specific and as > per the other existing messages. As long as the error message is unambiguous and accurate, I don't think it needs to be consistent with other messages. On the other hand, consider this: "Illegal combination of modifiers for the interface method {0}; only one of abstract, default, or static permitted" The first part states that this modifier combination is illegal for this "interface method" and the second part specifies "why". But in the one in question, the first part doesn't add any extra meaning because the part "abstract interface method" is anyway covered by the latter part of the message. And as for the method name, we highlight and report the error on the method in question and hence the specifying the method name itself can't add any more value. > > > 2. Consider renaming > > IllegalAbstractStrictfpModifierCombinationForInterfaceMethod to something > > shorter (IllegalModifierForAbstractInterfaceMethod ?) > > > > 3. Similarly IllegalModifierCombinationForInterfaceMethod -> > > IllegalModifiersForInterfaceMethod ? > > For the above, the suggested name does not look appropriate since the > modifiers are legal, Only the combination of the modifiers are not legal, > and hence the original names. The message ID is only a tag and need not be as descriptive as the message itself. Please refer to IProblem#IllegalModifierForInterfaceMethod18. Th suggested names are just examples of making them shorter. I just don't like the length of IDs which don't add any value. > And as for the method name, we highlight and report the error on the method
> in question and hence the specifying the method name itself can't add any
> more value.
The method name can be interesting if you look at problems in the Problems view. But it must not show up twice as in 1057 in the patch.
Let's look at a similar case in a class:
public abstract class Try {
abstract strictfp void foo(); /* 362: The abstract method foo in type Try can only set a visibility modifier, one of public or protected */
}
For abstract methods in classes as well as in interfaces, I think we can reuse IllegalAbstractModifierCombinationForMethod (362), but the message would better be stated like this (also avoids the non-JLS-compliant use of "visibility", which means "absence of shadowing", see 6.4.1):
362 = Illegal modifier for method {1}: abstract can only be combined with one of public or protected
(I removed the type name, but didn't adjust the message arguments, because that could break translations or client code that accesses IProblem#getArguments(). Keeping the type name would also be fine with me.).
Created attachment 233362 [details]
Proposed Patch
Reworked on the naming/messaging comments - treading a middle path for names.
committed via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=328c06fa136c963530115a367863b6a931384871 (In reply to comment #15) > Created attachment 233362 [details] > Proposed Patch > > Reworked on the naming/messaging comments - treading a middle path for names. I still see the same name for the problem IDs? (In reply to comment #16) > committed via > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?h=BETA_JAVA8&id=328c06fa136c963530115a367863b6a931384871 This problem ID has 61 characters in it and I don't think we need that to be that descriptive, do we? IllegalAbstractStrictfpModifierCombinationForInterfaceMethod Did you push the wrong change set, perhaps? I still see the problem ids having long names and the following error message:
+1057 = Illegal modifiers for the interface method {0}; strictfp is not permitted for abstract interface methods
What I really meant was something like the following:
+1057 = Illegal modifiers for the interface method {0}; strictfp is not permitted for abstract interface methods or (method {0})
Reopening for these to be fixed.
(In reply to comment #19) > Changes committed via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=d09fa4802281032dbffe5d54191a48ffcdbfb895 |
BETA_JAVA8: 0.6.1 pulled back the allowance made for an interface method to be tagged with the keyword synchronized. As a result, the following should not compile, but does: interface I { synchronized default void foo() { } } // --- While fixing this, also see if we need a Java8 version of this message: 359 = Illegal modifier for the interface method {0}; only public & abstract are permitted Now interface methods can also have default and static as keywords. Also see 1050 = Illegal modifier for the interface method {0}; only public, abstract, strictfp & synchronized are permitted