Community
Participate
Working Groups
Build Identifier: 20100617-1415 This warning would report misorted modifiers on classes, fields and methods. This is explained is JLS 3rd Edition (last lines): http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.1.1 http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.3.1 http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.4.3 A quick fix could be suggested to resolve the reported warnings. Reproducible: Always Steps to Reproduce: 1. Here is a sample code snippet for a field declaration: [bad] abstract public class A {...} [good] public abstract class A {...} 2. Here is a sample code snippet for a field declaration: [bad] final static private int a = 0; [good] private static final int a = 0; 3. Here is a sample code snippet for a method declaration: [bad] static public synchronized void doA() {...} [good] synchronized public static void doA() {...} Even though I'm a Eclipse user, I still use IntelliJ Inspection Gadgets to get this warning report and quick fixes. I'll try to dig into JDT to provide a base AST manipulation, I've been given this good starting point by the JDT team: http://www.eclipse.org/articles/article.php?file=Article-JavaCodeManipulation_AST/index.html With that done, we could extend the align fields feature with an align modifiers and fields one: private static final int a = 3; String bbb = "Eclipse rocks!"; protected final C cc = null; protected static final D d = new D(); What do you think? Chris
Move to JDT/UI for comment.
I wouldn't add a warning for this. Generated code should always have the right order, see org.eclipse.jdt.core.dom.AST#newModifiers(int). But a Clean Up (and Save Action) that fixes the order would be very nice. E.g. "Code Style > Declarations > Sort modifiers". See VariableDeclarationCleanUp in the org.eclipse.jdt.ui project for a starting point on how to implement this. The implementation will be a lot simpler than the VariableDeclarationFix, since it should just get the existing modifiers and rewrite them using our ModifierRewrite#setModifiers(int, TextEditGroup) helper. Tests for the new feature should go into CleanUpTest in org.eclipse.jdt.ui.tests. > extend the align fields feature with an align modifiers and fields one That would have to go into a separate request for the code formatter (which lives in JDT/Core). But I don't think that's too interesting. Field modifiers are not an important grouping device. A better practice is to order fields so that they appear in groups that are semantically connected. Furthermore, the wide gaps make the code hard to read (imagine e.g. only a single field with a parameterized type). And in properly documented code, the field declarations are quite far away from each other due to the Javadoc.
I would have done both: compiler warning and Clean Up (and Save Action). But I agree with you, warning should only be related to generated code: if modifiers aren't misorted in generated code, no compiler warning is needed. Besides, I thought about the formatter option, because the "Sort modifiers" feature would make it possible. But you're right that could make code totally unreadable with Generics... Thanks a lot Markus for the starting points you gave me, I'll dive deep into.
On which version should I start working on this patch: 3.7M1, I-builds or N-builds? Is there a document where checking out from CVS tags is explained? I can't see any tag from the CVS Repositories view with anonymous access. Thanks, Chris
I've downloaded 3.7M1 as working environment and I checked out the HEAD revision of the projects. Therefore, patch will be made against HEAD and tested within 3.7M1.
Yes, patches should always be against HEAD and tested against the latest available I-build. But 3.7M1 should also be OK.
Work is in progress. I've created options to choose on which nodes to correct modifiers order: classes (and enums), fields and methods. But it's maybe too much: a single option to fix them all could be enough, couldn't it?
I'll try to write as few lines as possible and reuse available API. That's why I'm thinking to visit BodyDeclaration nodes, clear their modifiers and rewrite them back using the ASTNodeFactory#newModifiers API, which says: "This order is consistent with the recommendations in JLS2 8.1.1, 8.3.1, and 8.4.3.".
Work is almost done but is there an easy way to rewrite node modifiers without copying/rebuilding the entire node? I had a look to VariableDeclarationRewrite but it seems to be a lot of code just to modify modifier nodes order.
(In reply to comment #7) > Work is in progress. I've created options to choose on which nodes to correct > modifiers order: classes (and enums), fields and methods. But it's maybe too > much: a single option to fix them all could be enough, couldn't it? It should work like any other Clean Up (Source > Clean Up...).
Created attachment 176660 [details] Clean Up > Code Style > Correct modifiers order Let me know if the sub-options are useful.
I agree with you Dani, the clean up workflow isn't modified. Could you please tell me if the sub-options should be kept? Thank you, Chris
Created attachment 176664 [details] Patch v1.0 [don't work] Known issues: - Only type declarations have their modifiers corrected in the cleaned up result. Field and method declaration nodes are updated but not rewritten. I think they unparented after calling ASTNode#copySubtree. - CorrectModifiersOrderCleanUp#getPreview isn't called to update the preview page.
Created attachment 176665 [details] Breakpoints Export Breakpoints to broken methods.
I've uploaded a patch, I'm going to need help on this API: https://bugs.eclipse.org/bugs/attachment.cgi?id=176664&action=diff#core_sec1 Thank you, Chris
Hi Christophe > Known issues: > - CorrectModifiersOrderCleanUp#getPreview isn't called to update the preview > page. See CodeStyleTabPage.createPreviewCleanUps(Map)
Thank you Benno, I'll look at this API.
As I said, you should use ModifierRewrite to set the modifiers. CorrectModifiersOrderRewrite is not necessary (and is buggy, see e.g. the example below). Please remove the sub-options and only add one checkbox called "Sort modifiers". Use this term everywhere, including in the option constants. In CorrectModifiersOrderFix#createCleanUp(..) you're wrongly using VariableDeclarationFix_add_final_change_name Example to play with: package m; final @Deprecated /*internal*/ public class X { @Deprecated static @SuppressWarnings("all") public void main(String[] args) { } }
(In reply to comment #18) > As I said, you should use ModifierRewrite to set the modifiers. > CorrectModifiersOrderRewrite is not necessary (and is buggy, see e.g. the > example below). OK, I'll try to do so and reuse existing code base. > Please remove the sub-options and only add one checkbox called "Sort > modifiers". Use this term everywhere, including in the option constants. No problem, that sounds really better. We should update this bug description. > In CorrectModifiersOrderFix#createCleanUp(..) you're wrongly using > VariableDeclarationFix_add_final_change_name OK, I'll have a deeper look into VariableDeclarationFix. > Example to play with: > > package m; > final @Deprecated /*internal*/ public class X { > @Deprecated static @SuppressWarnings("all") > public void main(String[] args) { > } > } Thank you Markus for the time you've spent on my patch. Chris
> > In CorrectModifiersOrderFix#createCleanUp(..) you're wrongly using > > VariableDeclarationFix_add_final_change_name > > OK, I'll have a deeper look into VariableDeclarationFix. It's just a reference to a message string that has been copied instead of replaced with a SortModifiers-specific message. The WithModifiersNodeFinder should be written with less code and should support all node kinds that can have modifiers (search for declarations of "org.eclipse.jdt.core.dom.*.modifiers()"). Use a HierarchicalASTVisitor and override the visit methods of all node types that have a #modifiers() method.
(In reply to comment #20) > It's just a reference to a message string that has been copied instead of > replaced with a SortModifiers-specific message. Great catch!
(In reply to comment #16) > Hi Christophe > > > Known issues: > > - CorrectModifiersOrderCleanUp#getPreview isn't called to update the preview > > page. > > See CodeStyleTabPage.createPreviewCleanUps(Map) Thank you Benno, I didn't catch this one.
Created attachment 177773 [details] Patch v1.1 [don't work] Here is the code snippet I use and which doesn't work: int modifiers= fWithModifiersNode.getModifiers(); ModifierRewrite modifierRewrite= ModifierRewrite.create(rewrite, fWithModifiersNode); modifierRewrite.setModifiers(0, group); modifierRewrite.setModifiers(modifiers, group);
Created attachment 177774 [details] Breakpoints Export
Created attachment 177777 [details] Preferences Dialog
Created attachment 177778 [details] Bug The patch v1.1 causes modifiers to be deleted, here is an example.
I've updated the patch, but I must have misused the ModifierRewrite because modifiers are deleted not sorted. Could you please give me another hint? Thanks, Chris
A ModifierRewrite can only be used once. Javadoc of setModifiers(..) already says "Removes all other flags", so the first line is wrong and unnecessary here: modifierRewrite.setModifiers(0, group); modifierRewrite.setModifiers(modifiers, group); I'll look at this for M3 (but I'll be away the next 2 weeks).
Sorry, didn't get to this in time. Moving to M4.
Sorry, I ran out of time again. Christophe, are you still working on this? Did the hint in comment 28 help? (In reply to comment #25) > Created attachment 177777 [details] > Preferences Dialog Since the new clean up should work on all kinds of declarations, it should go into a new group "Declarations". And we need some tests for the new feature (in CleanUpTest in the org.eclipse.jdt.ui.tests project).
Hi Marcus, I didn't make things better ... But, the last few weeks I learned a lot about the JDT API and wrote my own plugin (guys on the Eclipse Community Forums are great). It's a bare AST requestor/visitor which: - records modifications, - sorts modifiers, - rewrites compilation unit, - applies text changes - saves underlying buffer. Well, it does the job but doesn't support JDT architecture. Anyway, I think I can now make the patch better and learn some more. I'll work on it again and submit a new patch soon for 3.7M6. Good night, Chris
That's great news, thanks!
The previous piece of advice was relevant, but I still have a problem with the rewrite code, it generates a null change. I think I should remove old modifier nodes before inserting new ones with the ModifierRewrite API. That's what I've tried but fWithModifiersNode.modifiers().clear() didn't do the job (no exception but modifiers are duplicated). Did I make myself clear enough to get another hint? Thanks, Chris The rewrite API from the SortModifiersOperation class: public void rewriteAST(CompilationUnitRewrite cuRewrite, LinkedProposalModel model) throws CoreException { ASTRewrite rewrite= cuRewrite.getASTRewrite(); TextEditGroup group= createTextEditGroup(FixMessages.VariableDeclarationFix_sortModifiers_description, cuRewrite); int modifiers= fWithModifiersNode.getModifiers(); // fWithModifiersNode.modifiers().clear(); ModifierRewrite listRewrite= ModifierRewrite.create(rewrite, fWithModifiersNode); listRewrite.setModifiers(modifiers, group); } The thrown exception is the following: org.eclipse.core.runtime.CoreException: The fix 'Correct modifiers order' generated a null change. at org.eclipse.jdt.internal.corext.fix.CompilationUnitRewriteOperationsFix.createChange(CompilationUnitRewriteOperationsFix.java:105) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring.calculateChange(CleanUpRefactoring.java:799) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring$CleanUpASTRequestor.calculateSolutions(CleanUpRefactoring.java:299) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring$CleanUpASTRequestor.acceptAST(CleanUpRefactoring.java:277) at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:883) at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:577) at org.eclipse.jdt.core.dom.ASTParser.createASTs(ASTParser.java:888) at org.eclipse.jdt.internal.corext.dom.ASTBatchParser.createASTs(ASTBatchParser.java:99) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring$CleanUpFixpointIterator.next(CleanUpRefactoring.java:399) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring.cleanUpProject(CleanUpRefactoring.java:707) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring.checkFinalConditions(CleanUpRefactoring.java:663) at org.eclipse.ltk.core.refactoring.CheckConditionsOperation.run(CheckConditionsOperation.java:85) at org.eclipse.ltk.core.refactoring.CreateChangeOperation.run(CreateChangeOperation.java:121) at org.eclipse.ltk.core.refactoring.PerformChangeOperation.run(PerformChangeOperation.java:209) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2313) at org.eclipse.ltk.internal.ui.refactoring.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:87) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:121)
> fWithModifiersNode.modifiers().clear() The clear() is fundamentally wrong, since we use the declarative ASTRewrite framework, which records modifications externally. You should never directly manipulate nodes of the existing AST. I guess that's what caused the NPE. I would expect that the following is all you need: int modifiers= fWithModifiersNode.getModifiers(); ModifierRewrite listRewrite= ModifierRewrite.create(rewrite, fWithModifiersNode); listRewrite.setModifiers(modifiers, group); If that's not working, then please attach your current patch and I can have look.
Thank you Markus for your quick reply. I'm sorry I didn't catch the email notification (hidden label in Gmail). Your clear explanation helped me to well understand my mistake, I've got used to directly modify the AST. Besides, I've tried to remove the clear() API call, but then a the CoreException is raised on null change. I attached the updated patch against the HEAD tested in the latest available I-Build. Thanks, Chris
Created attachment 187990 [details] Single ModifierRewrite call without directly modifying the AST.
Created attachment 188358 [details] SortModifiersFix.java I'm sorry, I missed that ModifierRewrite#internalSetModifiers(..) skips existing modifiers and updates the newModifiers variable, so that method indeed doesn't sort modifiers and can't be used here. Furthermore, the current cleanup infrastructure expects that you only create a SortModifiersFix (and hence also SortModifiersOperations) if you're actually sure that there is something to modify. Therefore, parts of the implementation have to be moved into the UnsortedModifiersNodeFinder. I've implemented this in the attached SortModifiersFix.java.
Created attachment 191941 [details] Fixed patch and added JUnit test method.
Thank you Markus and sorry for this late reply, I totally ran out of time (job switch). Here's a fixed patch with a JUnit test method.
Sorry, I didn't get to this in time, and now it's too late to add a feature to M7. Moving to 3.8 M1.
Sorry that I have to move this again. I really have to reserve more time for this. Now with Java 8, the Clean Up should also make sure annotations are sorted correctly depending on their @Target: TYPE_USE annotations should be at the end of the modifiers list, and all annotations without the TYPE_USE target should be in front of all the other modifiers.
It has been another 2 years.. any word when this can be added?
(In reply to Jakub Bocheński from comment #42) > It has been another 2 years.. any word when this can be added? There's no ETA. Too busy with other stuff. Sorry.
Another year later... PLEEEASE add this feature. Annotation re-ordering can be added later as well.
(In reply to Nasuki Matamoto from comment #44) > Another year later... > PLEEEASE add this feature. Annotation re-ordering can be added later as well. Feel free to revive the old patch and contribute it via Gerrit. See https://wiki.eclipse.org/Platform_UI/How_to_Contribute
Christophe's patch is so close. :)
