Community
Participate
Working Groups
With the following code: package test; import java.util.Arrays; import java.util.List; public class Test2 { public static void main(String[] args) { List<String> list = Arrays.asList("ZERO", "ONE", "TWO"); for (int i = 0; i < list.size(); i++) { System.out.println(list.get(i)); } } } JDT provides no proposal to convert it to foreach.
New Gerrit change created: https://git.eclipse.org/r/146018
Gerrit change https://git.eclipse.org/r/146018 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=ce2677be1d23a0c436216409b293ebc74498ecb2
Resolved for 4.13M3
New Gerrit change created: https://git.eclipse.org/r/146534
Gerrit change https://git.eclipse.org/r/146534 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=d643235a7af226f4eef364a64bcd2ae42bd1c886
I think this change caused a regression. I am using 4.13 M2 and when saving one of my java classes I get an error dialog saying that "save actions have failed" because "A save participant caused problems". The cause is this NPE: java.lang.NullPointerException at org.eclipse.jdt.internal.corext.fix.ConvertForLoopOperation.getIntroducedVariableName(ConvertForLoopOperation.java:657) at org.eclipse.jdt.internal.corext.fix.ConvertLoopFixCore$ControlStatementFinder.getConvertOperation(ConvertLoopFixCore.java:87) at org.eclipse.jdt.internal.corext.fix.ConvertLoopFixCore$ControlStatementFinder.visit(ConvertLoopFixCore.java:56) at org.eclipse.jdt.core.dom.ForStatement.accept0(ForStatement.java:212) at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2874) at org.eclipse.jdt.core.dom.ASTNode.acceptChildren(ASTNode.java:2945) at org.eclipse.jdt.core.dom.Block.accept0(Block.java:128) at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2874) at org.eclipse.jdt.core.dom.ASTNode.acceptChild(ASTNode.java:2922) at org.eclipse.jdt.core.dom.MethodDeclaration.accept0(MethodDeclaration.java:617) at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2874) at org.eclipse.jdt.core.dom.ASTNode.acceptChildren(ASTNode.java:2945) at org.eclipse.jdt.core.dom.TypeDeclaration.accept0(TypeDeclaration.java:447) at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2874) at org.eclipse.jdt.core.dom.ASTNode.acceptChildren(ASTNode.java:2945) at org.eclipse.jdt.core.dom.CompilationUnit.accept0(CompilationUnit.java:258) at org.eclipse.jdt.core.dom.ASTNode.accept(ASTNode.java:2874) at org.eclipse.jdt.internal.corext.fix.ConvertLoopFix.createCleanUp(ConvertLoopFix.java:43) at org.eclipse.jdt.internal.ui.fix.ConvertLoopCleanUp.createFix(ConvertLoopCleanUp.java:54) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring.calculateChange(CleanUpRefactoring.java:792) at org.eclipse.jdt.internal.corext.fix.CleanUpPostSaveListener.saved(CleanUpPostSaveListener.java:390) at org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitDocumentProvider$5.run(CompilationUnitDocumentProvider.java:1639) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45) In line 657: String[] proposals= getVariableNameProposals(fArrayAccess.resolveTypeBinding(), javaProject); fArrayAccess is null. Commit ce2677be1d23a0c did touch the validateLengthQuery method - it added third branch: } else if (lengthQuery instanceof MethodInvocation) { in the other two fArrayAccess and fArrayBinding is set, but it's not set in this new branch.
New Gerrit change created: https://git.eclipse.org/r/147819
(In reply to Eclipse Genie from comment #7) > New Gerrit change created: https://git.eclipse.org/r/147819 This change fixed my concrete use-case. I just did it by doing similar coding as in the other two branches. Don't know if it really is correct. As this is mainly copied code one should sind about refactoring this into a private method or somethings. Can a JDT commit pls. take over this change and finalize it?
(In reply to Matthias Becker from comment #8) > (In reply to Eclipse Genie from comment #7) > > New Gerrit change created: https://git.eclipse.org/r/147819 > > This change fixed my concrete use-case. > I just did it by doing similar coding as in the other two branches. Don't > know if it really is correct. > As this is mainly copied code one should sind about refactoring this into a > private method or somethings. > > Can a JDT commit pls. take over this change and finalize it? I'll take it. Please supply the details regarding the for loop that this broke for. It should be added as a test case
(In reply to Jeff Johnston from comment #9) > (In reply to Matthias Becker from comment #8) > > (In reply to Eclipse Genie from comment #7) > > > New Gerrit change created: https://git.eclipse.org/r/147819 > > > > This change fixed my concrete use-case. > > I just did it by doing similar coding as in the other two branches. Don't > > know if it really is correct. > > As this is mainly copied code one should sind about refactoring this into a > > private method or somethings. > > > > Can a JDT commit pls. take over this change and finalize it? > > I'll take it. Please supply the details regarding the for loop that this > broke for. It should be added as a test case I cannot provide the complete class. But the loop was this one: for (int i = 0; i < sortedProposals.size(); i++) { } Quite simple. Or is the body of the loop also relevant?
New Gerrit change created: https://git.eclipse.org/r/147846
(In reply to Matthias Becker from comment #10) > (In reply to Jeff Johnston from comment #9) > > (In reply to Matthias Becker from comment #8) > > > (In reply to Eclipse Genie from comment #7) > > > > New Gerrit change created: https://git.eclipse.org/r/147819 > > > > > > This change fixed my concrete use-case. > > > I just did it by doing similar coding as in the other two branches. Don't > > > know if it really is correct. > > > As this is mainly copied code one should sind about refactoring this into a > > > private method or somethings. > > > > > > Can a JDT commit pls. take over this change and finalize it? > > > > I'll take it. Please supply the details regarding the for loop that this > > broke for. It should be added as a test case > > I cannot provide the complete class. But the loop was this one: > > for (int i = 0; i < sortedProposals.size(); i++) { > } > > > Quite simple. Or is the body of the loop also relevant? To some degree - i.e. how the sortedProposals is referenced and how it is originally declared for the for loop to see is important (e.g. parameter, field, local variable). If you can reproduce with a simple test, that would be helpful as there are already tests in place that work fine. That said, I have posted a fix. The call to getVariableNameProposals should have been screened with a check for fisCollection like it is done elsewhere and a special collections version of the method should have been used. Please confirm that the fix works for you.
(In reply to Jeff Johnston from comment #12) > (In reply to Matthias Becker from comment #10) > > (In reply to Jeff Johnston from comment #9) > > > (In reply to Matthias Becker from comment #8) > > > > (In reply to Eclipse Genie from comment #7) > > > > > New Gerrit change created: https://git.eclipse.org/r/147819 > > > > > > > > This change fixed my concrete use-case. > > > > I just did it by doing similar coding as in the other two branches. Don't > > > > know if it really is correct. > > > > As this is mainly copied code one should sind about refactoring this into a > > > > private method or somethings. > > > > > > > > Can a JDT commit pls. take over this change and finalize it? > > > > > > I'll take it. Please supply the details regarding the for loop that this > > > broke for. It should be added as a test case > > > > I cannot provide the complete class. But the loop was this one: > > > > for (int i = 0; i < sortedProposals.size(); i++) { > > } > > > > > > Quite simple. Or is the body of the loop also relevant? > > To some degree - i.e. how the sortedProposals is referenced and how it is > originally declared for the for loop to see is important (e.g. parameter, > field, local variable). If you can reproduce with a simple test, that would > be helpful as there are already tests in place that work fine. This is a member of the class protected List<acme.DdlCompletion> semanticCompletions = new ArrayList<acme.IDdlParser.DdlCompletion>(); and this is a local variable just some line in front of the loop. List<AbstractDdlCodeCompletionProposal> sortedProposals = this.semanticCodeCompletionProposals; > Please confirm that the fix works for you. Yes the fixe works for me.
Gerrit change https://git.eclipse.org/r/147846 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=688926b42084deeb203e2cff3528ec03108976a6
(In reply to Eclipse Genie from comment #14) > Gerrit change https://git.eclipse.org/r/147846 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=688926b42084deeb203e2cff3528ec03108976a6 are you going to provide another gerrit that contains an automated test for that use case?
i(In reply to Matthias Becker from comment #15) > (In reply to Eclipse Genie from comment #14) > > Gerrit change https://git.eclipse.org/r/147846 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > > ?id=688926b42084deeb203e2cff3528ec03108976a6 > > are you going to provide another gerrit that contains an automated test for > that use case? I'll try and get it in today if possible.
New Gerrit change created: https://git.eclipse.org/r/147940
Gerrit change https://git.eclipse.org/r/147940 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=5cdb23a081339b2b7fcac2a081253895a1843fdd
(In reply to Matthias Becker from comment #15) > (In reply to Eclipse Genie from comment #14) > > Gerrit change https://git.eclipse.org/r/147846 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > > ?id=688926b42084deeb203e2cff3528ec03108976a6 > > are you going to provide another gerrit that contains an automated test for > that use case? Test merged. It was a path taken by adding as a cleanup that wasn't tested in the quickfix tests. I have added a cleanup test and verified it called the same method that was fixed to remove the NPE.
Released for 4.13M3
Verified for 4.13 M3 using build I20190821-0600
So far, this fix has resulted in two bugs that need to be fixed for RC2. Jeff, please revisit the fix for this bug and if it looks risky then we should revert it and add it back when the master branch opens for 4.14 so that we get more time before the release.
(In reply to Noopur Gupta from comment #22) > So far, this fix has resulted in two bugs that need to be fixed for RC2. > > Jeff, please revisit the fix for this bug and if it looks risky then we > should revert it and add it back when the master branch opens for 4.14 so > that we get more time before the release. I believe I have fixed the regressions but I agree that post-poning to 4.14 would ensure no more un-seen problems show up in the release as there is no time for users to test the latest version. There are multiple bugs/patches, should the files be reset in one patch or should there be multiple reverts? I am not sure what the process is here.
Should everything be reverted or just one of the patches?
(In reply to Alexander Kurtakov from comment #24) > Should everything be reverted or just one of the patches? One patch cannot be reverted. There has been refactoring in-between. I am going to restore the ConverForLoopOperation to a refactored version that does not support Collections and remove the tests. After 4.14, I will fix the bug that just came in regarding multiple Collections referenced. I will post a patch shortly after running tests and include Noopur as reviewer and set the appropriate flags.
(In reply to Jeff Johnston from comment #25) > I will post a patch shortly after running tests and include Noopur as > reviewer and set the appropriate flags. Sure. Also, please revert the corresponding N&N entry.
(In reply to Noopur Gupta from comment #26) > (In reply to Jeff Johnston from comment #25) > > I will post a patch shortly after running tests and include Noopur as > > reviewer and set the appropriate flags. > Sure. Also, please revert the corresponding N&N entry. Noopur, I also have a patch for the new foreach bug. I have added you as reviewer. If you would prefer that patch over the revert, let me know. Otherwise, I will post the revert patch and remove the N&N.
(In reply to Jeff Johnston from comment #27) > (In reply to Noopur Gupta from comment #26) > > (In reply to Jeff Johnston from comment #25) > > > I will post a patch shortly after running tests and include Noopur as > > > reviewer and set the appropriate flags. > > Sure. Also, please revert the corresponding N&N entry. > > Noopur, I also have a patch for the new foreach bug. I have added you as > reviewer. If you would prefer that patch over the revert, let me know. > Otherwise, I will post the revert patch and remove the N&N. Hi Noopur, Just to update. I have posted a request to the eclipse-pmc list to submit that patch to the blocking bug for RC2 and offered to revert if they wish. If you are ok with the changes, I will change this bug back to verified.
See 550672 comment #6.
Verified for 4.13RC2 using I20190904-1805 build
