Community
Participate
Working Groups
Created attachment 108083 [details] editor screenshot It would be convenient to have a quickfix for iterating over a collection without the need to store it (eg as a local var). void foo(Map<String, Integer> map) { map.keySet()<-- Cursor position here } The best way (I know) to iterate over a computed collection is - quickfix to assign as local var - use the foreach template - inline the local var A quickfix for iterating over the collection at the cursor position could save several key strokes.
Move to JDT/UI
Should be a quick fix / quick assist like "Assign statement to new local variable", which works whether or not the ";" is there after the ExpressionStatement.
Created attachment 236902 [details] This patch fixes the bug. Added a new Quick Assist proposal to the QuickAssistProcessor, including tests. The patch covers: * Iterate over iterables/arrays using foreach * Iterate over iterables using iterator based for loops * Iterate over arrays using index based for loops The assist is available on: * local variables, method parameters, and fields (iterables or arrays) List foos; ... foos<-- Cursor position here * method invocations returning iterables/arrays map.keySet()<-- Cursor position here Like suggested, it works whether or not the ";" is there after the ExpressionStatement. foos;<-- Cursor position here map.keySet();<-- Cursor position here The unit tests should cover typical and corner cases. Looking forward to your feedback. Its a small but annoying issue so me and my Yatta colleagues hope to see the patch in the next release :) This contribution complies with http://www.eclipse.org/legal/CoO.php
Manju, please test and review.
*** Bug 376765 has been marked as a duplicate of this bug. ***
Thanks for the patch. It worked like a charm. Released the patch as : http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=5e64224cce6e1c08533fec6fcf0c3143d1e2a245 Few minor points to take care in future patches: 1. When a new file is created the Copyright year should be the current year. 2. In Javadoc use the html tag <code></code> to refer literals or keywords. Looking forward for many more good quality patches in future.
The code structure looks good, but there are a few points that need improvement: 1. Don't use ASTNode.copySubtree unless really necessary. In GenerateForLoopAssistProposal, all usages can be replaced with "rewrite.createCopyTarget(fSubExpression)". The advantage is that you don't lose comments and formatting of the copied expression. 2. When creating new ASTNodes for a type reference, don't use ast#newSimpleType(*), but always use ImportRewrite#addImport(*, AST, CSIRC). The result of that method is the Type node that you can insert into the AST. You can use #addImport(*) multiple times for the same type. The snippet below shows why a ContextSensitiveImportRewriteContext is necessary to handle all situations correctly. The position of the CSIRC can e.g. be fCurrentNode. 3. When iterating over a raw Iterable, don't assume expr.iterator() returns an Iterator<Object>. Better leave the Iterator type raw. The expression may eventually be generified with a different type argument, and at that time, the generated Iterator<Object> would cause a compile error. Here's a snippet with two calls in A#foo() that test these situations. The result should use fully-qualified names where necessary and should not lose formatting in the expressions. package snippet; public class A { class Object {} class Iterator {} void foo() { B.get( /*important: empty*/ ); B.raw(1+ 2); } } package snippet; import java.util.ArrayList; import java.util.Date; import java.util.Set; public class B { static ArrayList<Date> get() { return new ArrayList<Date>(); } static Set raw(int i) { return java.util.Collections.emptySet(); } }
4. QuickAssistProcessor#getGenerateForLoopProposals(*) should be static. 5. A few methods in GenerateForLoopAssistProposal take an AST and an ASTRewrite. The AST can easily be dropped from the parameter list and replaced by AST ast= rewrite.getAST();
Created attachment 237236 [details] Patch containing suggested improvements Thanks for the reviews; I adapted the patch as suggested. As the previous patch is already pushed to master, this patch can be applied on top of the master branch. The following things should work correctly now: - All required imports are added as long as they create no naming conflicts (In case of naming conflicts fully qualified names are used) - The quick fix keeps the format of the wrapped expression - In case of generating a loop over a raw type the type object is not inferred as generic type argument anymore I also changed QuickAssistProcessor#getGenerateForLoopProposals(*) to be static and added the unnecessary AST method parameter as you suggested. The patch also adds additional tests covering the described behaviour as well as new tests for generic types and import handling for arrays. This contribution complies with http://www.eclipse.org/legal/CoO.php
*** Bug 421299 has been marked as a duplicate of this bug. ***
The changes looks fine. Released the patch as: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=06d69b4e9a453b31a9617a2d5c681e762376d562
Thanks, the rewriting looks good now. Some more polish: 1. "Iterate over array using for" should also declare a variable for the current element in the ForStatement body (like "Iterate over iterable using iterator" already does), i.e. generate: for (int i = 0; i < array.length; i++) { String string = array[i]; } 2. Only the first occurrence of a generated name should be a tab position in the linked mode. Exclude the references like this: addLinkedPosition(..., LinkedPositionGroup.NO_STOP, ...); 3. The functionality is currently only available as quick assist. This is good and necessary for cases where the reference doesn't cause a compile error, e.g. for "getList();". But in case there is a compile error, the fixes are also just available via Ctrl+1, but they don't show up in the quick fix hover. This problem has shown up before for other quick assists/fixes. See e.g. callers of QuickAssistProcessor#getInferDiamondArgumentsProposal(..) and how the implementation of that method prevents duplicates: // don't add if already added as quick fix if (containsMatchingProblem(locations, IProblem.DiamondNotBelow17)) return false; 4. The quick assist name "Iterate over array or iterable using foreach" looks a bit clumsy. The quick assist knows whether it will apply to an array or an iterable expression, so we shouldn't bother the user with a long "... or ..." name. Is it really important to spell out the collection type? Could we just change the names to: - Create enhanced 'for' loop - Create 'for' loop <= for array type - Create 'for' loop using Iterator <= for subtypes of Iterable - Create 'for' loop using index <= for subtypes of List Bug 89432 comment 13 already discussed naming options for a related quick assist. It would make sense to align all these names, i.e. also change those to - Convert to 'for' loop using index - Convert to 'for' loop using Iterator 5. The "Create 'for' loop using index" for subtypes of List should be implemented as well.
Created attachment 237461 [details] Patch containing suggested polishing Your suggestions sound good, I included them in a new patch. It addresses the following issues: - the proposal names comply to your suggested naming convention - added a new for loop proposal to iterate over a java.util.List implementation using an index and java.util.List#get(int) - if using the quick assist to loop over an array, the temporary variable for the actual element is generated now - using tab jumps only to the first occurrence of a variable, next ones are skipped - the proposals are available in the quick fix hover now (added duplicate avoidance as well) Furthermore the patch contains new tests for bounded wildcards and the quick fix feature. As the previous changeset was already pushed to master, this new patch has to be applied on top of the previous ones. This contribution complies with http://www.eclipse.org/legal/CoO.php
I didn't release the removal of the call to getGenerateForLoopProposals(..) in QuickAssistProcessor#hasAssists(IInvocationContext), and I fixed the "if (resultingCollections == null)" case. hasAssists(..) is used when the "Light bulb for quick assist" is enabled. This code path is probably not reachable today, but it helps to keep all get*Proposals(..) methods in sync. In GenerateForLoopAssistProposal#extractElementType(AST), I replaced the custom code with a second call to Bindings#normalizeForDeclarationUse(..). Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=424380500fc9a894495314244132e460b2dc3090
(In reply to Lukas Hanke from comment #3) > The assist is available on: > > * local variables, method parameters, and fields (iterables or arrays) > > List foos; > ... > foos<-- Cursor position here Unfortunately, this does (no longer?) work in I20140318-0830, see bug 430818.
