Community
Participate
Working Groups
This is the starting point - try statement with no resources ------------------------------------------------------------------------- FileReader reader1 = new FileReader("file1"); try { int ch; while ((ch = reader1.read()) != -1) { System.out.println(ch); } } finally { } ------------------------------------------------------------------------- This is the result - the resource is not inserted inside the try ------------------------------------------------------------------------- FileReader reader1 = new FileReader("file1")try { int ch; while ((ch = reader1.read()) != -1) { System.out.println(ch); } } finally { } ------------------------------------------------------------------------- Reproducible test case in the attached project. (It is a plain java project containing one class, just run it as a java application) Things are better if the try statement already contains one or more resource. Note the ; after the second resource declaration. ------------------------------------------------------------------------- FileReader reader = new FileReader("file1"); try (FileReader reader1 = new FileReader("file1"); FileReader reader2 = new FileReader("file2");) { int ch; while ((ch = reader1.read()) != -1) { System.out.println(ch); reader.read(); } } ------------------------------------------------------------------------- The ; is already there but another is inserted resulting in an error. ------------------------------------------------------------------------- try (FileReader reader1 = new FileReader("file1"); FileReader reader2 = new FileReader("file2");; FileReader reader = new FileReader("file1")) { int ch; while ((ch = reader1.read()) != -1) { System.out.println(ch); reader.read(); } } -------------------------------------------------------------------------
This issue is blocking bug 349390.
I'll take a look.
Created attachment 199130 [details] test case I thought I had attached the test project... anyway attaching it now.
(In reply to comment #0) > Things are better if the try statement already contains one or more resource. > Note the ; after the second resource declaration. > ------------------------------------------------------------------------- > FileReader reader = new FileReader("file1"); > try (FileReader reader1 = new FileReader("file1"); > FileReader reader2 = new FileReader("file2");) { > int ch; > while ((ch = reader1.read()) != -1) { > System.out.println(ch); > reader.read(); > } > } > ------------------------------------------------------------------------- > > The ; is already there but another is inserted resulting in an error. > ------------------------------------------------------------------------- > try (FileReader reader1 = new FileReader("file1"); > FileReader reader2 = new FileReader("file2");; FileReader reader = new > FileReader("file1")) { > int ch; > while ((ch = reader1.read()) != -1) { > System.out.println(ch); > reader.read(); > } > } > ------------------------------------------------------------------------- Another issue if formatting, the resource is inserted on the same line as the last resource. The formatter options for line wrapping should be followed. The default setting is to keep every resource on new line, rewrite should at least follow this.
(In reply to comment #4) > Another issue if formatting, the resource is inserted on the same line as the > last resource. The formatter options for line wrapping should be followed. The > default setting is to keep every resource on new line, rewrite should at least > follow this. The default is to keep every resource on a new line if needed. There is no force bit set for this formatting. So it is difficult to know in the formatting if a new line should be inserted. To know what needs to be inserted, we use the org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteFormatter.Prefix instances. The problem with this is that we use a small code snippet and we check how it ends up being formatted. Then we extract the portion of code that we want. We cannot guess that the actual code goes beyond the end of line value while the code snippet doesn't. Is it possible for the UI calls to format the modified code once the ast rewrite is applied ? If you see a way to know when a new line should be inserted, then please let me know.
(In reply to comment #5) > Is it possible for the UI calls to format the modified code once the ast > rewrite is applied ? No. Also this would be the wrong thing to do, the Javadoc of ASTRewrite says " The rewrite infrastructure tries to generate minimal * text changes, preserve existing comments and indentation, and follow code * formatter settings. " (In reply to comment #0) > This is the result - the resource is not inserted inside the try Any luck with this issue ? The formatting and ; issue is something I can live with for now, but this issue makes bug 349390 impossible to fix.
(In reply to comment #6) > No. Also this would be the wrong thing to do, the Javadoc of ASTRewrite says > " > The rewrite infrastructure tries to generate minimal > * text changes, preserve existing comments and indentation, and follow code > * formatter settings. > " Yes, but for alignments and new lines, this is not possible to follow. The problem being that the code snippet used to "detect" formatter settings cannot be set so that the line size option is respected when new lines are added only if needed (default settings for resources alignments). > Any luck with this issue ? The formatting and ; issue is something I can live > with for now, but this issue makes bug 349390 impossible to fix. This is under control now. I am testing the changes before releasing.
> Is it possible for the UI calls to format the modified code once the ast > rewrite is applied ? Only with ugly hacks. We usually do "pure" ASTRewrite changes. For this, we would have to track nodes across the rewrite and then format the added nodes again, taking extra care that we don't touch the existing nodes that should stay unchanged. Then we'd have to add the formatting changes to the rewrite edits. It looks like this is a general problem in the ASTRewrite (see e.g. bug 137499 comment 2 and other open bugs). This also happens in other cases, e.g. when you apply one of the quick fixes in the method body here (default Eclipse profile): import java.util.LinkedHashMap; import java.util.List; public class Arry { @SuppressWarnings({ "null", "unused", "this is a very long token", "this is another very long token", "and a third long token" }) void foo(LinkedHashMap<String, Integer> a, LinkedHashMap<String, Integer> b) { java.util.List list = null; list.size(); int xx; foo(a, b, a); } } If you can add a special case for this situation and just always add a newline even with the default formatter settings, then I guess that would solve the majority of cases. Otherwise, we can also live without this.
Created attachment 199198 [details] Proposed fix + regression tests Deepak, please give it a try.
Created attachment 199200 [details] Proposed fix + regression tests This new patch is forcing a new line between resources in twr. This makes the formatting more reliable and the ast rewrite can respect this formatting in this case. I'll rerun all tests to make sure I don't break an existing test. All tests passed with the previous patch.
Is it really necessary to copy the huge ASTRewriteAnalyzer.ListRewriter.rewriteList(..) method? I'd feel more confident (also for future fixes) if you just changed the existing method and added an optional "String endKeyword" parameter (leave a the delegate method that keeps the old parameter list). For the line break issue, I'd rather keep the default formatter options untouched. Can't you just add the "force" flag in the formatter options that are used by the ASTRewrite?
(In reply to comment #11) > Is it really necessary to copy the huge > ASTRewriteAnalyzer.ListRewriter.rewriteList(..) method? I'd feel more confident > (also for future fixes) if you just changed the existing method and added an > optional "String endKeyword" parameter (leave a the delegate method that keeps > the old parameter list). This was not meant to be the final patch. It was provided to see if the blocked bug can be fixed with it. I do plan to clean this before releasing. > For the line break issue, I'd rather keep the default formatter options > untouched. Can't you just add the "force" flag in the formatter options that > are used by the ASTRewrite? I'll see what I can do and adjust the patch accordingly.
Thanks Olivier! The patch solves the blocking issue from comment 0 and also with default formatter setting a new line is inserted for a new resource. So the behavior is much better now. However, there are still some formatting issues with patch from bug 349390. I am looking into them. If you want we can deal with those in separate bugs.
..and the ; issue from comment 0 is also fixed.
Created attachment 199278 [details] Final patch
This last patch fixes issues reported by Markus in comment 11.
Released in BETA_JAVA7 branch.
(In reply to comment #15) > Created attachment 199278 [details] [diff] > Final patch The ASTRewriteFormatter constructor now modifies the options map. This map is passed in by 2 APIs: - ASTRewrite#rewriteAST(IDocument, Map) - CompilationUnit#rewrite(IDocument, Map) I think it's best if we just state in the APIs that the map may be modified. This is a slight behavior change, but I don't think it will affect any client.
Reopening to make sure comment 18 doesn't get forgotten.
(In reply to comment #19) > Reopening to make sure comment 18 doesn't get forgotten. Only the inner option map is modified. The one passed in is not modified. Do we really need to update the documentation for this ?
> Only the inner option map is modified. The one passed in is not modified. I don't understand that. The 2 APIs I mentioned receive a map. This map is passed through a few method calls and then assigned to this.options in the ASTRewriteFormatter constructor. The map is not copied anywhere, so the "inner" and "passed in" map are really the same.
(In reply to comment #21) > I don't understand that. The 2 APIs I mentioned receive a map. This map is > passed through a few method calls and then assigned to this.options in the > ASTRewriteFormatter constructor. The map is not copied anywhere, so the "inner" > and "passed in" map are really the same. Right. I think we should make sure the map passed in is not modified. So since we are modifying it, we should use a copy of it. Changing the given map looks like a bug to me.
Created attachment 200297 [details] Proposed fix With this patch, I don't think we need to add anything in the two method's javadocs.
Created attachment 200302 [details] Proposed additional fix We can also go that way, although the copying will be useless in most cases. Either way, I saw another bug: When the passed map is null, then the option is currently not set. This patch uses the copying strategy and fixes the other bug.
Yes, even better. Released in BETA_JAVA7 branch only.
Verified for 3.8M1 with build I20110729-1200.
