Bug 440447 - ASTRewrite Loses Nested Modifications

Modified ChangeGeneratorWriterVisitor to cope with nested modifications
as described in the ticket.

Also added some related change generator tests to the corresponding
tests suites.

Refined change to cope with sibling changes too.

Change-Id: Ia04f3d01e50375828588e7fa65e2ec3782c58e79
Signed-off-by: Thomas Corbat <tcorbat@hsr.ch>
Reviewed-on: https://git.eclipse.org/r/30525
Tested-by: Hudson CI
Reviewed-by: Sergey Prigogin <eclipse.sprigogin@gmail.com>

core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/rewrite/changegenerator/replace/CtorChainInitializerTest.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2008, 2010 Institute for Software, HSR Hochschule fuer Technik  
+ * Copyright (c) 2008, 2014 Institute for Software, HSR Hochschule fuer Technik  
  * Rapperswil, University of applied sciences and others
  * All rights reserved. This program and the accompanying materials 
  * are made available under the terms of the Eclipse Public License v1.0 
@@ -33,8 +33,15 @@ public class CtorChainInitializerTest extends ChangeGeneratorTest {
 
 	@Override
 	protected void setUp() throws Exception {
-		source = "TestClass::TestClass(int a):beta(b){\n}\n\n"; //$NON-NLS-1$
+		source = 
-		expectedSource = "TestClass::TestClass(int a): alpha(a){\n}\n\n"; //$NON-NLS-1$
+				"TestClass::TestClass(int a):beta(b){\n" +
+				"}\n" +
+				"\n";
+		expectedSource =
+				"TestClass::TestClass(int a) :\n" +
+				"		alpha(a) {\n" +
+				"}\n" +
+				"\n";
 		super.setUp();
 	}
 

core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/rewrite/changegenerator/replace/MultilineWhitespaceHandlingTest.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011 Institute for Software, HSR Hochschule fuer Technik  
+ * Copyright (c) 2011, 2014 Institute for Software, HSR Hochschule fuer Technik  
  * Rapperswil, University of applied sciences and others
  * All rights reserved. This program and the accompanying materials 
  * are made available under the terms of the Eclipse Public License v1.0 
@@ -24,13 +24,23 @@ import org.eclipse.cdt.internal.core.dom.rewrite.ASTModificationStore;
 public class MultilineWhitespaceHandlingTest extends ChangeGeneratorTest {
 	
 	public MultilineWhitespaceHandlingTest(){
-		super("Whitespace Handling in Replace"); //$NON-NLS-1$
+		super("Multiline Whitespace Handling in Replace"); //$NON-NLS-1$
 	}
 
 	@Override
 	protected void setUp() throws Exception {
-		source = "void foo(){\r\n\r\n  for(int i = 0; i < 10; i++){\r\n\r\n  }\r\n\r\n"; //$NON-NLS-1$
+		source =
-		expectedSource = "void foo(){\r\n\r\n  for(int i = 0; i < 10; i++){    int i;\r\n    int j;\r\n\r\n  }\r\n\r\n"; //$NON-NLS-1$
+				"void foo() {\n" +
+				"\n" +
+				"  for(int i = 0; i < 10; i++){\n" +
+				"\n" +
+				"  }\n" +
+				"\n";
+		expectedSource = 
+				"void foo() {\n" +
+				"	for (int i = 0; i < 10; i++) {\n" +
+				"	}\n" +
+				"}";
 		super.setUp();
 	}
 

core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/rewrite/changegenerator/replace/NestedReplaceTest.java

@@ -0,0 +1,88 @@
+/*******************************************************************************
+ * Copyright (c) 2014 Institute for Software, HSR Hochschule fuer Technik
+ * Rapperswil, University of applied sciences and others
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Thomas Corbat (IFS) - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.cdt.core.parser.tests.rewrite.changegenerator.replace;
+
+import junit.framework.Test;
+
+import org.eclipse.cdt.core.dom.ast.ASTVisitor;
+import org.eclipse.cdt.core.dom.ast.IASTCompoundStatement;
+import org.eclipse.cdt.core.dom.ast.IASTExpression;
+import org.eclipse.cdt.core.dom.ast.IASTExpressionStatement;
+import org.eclipse.cdt.core.dom.ast.IASTName;
+import org.eclipse.cdt.core.dom.ast.IASTNullStatement;
+import org.eclipse.cdt.core.dom.ast.IASTStatement;
+import org.eclipse.cdt.core.dom.ast.IASTUnaryExpression;
+import org.eclipse.cdt.core.dom.ast.INodeFactory;
+import org.eclipse.cdt.core.parser.tests.rewrite.changegenerator.ChangeGeneratorTest;
+import org.eclipse.cdt.internal.core.dom.rewrite.ASTModification;
+import org.eclipse.cdt.internal.core.dom.rewrite.ASTModification.ModificationKind;
+import org.eclipse.cdt.internal.core.dom.rewrite.ASTModificationStore;
+
+public class NestedReplaceTest extends ChangeGeneratorTest {
+
+	public NestedReplaceTest() {
+		super("Nested Replace"); //$NON-NLS-1$
+	}
+
+	@Override
+	protected void setUp() throws Exception {
+		source =
+				"void foo(int x) {\n" +
+				"	x += 1;\n" +
+				"}";
+		expectedSource = 
+				"void foo(int x) {\n" +
+				"	x++;\n" +
+				"}";
+		super.setUp();
+	}
+
+	public static Test suite() {
+		return new NestedReplaceTest();
+	}
+
+	@Override
+	protected ASTVisitor createModificator(final ASTModificationStore modStore) {
+		return new ASTVisitor() {
+			{
+				shouldVisitStatements = true;
+			}
+
+			@Override
+			public int visit(IASTStatement statement) {
+				if (statement instanceof IASTCompoundStatement) {
+					IASTCompoundStatement compoundStatement = (IASTCompoundStatement) statement;
+					INodeFactory factory = statement.getTranslationUnit().getASTNodeFactory();
+
+					IASTCompoundStatement newCompoundStatement = factory.newCompoundStatement();
+					IASTNullStatement dummyStatement = factory.newNullStatement();
+					newCompoundStatement.addStatement(dummyStatement);
+					ASTModification compoundReplacement = new ASTModification(ModificationKind.REPLACE, compoundStatement, newCompoundStatement, null);
+					modStore.storeModification(null, compoundReplacement);
+
+					IASTName emptyName = factory.newName();
+					IASTExpression idExpression = factory.newIdExpression(emptyName);
+					IASTExpression incrementExpression = factory.newUnaryExpression(IASTUnaryExpression.op_postFixIncr, idExpression);
+					IASTExpressionStatement newStatement = factory.newExpressionStatement(incrementExpression);
+					IASTStatement replacedStatement = compoundStatement.getStatements()[0];
+					ASTModification statementModification = new ASTModification(ModificationKind.REPLACE, dummyStatement, newStatement, null);
+					modStore.storeModification(compoundReplacement, statementModification);
+
+					IASTName xName = factory.newName("x".toCharArray());
+					ASTModification nameModification = new ASTModification(ModificationKind.REPLACE, emptyName, xName, null);
+					modStore.storeModification(statementModification, nameModification);
+				}
+				return PROCESS_ABORT;
+			}
+		};
+	}
+}

core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/rewrite/changegenerator/replace/ReplaceForLoopBodyTest.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011 Institute for Software, HSR Hochschule fuer Technik  
+ * Copyright (c) 2011, 2014 Institute for Software, HSR Hochschule fuer Technik  
  * Rapperswil, University of applied sciences and others
  * All rights reserved. This program and the accompanying materials 
  * are made available under the terms of the Eclipse Public License v1.0 
@@ -30,8 +30,30 @@ public class ReplaceForLoopBodyTest extends ChangeGeneratorTest {
 
 	@Override
 	protected void setUp() throws Exception {
-		source = "void foo(){\r\n\r\n  for(int i = 0; i < 10; i++){\r\n\r\n  }\r\n\r\n  for(int j = 0; j < 10; j++){\r\n\r\n  }\r\n\r\n}"; //$NON-NLS-1$
+		source =
-		expectedSource = "void foo(){\r\n\r\n  for(;;);\r\n\r\n  for(int j = 0; j < 10; j++){\r\n\r\n  }\r\n\r\n}"; //$NON-NLS-1$
+				"void foo() {\n" +
+				"\n" +
+				"  for(int i = 0; i < 10; i++){\n" +
+				"\n" +
+				"  }\n" +
+				"\n" +
+				"  for(int j = 0; j < 10; j++){\n" +
+				"\n" +
+				"  }\n" +
+				"\n" +
+				"}";
+		expectedSource =
+				"void foo() {\n" +
+				"\n" +
+				"	for (;;)\n" +
+				"		;\n" +
+				"\n" +
+				"\n" +
+				"  for(int j = 0; j < 10; j++){\n" +
+				"\n" +
+				"  }\n" +
+				"\n" +
+				"}";
 		forReplaced = false;
 		super.setUp();
 	}

core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/rewrite/changegenerator/replace/ReplaceTestSuite.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2008, 2011 Institute for Software, HSR Hochschule fuer Technik  
+ * Copyright (c) 2008, 2014 Institute for Software, HSR Hochschule fuer Technik  
  * Rapperswil, University of applied sciences and others
  * All rights reserved. This program and the accompanying materials 
  * are made available under the terms of the Eclipse Public License v1.0 
@@ -22,20 +22,25 @@ public class ReplaceTestSuite {
 	public static Test suite() throws Exception {
 		TestSuite suite = new TestSuite("ReplaceTestSuite");
 	
-		suite.addTest(NameTest.suite());
-		suite.addTest(MoveTest.suite());
-		suite.addTest(MoveRenameTest.suite());
-		suite.addTest(SameNameTest.suite());
-		suite.addTest(IdenticalTest.suite());
-		suite.addTest(PointerInParameterTest.suite());
-		suite.addTest(ExceptionTest.suite());
 		suite.addTest(ArrayModifierTest.suite());
-		suite.addTest(InitializerTest.suite());
-		suite.addTest(ExpressionTest.suite());
 		suite.addTest(ArraySizeExpressionTest.suite());
+		suite.addTest(CtorChainInitializerTest.suite());
+		suite.addTest(ExceptionTest.suite());
+		suite.addTest(ExpressionTest.suite());
+		suite.addTest(IdenticalTest.suite());
+		suite.addTest(InitializerTest.suite());
+		suite.addTest(MoveRenameTest.suite());
+		suite.addTest(MoveTest.suite());
+		suite.addTest(MultilineWhitespaceHandlingTest.suite());
+		suite.addTest(NameTest.suite());
+		suite.addTest(NestedReplaceTest.suite());
 		suite.addTest(NewInitializerExpressionTest.suite());
-		suite.addTest(StatementTest.suite());
+		suite.addTest(PointerInParameterTest.suite());
+		suite.addTest(ReplaceForLoopBodyTest.suite());
 		suite.addTest(ReplaceInsertStatementTest.suite());
+		suite.addTest(SameNameTest.suite());
+		suite.addTest(StatementTest.suite());
+		suite.addTest(WhitespaceHandlingTest.suite());
 		return suite;
 	}
 }

core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/rewrite/changegenerator/replace/WhitespaceHandlingTest.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011 Institute for Software, HSR Hochschule fuer Technik  
+ * Copyright (c) 2011, 2014 Institute for Software, HSR Hochschule fuer Technik  
  * Rapperswil, University of applied sciences and others
  * All rights reserved. This program and the accompanying materials 
  * are made available under the terms of the Eclipse Public License v1.0 
@@ -36,8 +36,30 @@ public class WhitespaceHandlingTest extends ChangeGeneratorTest {
 
 	@Override
 	protected void setUp() throws Exception {
-		source = "void foo(){\r\n\r\n  for(int i = 0; i < 10; i++){\r\n\r\n  }\r\n\r\n  for(int j = 0; j < 10; j++){\r\n\r\n  }\r\n\r\n}"; //$NON-NLS-1$
+		source =
-		expectedSource = "void foo(){\r\n\r\n  for(int i = 0; i < 10; i++);\r\n\r\n  for(int j = 0; j < 10; j++){\r\n\r\n  }\r\n\r\n}"; //$NON-NLS-1$
+				"void foo() {\n" +
+				"\n" +
+				"  for(int i = 0; i < 10; i++){\n" +
+				"\n" +
+				"  }\n" +
+				"\n" +
+				"  for(int j = 0; j < 10; j++){\n" +
+				"\n" +
+				"  }\n" +
+				"\n" +
+				"}";
+		expectedSource =
+				"void foo() {\n" +
+				"\n" +
+				"	for (int i = 0; i < 10; i++)\n" +
+				"		;\n" +
+				"\n" +
+				"\n" +
+				"  for(int j = 0; j < 10; j++){\n" +
+				"\n" +
+				"  }\n" +
+				"\n" +
+				"}";
 		forReplaced = false;
 		super.setUp();
 	}

core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/rewrite/changegenerator/ChangeGeneratorWriterVisitor.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2008, 2011 Institute for Software, HSR Hochschule fuer Technik
+ * Copyright (c) 2008, 2014 Institute for Software, HSR Hochschule fuer Technik
  * Rapperswil, University of applied sciences and others
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -9,6 +9,7 @@
  * Contributors:
  *     Institute for Software - initial API and implementation
  *     Markus Schorn (Wind River Systems)
+ *     Thomas Corbat (IFS)
  *******************************************************************************/
 package org.eclipse.cdt.internal.core.dom.rewrite.changegenerator;
 
@@ -363,19 +364,16 @@ public class ChangeGeneratorWriterVisitor extends ASTWriterVisitor {
 				return PROCESS_SKIP;
 			}
 		}
-
 		// Check all insert before and append modifications for the current node.
 		// If necessary put it onto the stack.
 		for (IASTNode currentModifiedNode : stack.getModifiedNodes()) {
 			for (ASTModification currentMod : stack.getModificationsForNode(currentModifiedNode)) {
-				if (currentMod.getNewNode() == node) {
+				if (currentMod.getNewNode() == node && currentMod.getKind() != ModificationKind.REPLACE) {
-					if (currentMod.getKind() != ModificationKind.REPLACE) {
 					stack.pushScope(currentModifiedNode);
 					return PROCESS_CONTINUE;
 				}
 			}
 		}
-		}
 		// Check all replace modifications for the current node. Visit the replacing node if found.
 		for (IASTNode currentModifiedNode : stack.getModifiedNodes()) {
 			for (ASTModification currentMod : stack.getModificationsForNode(currentModifiedNode)) {
@@ -390,6 +388,17 @@ public class ChangeGeneratorWriterVisitor extends ASTWriterVisitor {
 			}
 		}
 
+		// Check replace modifications for the current node. Is required as nodes could have been replaced
+		// externally, e.g. in ASTModificationHelper.
+		for (IASTNode currentModifiedNode : stack.getModifiedNodes()) {
+			for (ASTModification currentMod : stack.getModificationsForNode(currentModifiedNode)) {
+				if (currentMod.getNewNode() == node && currentMod.getKind() == ModificationKind.REPLACE) {
+					stack.pushScope(currentModifiedNode);
+					return PROCESS_CONTINUE;
+				}
+			}
+		}
+
 		return PROCESS_CONTINUE;
 	}