Skip to content

Commit

Permalink
Bug 430818: [1.8][quick fix] Quick fix for "for loop" is not shown fo…
Browse files Browse the repository at this point in the history 
…r bare local variable/argument/field

Signed-off-by: Lukas Hanke <[email protected]>
  • Loading branch information
@lhanke @mkeller
lhanke authored and mkeller committed May 14, 2014
1 parent c4681b2 commit ff852ca
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 69 deletions.
36 changes: 16 additions & 20 deletions org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/AssistQuickFixTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* IBM Corporation - initial API and implementation
* Sebastian Davids <[email protected]> - testInvertEquals1-23
* Lukas Hanke <[email protected]> - Bug 241696 [quick fix] quickfix to iterate over a collection - https://bugs.eclipse.org/bugs/show_bug.cgi?id=241696
* Lukas Hanke <[email protected]> - Bug 430818 [1.8][quick fix] Quick fix for "for loop" is not shown for bare local variable/argument/field - https://bugs.eclipse.org/bugs/show_bug.cgi?id=430818
*******************************************************************************/
package org.eclipse.jdt.ui.tests.quickfix;

Expand Down Expand Up @@ -8611,7 +8612,7 @@ public void testGenerateForSimple() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 2);
assertNumberOfProposals(proposals, 4);
assertCorrectLabels(proposals);

String[] expected= new String[2];
Expand All @@ -8633,8 +8634,7 @@ public void testGenerateForSimple() throws Exception {
buf.append("import java.util.Iterator;\n");
buf.append("public class E {\n");
buf.append(" void foo(Collection<String> collection) {\n");
buf.append(" for (Iterator<String> iterator = collection.iterator(); iterator\n");
buf.append(" .hasNext();) {\n");
buf.append(" for (Iterator<String> iterator = collection.iterator(); iterator.hasNext();) {\n");
buf.append(" String string = iterator.next();\n");
buf.append(" \n");
buf.append(" }\n");
Expand Down Expand Up @@ -8672,7 +8672,7 @@ public void testGenerateForWithSemicolon() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 4);
assertNumberOfProposals(proposals, 2);
assertCorrectLabels(proposals);

String[] expected= new String[2];
Expand Down Expand Up @@ -8793,7 +8793,7 @@ public void testGenerateForComplexParametrization() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 3);
assertNumberOfProposals(proposals, 5);
assertCorrectLabels(proposals);

String[] expected= new String[3];
Expand All @@ -8817,8 +8817,7 @@ public void testGenerateForComplexParametrization() throws Exception {
buf.append("import java.util.LinkedList;\n");
buf.append("public class E {\n");
buf.append(" void foo(MySecondOwnIterable collection) {\n");
buf.append(" for (Iterator<String> iterator = collection.iterator(); iterator\n");
buf.append(" .hasNext();) {\n");
buf.append(" for (Iterator<String> iterator = collection.iterator(); iterator.hasNext();) {\n");
buf.append(" String string = iterator.next();\n");
buf.append(" \n");
buf.append(" }\n");
Expand Down Expand Up @@ -8874,7 +8873,7 @@ public void testGenerateForGenerics() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 2);
assertNumberOfProposals(proposals, 4);
assertCorrectLabels(proposals);

String[] expected= new String[2];
Expand All @@ -8898,8 +8897,7 @@ public void testGenerateForGenerics() throws Exception {
buf.append("import java.util.Iterator;\n");
buf.append("public class E {\n");
buf.append(" void <T extends Date> foo(Collection<T> collection) {\n");
buf.append(" for (Iterator<T> iterator = collection.iterator(); iterator\n");
buf.append(" .hasNext();) {\n");
buf.append(" for (Iterator<T> iterator = collection.iterator(); iterator.hasNext();) {\n");
buf.append(" T t = iterator.next();\n");
buf.append(" \n");
buf.append(" }\n");
Expand Down Expand Up @@ -9005,7 +9003,7 @@ public void testGenerateForUpperboundWildcard() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 3);
assertNumberOfProposals(proposals, 5);
assertCorrectLabels(proposals);

String[] expected= new String[3];
Expand Down Expand Up @@ -9083,7 +9081,7 @@ public void testGenerateForLowerboundWildcard() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 3);
assertNumberOfProposals(proposals, 5);
assertCorrectLabels(proposals);

String[] expected= new String[3];
Expand Down Expand Up @@ -9230,7 +9228,7 @@ public void testGenerateForMissingParametrization() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 2);
assertNumberOfProposals(proposals, 4);
assertCorrectLabels(proposals);

String[] expected= new String[2];
Expand All @@ -9252,8 +9250,7 @@ public void testGenerateForMissingParametrization() throws Exception {
buf.append("import java.util.Iterator;\n");
buf.append("public class E {\n");
buf.append(" void foo(Collection collection) {\n");
buf.append(" for (Iterator iterator = collection.iterator(); iterator\n");
buf.append(" .hasNext();) {\n");
buf.append(" for (Iterator iterator = collection.iterator(); iterator.hasNext();) {\n");
buf.append(" Object object = iterator.next();\n");
buf.append(" \n");
buf.append(" }\n");
Expand Down Expand Up @@ -9293,7 +9290,7 @@ public void testGenerateForLowVersion() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 1);
assertNumberOfProposals(proposals, 3);
assertProposalDoesNotExist(proposals, CorrectionMessages.QuickAssistProcessor_generate_enhanced_for_loop);
assertCorrectLabels(proposals);

Expand All @@ -9306,8 +9303,7 @@ public void testGenerateForLowVersion() throws Exception {
buf.append("import java.util.Iterator;\n");
buf.append("public class E {\n");
buf.append(" void foo(Collection collection) {\n");
buf.append(" for (Iterator iterator = collection.iterator(); iterator\n");
buf.append(" .hasNext();) {\n");
buf.append(" for (Iterator iterator = collection.iterator(); iterator.hasNext();) {\n");
buf.append(" Object object = iterator.next();\n");
buf.append(" \n");
buf.append(" }\n");
Expand Down Expand Up @@ -9344,7 +9340,7 @@ public void testGenerateForArray() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 2);
assertNumberOfProposals(proposals, 4);
assertCorrectLabels(proposals);

String[] expected= new String[2];
Expand Down Expand Up @@ -9401,7 +9397,7 @@ public void testGenerateForNameClash() throws Exception {
AssistContext context= getCorrectionContext(cu, buf.toString().lastIndexOf(selection) + selection.length(), 0);
List proposals= collectAssists(context, false);

assertNumberOfProposals(proposals, 2);
assertNumberOfProposals(proposals, 4);
assertCorrectLabels(proposals);

String[] expected= new String[2];
Expand Down
13 changes: 6 additions & 7 deletions ...lipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/LocalCorrectionsQuickFixTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* IBM Corporation - initial API and implementation
* Benjamin Muskalla <[email protected]> - [quick fix] Shouldn't offer "Add throws declaration" quickfix for overriding signature if result would conflict with overridden signature
* Lukas Hanke <[email protected]> - Bug 241696 [quick fix] quickfix to iterate over a collection - https://bugs.eclipse.org/bugs/show_bug.cgi?id=241696
* Lukas Hanke <[email protected]> - Bug 430818 [1.8][quick fix] Quick fix for "for loop" is not shown for bare local variable/argument/field - https://bugs.eclipse.org/bugs/show_bug.cgi?id=430818
*******************************************************************************/
package org.eclipse.jdt.ui.tests.quickfix;

Expand Down Expand Up @@ -55,7 +56,7 @@ public class LocalCorrectionsQuickFixTest extends QuickFixTest {
* caused by:
* Bug 430336: [1.8][compiler] Bad syntax error recovery: Lonely identifier should be variable name, not type
*/
public static final boolean BUG_430818= true;
public static final boolean BUG_430818= false;

private IJavaProject fJProject1;
private IPackageFragmentRoot fSourceFolder;
Expand Down Expand Up @@ -9814,7 +9815,7 @@ public void testLoopOverAddedToFixesForVariable() throws Exception {
newOptions.put(DefaultCodeFormatterConstants.FORMATTER_PUT_EMPTY_STATEMENT_ON_NEW_LINE, "true");
try {
fJProject1.setOptions(newOptions);
List proposals= collectCorrections(cu, getASTRoot(cu), 2, null);
List proposals= collectCorrections(cu, getASTRoot(cu), 3, null);

assertNumberOfProposals(proposals, 2);
assertCorrectLabels(proposals);
Expand All @@ -9838,8 +9839,7 @@ public void testLoopOverAddedToFixesForVariable() throws Exception {
buf.append("import java.util.Iterator;\n");
buf.append("public class E {\n");
buf.append(" void foo(Collection<String> collection) {\n");
buf.append(" for (Iterator<String> iterator = collection.iterator(); iterator\n");
buf.append(" .hasNext();) {\n");
buf.append(" for (Iterator<String> iterator = collection.iterator(); iterator.hasNext();) {\n");
buf.append(" String string = iterator.next();\n");
buf.append(" \n");
buf.append(" }\n");
Expand Down Expand Up @@ -9942,7 +9942,7 @@ public void testGenerateForeachNotAddedForLowVersion() throws Exception {
newOptions.put(DefaultCodeFormatterConstants.FORMATTER_PUT_EMPTY_STATEMENT_ON_NEW_LINE, "true");
try {
fJProject1.setOptions(newOptions);
List proposals= collectCorrections(cu, getASTRoot(cu), 2, null);
List proposals= collectCorrections(cu, getASTRoot(cu), 3, null);

assertNumberOfProposals(proposals, 1);
assertCorrectLabels(proposals);
Expand All @@ -9957,8 +9957,7 @@ public void testGenerateForeachNotAddedForLowVersion() throws Exception {
buf.append("import java.util.Iterator;\n");
buf.append("public class E {\n");
buf.append(" void foo(Collection collection) {\n");
buf.append(" for (Iterator iterator = collection.iterator(); iterator\n");
buf.append(" .hasNext();) {\n");
buf.append(" for (Iterator iterator = collection.iterator(); iterator.hasNext();) {\n");
buf.append(" Object object = iterator.next();\n");
buf.append(" \n");
buf.append(" }\n");
Expand Down
83 changes: 64 additions & 19 deletions org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Chris West (Faux) <[email protected]> - [quick assist] "Use 'StringBuilder' for string concatenation" could fix existing misuses - https://bugs.eclipse.org/bugs/show_bug.cgi?id=282755
* Lukas Hanke <[email protected]> - Bug 241696 [quick fix] quickfix to iterate over a collection - https://bugs.eclipse.org/bugs/show_bug.cgi?id=241696
* Eugene Lucash <[email protected]> - [quick assist] Add key binding for Extract method Quick Assist - https://bugs.eclipse.org/424166
* Lukas Hanke <[email protected]> - Bug 430818 [1.8][quick fix] Quick fix for "for loop" is not shown for bare local variable/argument/field - https://bugs.eclipse.org/bugs/show_bug.cgi?id=430818
*******************************************************************************/
package org.eclipse.jdt.internal.ui.text.correction;

Expand Down Expand Up @@ -67,7 +68,6 @@
import org.eclipse.jdt.core.dom.EnhancedForStatement;
import org.eclipse.jdt.core.dom.Expression;
import org.eclipse.jdt.core.dom.ExpressionStatement;
import org.eclipse.jdt.core.dom.FieldAccess;
import org.eclipse.jdt.core.dom.ForStatement;
import org.eclipse.jdt.core.dom.IBinding;
import org.eclipse.jdt.core.dom.IMethodBinding;
Expand All @@ -90,6 +90,7 @@
import org.eclipse.jdt.core.dom.PrefixExpression;
import org.eclipse.jdt.core.dom.PrefixExpression.Operator;
import org.eclipse.jdt.core.dom.PrimitiveType;
import org.eclipse.jdt.core.dom.QualifiedName;
import org.eclipse.jdt.core.dom.ReturnStatement;
import org.eclipse.jdt.core.dom.SimpleName;
import org.eclipse.jdt.core.dom.SimpleType;
Expand Down Expand Up @@ -124,6 +125,7 @@
import org.eclipse.jdt.internal.corext.dom.DimensionRewrite;
import org.eclipse.jdt.internal.corext.dom.LinkedNodeFinder;
import org.eclipse.jdt.internal.corext.dom.ModifierRewrite;
import org.eclipse.jdt.internal.corext.dom.ScopeAnalyzer;
import org.eclipse.jdt.internal.corext.dom.Selection;
import org.eclipse.jdt.internal.corext.dom.SelectionAnalyzer;
import org.eclipse.jdt.internal.corext.dom.TokenScanner;
Expand Down Expand Up @@ -2741,26 +2743,31 @@ private static boolean getConvertIterableLoopProposal(IInvocationContext context
}

public static boolean getGenerateForLoopProposals(IInvocationContext context, ASTNode coveringNode, IProblemLocation[] locations, Collection<ICommandAccess> resultingCollections) {
Statement statement= ASTResolving.findParentStatement(coveringNode);
if (!(statement instanceof ExpressionStatement)) {
return false;
}

if (containsMatchingProblem(locations, IProblem.ParsingErrorInsertToComplete))
return false;

Expression expression= ((ExpressionStatement) statement).getExpression();
Statement statement= ASTResolving.findParentStatement(coveringNode);
ICompilationUnit cu= context.getCompilationUnit();
ITypeBinding expressionType= null;
Expression expression= null;
int relevanceBoost= 0;

if (expression instanceof MethodInvocation
|| expression instanceof SimpleName
|| expression instanceof FieldAccess) {
expressionType= expression.resolveTypeBinding();
} else if (expression instanceof Assignment
&& ((Assignment) expression).getRightHandSide().resolveTypeBinding() == null
&& ((Assignment) expression).getLeftHandSide().resolveTypeBinding() != null) {
expressionType= ((Assignment) expression).getLeftHandSide().resolveTypeBinding();
if (statement instanceof ExpressionStatement) {
expressionType= extractExpressionType((ExpressionStatement) statement);
expression= ((ExpressionStatement) statement).getExpression();

} else if (statement instanceof VariableDeclarationStatement && ((VariableDeclarationStatement) statement).fragments().size() == 1
&& ((VariableDeclarationStatement) statement).fragments().get(0).toString().equals("$missing$")) { //$NON-NLS-1$
// variable name is resolved to the type in a variable declaration statement, see https://bugs.eclipse.org/430336
Type type= ((VariableDeclarationStatement) statement).getType();
if (type instanceof SimpleType) {
SimpleType simpleType= (SimpleType) type;
expressionType= extractExpressionType(simpleType);
expression= simpleType.getName();
relevanceBoost+= 6; // need to overrule the bad UnresolvedElementsSubProcessor#addNewTypeProposals(..)
}
} else {
return false;
}

if (expressionType == null)
Expand All @@ -2769,25 +2776,63 @@ public static boolean getGenerateForLoopProposals(IInvocationContext context, AS
if (Bindings.findTypeInHierarchy(expressionType, "java.lang.Iterable") != null) { //$NON-NLS-1$
if (resultingCollections == null)
return true;
resultingCollections.add(new GenerateForLoopAssistProposal(cu, statement, expression, GenerateForLoopAssistProposal.GENERATE_ITERATOR_FOR));
GenerateForLoopAssistProposal proposal= new GenerateForLoopAssistProposal(cu, expressionType, statement, expression, GenerateForLoopAssistProposal.GENERATE_ITERATOR_FOR);
proposal.setRelevance(proposal.getRelevance() + relevanceBoost);
resultingCollections.add(proposal);
if (Bindings.findTypeInHierarchy(expressionType, "java.util.List") != null) { //$NON-NLS-1$
resultingCollections.add(new GenerateForLoopAssistProposal(cu, statement, expression, GenerateForLoopAssistProposal.GENERATE_ITERATE_LIST));
proposal= new GenerateForLoopAssistProposal(cu, expressionType, statement, expression, GenerateForLoopAssistProposal.GENERATE_ITERATE_LIST);
proposal.setRelevance(proposal.getRelevance() + relevanceBoost);
resultingCollections.add(proposal);
}
} else if (expressionType.isArray()) {
if (resultingCollections == null)
return true;
resultingCollections.add(new GenerateForLoopAssistProposal(cu, statement, expression, GenerateForLoopAssistProposal.GENERATE_ITERATE_ARRAY));
GenerateForLoopAssistProposal proposal= new GenerateForLoopAssistProposal(cu, expressionType, statement, expression, GenerateForLoopAssistProposal.GENERATE_ITERATE_ARRAY);
proposal.setRelevance(proposal.getRelevance() + relevanceBoost);
resultingCollections.add(proposal);
} else {
return false;
}

if (JavaModelUtil.is50OrHigher(cu.getJavaProject())) {
resultingCollections.add(new GenerateForLoopAssistProposal(cu, statement, expression, GenerateForLoopAssistProposal.GENERATE_FOREACH));
GenerateForLoopAssistProposal proposal= new GenerateForLoopAssistProposal(cu, expressionType, statement, expression, GenerateForLoopAssistProposal.GENERATE_FOREACH);
proposal.setRelevance(proposal.getRelevance() + relevanceBoost);
resultingCollections.add(proposal);
}

return true;
}

private static ITypeBinding extractExpressionType(ExpressionStatement statement) {
Expression expression= statement.getExpression();
if (expression.getNodeType() == ASTNode.METHOD_INVOCATION
|| expression.getNodeType() == ASTNode.FIELD_ACCESS) {
return expression.resolveTypeBinding();
}
return null;
}

private static ITypeBinding extractExpressionType(SimpleType variableIdentifier) {
ScopeAnalyzer analyzer= new ScopeAnalyzer((CompilationUnit) variableIdentifier.getRoot());
// get the name of the variable to search the type for
Name name= null;
if (variableIdentifier.getName() instanceof SimpleName) {
name= variableIdentifier.getName();
} else if (variableIdentifier.getName() instanceof QualifiedName) {
name= ((QualifiedName) variableIdentifier.getName()).getName();
}

// analyze variables which are available in the scope at the position of the quick assist invokation
IBinding[] declarationsInScope= analyzer.getDeclarationsInScope(name.getStartPosition(), ScopeAnalyzer.VARIABLES);
for (int i= 0; i < declarationsInScope.length; i++) {
IBinding currentVariable= declarationsInScope[i];
if (((IVariableBinding) currentVariable).getName().equals(name.getFullyQualifiedName())) {
return ((IVariableBinding) currentVariable).getType();
}
}
return null;
}

private static ForStatement getEnclosingForStatementHeader(ASTNode node) {
return getEnclosingHeader(node, ForStatement.class, ForStatement.INITIALIZERS_PROPERTY, ForStatement.EXPRESSION_PROPERTY, ForStatement.UPDATERS_PROPERTY);
}
Expand Down
Loading

0 comments on commit ff852ca

Please sign in to comment.