Bug 489468 - Extract Function creates illegal declaration in .h when

there is a using statement in the .cpp for an argument type

Change-Id: Ie54ce13b434bab21f96b0c6bb7347846d52314e0

core/org.eclipse.cdt.ui.tests/ui/org/eclipse/cdt/ui/tests/refactoring/extractfunction/ExtractFunctionRefactoringTest.java

@@ -16,8 +16,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-import junit.framework.Test;
-
 import org.eclipse.cdt.ui.PreferenceConstants;
 import org.eclipse.cdt.ui.tests.refactoring.RefactoringTestBase;
 
@@ -27,6 +25,8 @@ import org.eclipse.cdt.internal.ui.refactoring.extractfunction.ExtractFunctionIn
 import org.eclipse.cdt.internal.ui.refactoring.extractfunction.ExtractFunctionRefactoring;
 import org.eclipse.cdt.internal.ui.refactoring.utils.VisibilityEnum;
 
+import junit.framework.Test;
+
 /**
  * Tests for Extract Function refactoring.
  */
@@ -35,12 +35,12 @@ public class ExtractFunctionRefactoringTest extends RefactoringTestBase {
 	private String extractedFunctionName = "extracted";
 	private String returnValue;
 	// Map from old names to new ones.
-	private Map<String, String> parameterRename = new HashMap<>();
+	private final Map<String, String> parameterRename = new HashMap<>();
 	// New positions of parameters, or null.
 	private int[] parameterOrder;
 	private VisibilityEnum visibility = VisibilityEnum.v_private;
 	private boolean virtual;
-	private boolean replaceDuplicates = true;
+	private final boolean replaceDuplicates = true;
 	private ExtractFunctionRefactoring refactoring;
 
 	public ExtractFunctionRefactoringTest() {
@@ -570,6 +570,79 @@ public class ExtractFunctionRefactoringTest extends RefactoringTestBase {
 		assertRefactoringSuccess();
 	}
 
+	//A.h
+	//#ifndef A_H_
+	//#define A_H_
+	//
+	//#include "B.h"
+	//
+	//class A {
+	//public:
+	//	void foo();
+	//};
+	//
+	//#endif /*A_H_*/
+	//====================
+	//#ifndef A_H_
+	//#define A_H_
+	//
+	//#include "B.h"
+	//
+	//class A {
+	//public:
+	//	void foo();
+	//
+	//private:
+	//	ns1::ns2::B extracted(ns1::ns2::B b);
+	//};
+	//
+	//#endif /*A_H_*/
+
+	//A.cpp
+	//#include "A.h"
+	//#include "B.h"
+	//
+	//using ns1::ns2::B;
+	//
+	//void A::foo() {
+	//	B b;
+	//	/*$*/b.m();/*$$*/
+	//	b.n();
+	//}
+	//====================
+	//#include "A.h"
+	//#include "B.h"
+	//
+	//using ns1::ns2::B;
+	//
+	//B A::extracted(B b) {
+	//	b.m();
+	//	return b;
+	//}
+	//
+	//void A::foo() {
+	//	B b;
+	//	b = extracted(b);
+	//	b.n();
+	//}
+
+	//B.h
+	//#ifndef B_H_
+	//#define B_H_
+	//
+	//namespace ns1 { namespace ns2 {
+	//struct B {
+	//  void m();
+	//  void n() const;
+	//};
+	//}}
+	//
+	//#endif /*B_H_*/
+	public void testUsingDeclaration() throws Exception {
+		getPreferenceStore().setValue(PreferenceConstants.FUNCTION_PASS_OUTPUT_PARAMETERS_BY_POINTER, true);
+		assertRefactoringSuccess();
+	}
+
 	//A.cpp
 	//void test() {
 	//	for (int i = 0; i < 2; i++) {

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/ClassMemberInserter.java

@@ -90,27 +90,57 @@ public class ClassMemberInserter {
 	private ClassMemberInserter() {
 	}
 
-	public static void createChange(ICPPASTCompositeTypeSpecifier classNode,
+	/**
+	 * Inserts a node inside a class declaration.
+	 *
+	 * @param classNode the type specifier of the class declaration
+	 * @param visibility desired visibility of the inserted declaration
+	 * @param nodeToAdd the node to add
+	 * @param isField whether the node being inserted is a field declaration or not
+	 * @param collector the modification collector recording the insertion
+	 * @return the rewriter for making further modifications to the inserted node
+	 */
+	public static ASTRewrite createChange(ICPPASTCompositeTypeSpecifier classNode,
 			VisibilityEnum visibility, IASTNode nodeToAdd, boolean isField,
 			ModificationCollector collector) {
+		List<ASTRewrite> addedNodesRewrites =
 				createChange(classNode, visibility,	Collections.singletonList(nodeToAdd), isField, collector);
+		return addedNodesRewrites.get(0);
 	}
 
-	public static void createChange(ICPPASTCompositeTypeSpecifier classNode,
+	/**
+	 * Inserts one more adjacent nodes inside a class declaration.
+	 *
+	 * @param classNode the type specifier of the class declaration
+	 * @param visibility desired visibility of the inserted declarations
+	 * @param nodesToAdd the nodes to add
+	 * @param isField whether the nodes being inserted are field declaration or not
+	 * @param collector the modification collector recording the insertion
+	 * @return the rewriters for making further modifications to the inserted nodes
+	 */
+	public static List<ASTRewrite> createChange(ICPPASTCompositeTypeSpecifier classNode,
 			VisibilityEnum visibility, List<IASTNode> nodesToAdd, boolean isField,
 			ModificationCollector collector) {
 		InsertionInfo info = findInsertionPoint(classNode, visibility, isField);
-		nodesToAdd = new ArrayList<IASTNode>(nodesToAdd);
-		if (info.getPrologue() != null)
-			nodesToAdd.add(0, info.getPrologue());
-		if (info.getEpilogue() != null)
-			nodesToAdd.add(info.getEpilogue());
-
 		ASTRewrite rewrite = collector.rewriterForTranslationUnit(classNode.getTranslationUnit());
-		for (IASTNode node : nodesToAdd) {
+
-			rewrite.insertBefore(info.getParentNode(), info.getInsertBeforeNode(), node,
+		if (info.getPrologue() != null) {
+			rewrite.insertBefore(info.getParentNode(), info.getInsertBeforeNode(), info.getPrologue(),
 					createEditDescription(classNode));
 		}
+
+		List<ASTRewrite> addedNodeRewrites = new ArrayList<>(nodesToAdd.size());
+		for (IASTNode node : nodesToAdd) {
+			addedNodeRewrites.add(rewrite.insertBefore(info.getParentNode(), info.getInsertBeforeNode(), node,
+					createEditDescription(classNode)));
+		}
+
+		if (info.getEpilogue() != null) {
+			rewrite.insertBefore(info.getParentNode(), info.getInsertBeforeNode(), info.getEpilogue(),
+					createEditDescription(classNode));
+		}
+
+		return addedNodeRewrites;
 	}
 
 	public static InsertionInfo findInsertionPoint(ICPPASTCompositeTypeSpecifier classNode,

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractfunction/ExtractFunctionRefactoring.java

@@ -13,6 +13,7 @@
 package org.eclipse.cdt.internal.ui.refactoring.extractfunction;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -34,6 +35,7 @@ import org.eclipse.osgi.util.NLS;
 import org.eclipse.text.edits.TextEditGroup;
 
 import org.eclipse.cdt.core.dom.ast.ASTVisitor;
+import org.eclipse.cdt.core.dom.ast.DOMException;
 import org.eclipse.cdt.core.dom.ast.IASTBinaryExpression;
 import org.eclipse.cdt.core.dom.ast.IASTComment;
 import org.eclipse.cdt.core.dom.ast.IASTCompoundStatement;
@@ -77,6 +79,7 @@ import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTQualifiedName;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTTemplateDeclaration;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTTemplateId;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTTemplateParameter;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPBinding;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPClassType;
 import org.eclipse.cdt.core.dom.ast.cpp.ICPPMethod;
 import org.eclipse.cdt.core.dom.rewrite.ASTRewrite;
@@ -129,6 +132,7 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 	public static final String ID =
 			"org.eclipse.cdt.internal.ui.refactoring.extractfunction.ExtractFunctionRefactoring"; //$NON-NLS-1$
 
+	private static final String[] EMPTY_STRING_ARRAY = {};
 	static final Integer NULL_INTEGER = Integer.valueOf(0);
 
 	private NodeContainer container;
@@ -312,11 +316,11 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 		final IASTName methodName = new CPPASTName(info.getMethodName().toCharArray());
 		MethodContext context = info.getMethodContext();
 
-		// Create declaration in class
+		// Create declaration in class.
 		if (context.getType() == ContextType.METHOD && !context.isInline()) {
 			createMethodDeclaration(methodName, context, collector);
 		}
-		// Create method definition
+		// Create method definition.
 		IASTNode firstNode = container.getNodesToWrite().get(0);
 		createMethodDefinition(methodName, context, firstNode, collector);
 
@@ -408,12 +412,77 @@ public class ExtractFunctionRefactoring extends CRefactoring {
 
 		IASTSimpleDeclaration methodDeclaration = getDeclaration(collector, astMethodName);
 
-		ClassMemberInserter.createChange(classDeclaration, info.getVisibility(),
+		ASTRewrite rewrite = ClassMemberInserter.createChange(classDeclaration, info.getVisibility(),
 				methodDeclaration, false, collector);
+
+		// Names of external bindings may have to be qualified to be used in a header file.
+		if (classDeclaration.getTranslationUnit().isHeaderUnit())
+			qualifyExternalReferences(methodDeclaration, classDeclaration, rewrite);
+	}
+
+	private void qualifyExternalReferences(IASTNode node, ICPPASTCompositeTypeSpecifier classDeclaration,
+			final ASTRewrite rewrite) {
+		final ICPPBinding owner = (ICPPBinding) classDeclaration.getName().resolveBinding();
+		final String[] contextQualifiers;
+		try {
+			contextQualifiers = owner.getQualifiedName();
+		} catch (DOMException e) {
+			CUIPlugin.log(e);
+			return;
+		}
+
+		node.accept(new ASTVisitor(true) {
+			@Override
+			public int visit(IASTName name) {
+				qualifyForContext((ICPPASTName) name, contextQualifiers, rewrite);
+				return PROCESS_SKIP; // Do non visit internals of qualified names.
+			}
+		});
+	}
+
+	private void qualifyForContext(ICPPASTName name, String[] contextQualifiers, ASTRewrite rewrite) {
+		ICPPASTName originalName = (ICPPASTName) name.getOriginalNode();
+		IBinding binding = originalName.resolveBinding();
+		try {
+			if (!(binding instanceof ICPPBinding))
+				return; // Qualification is not needed.
+			String[] names = ((ICPPBinding) binding).getQualifiedName();
+			names = removeCommonPrefix(names, contextQualifiers);
+			if (names.length <= 1)
+				return; // Qualification is not needed.
+
+			ICPPASTQualifiedName qualifiedName;
+			if (name instanceof ICPPASTQualifiedName) {
+				qualifiedName = (ICPPASTQualifiedName) name;
+				if (qualifiedName.getQualifier().length >= names.length - 1)
+					return; // Qualified already.
+			} else {
+				qualifiedName = new CPPASTQualifiedName(name.copy(CopyStyle.withLocations));
+			}
+			for (int i = 0; i < names.length - qualifiedName.getQualifier().length; i++) {
+				qualifiedName.addNameSpecifier(new CPPASTName(names[i].toCharArray()));
+			}
+			if (!(name instanceof ICPPASTQualifiedName))
+				rewrite.replace(name, qualifiedName, null);
+		} catch (DOMException e) {
+			CUIPlugin.log(e);
+			return;
+		}
+	}
+
+	private String[] removeCommonPrefix(String[] array1, String[] array2) {
+		for (int i = 0; i < array1.length && i < array2.length; i++) {
+			if (!array1[i].equals(array2[i])) {
+				if (i == 0)
+					return array1;
+				return Arrays.copyOfRange(array1, i, array1.length);
+			}
+		}
+		return EMPTY_STRING_ARRAY;
 	}
 
 	private void replaceSimilar(ModificationCollector collector, IASTName methodName) {
-		// Find similar code
+		// Find similar code.
 		final List<IASTNode> nodesToRewriteWithoutComments = getNodesWithoutComments(container.getNodesToWrite());
 		final List<IASTNode> initTrail = getTrail(nodesToRewriteWithoutComments);
 		IASTTranslationUnit ast = nodesToRewriteWithoutComments.get(0).getTranslationUnit();