Bug 532120: Catch by const reference ignores const placement setting

The original implementation used plain-text string manipulation of the
IDocument. This changeset changes the implementation to make use of the
ASTRewrite infrastructure, which automatically honors the const
placement setting.

Change-Id: Ib5ae9381b93ca8ab4d1ad3e16b1c3c8b1ec62d78
Signed-off-by: Felix Morgner <fmorgner@hsr.ch>

codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByConstReferenceQuickFix.java

@@ -11,22 +11,26 @@
  *******************************************************************************/
 package org.eclipse.cdt.codan.internal.checkers.ui.quickfix;
 
+import java.util.Optional;
+
 import org.eclipse.cdt.codan.internal.checkers.ui.Messages;
-import org.eclipse.cdt.codan.ui.AbstractCodanCMarkerResolution;
+import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier;
-import org.eclipse.core.resources.IMarker;
+import org.eclipse.cdt.core.dom.ast.IASTNode.CopyStyle;
-import org.eclipse.jface.text.IDocument;
+import org.eclipse.cdt.core.dom.ast.IASTSimpleDeclaration;
 
 /**
  * Quick fix for catch by value
  */
-public class CatchByConstReferenceQuickFix extends AbstractCodanCMarkerResolution {
+public class CatchByConstReferenceQuickFix extends CatchByReferenceQuickFix {
 	@Override
 	public String getLabel() {
 		return Messages.CatchByConstReferenceQuickFix_Message;
 	}
 
-	@Override
+	protected Optional<IASTDeclSpecifier> getNewDeclSpecifier(IASTSimpleDeclaration declaration) {
-	public void apply(IMarker marker, IDocument document) {
+		IASTDeclSpecifier declSpecifier = declaration.getDeclSpecifier();
-		CatchByReferenceQuickFix.applyCatchByReferenceQuickFix(marker, document, true);
+		IASTDeclSpecifier replacement = declSpecifier.copy(CopyStyle.withLocations);
-	}
+		replacement.setConst(true);
+		return Optional.of(replacement);
+	}	
 }

codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFix.java

@@ -10,65 +10,97 @@
  *******************************************************************************/
 package org.eclipse.cdt.codan.internal.checkers.ui.quickfix;
 
+import java.util.Optional;
+
 import org.eclipse.cdt.codan.internal.checkers.ui.CheckersUiActivator;
 import org.eclipse.cdt.codan.internal.checkers.ui.Messages;
-import org.eclipse.cdt.codan.ui.AbstractCodanCMarkerResolution;
+import org.eclipse.cdt.codan.ui.AbstractAstRewriteQuickFix;
+import org.eclipse.cdt.core.dom.ast.ASTNodeFactoryFactory;
+import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier;
+import org.eclipse.cdt.core.dom.ast.IASTDeclarator;
+import org.eclipse.cdt.core.dom.ast.IASTNode;
+import org.eclipse.cdt.core.dom.ast.IASTNode.CopyStyle;
+import org.eclipse.cdt.core.dom.ast.IASTSimpleDeclaration;
+import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTReferenceOperator;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPNodeFactory;
+import org.eclipse.cdt.core.dom.rewrite.ASTRewrite;
+import org.eclipse.cdt.core.index.IIndex;
+import org.eclipse.cdt.core.model.ITranslationUnit;
 import org.eclipse.core.resources.IMarker;
-import org.eclipse.jface.text.BadLocationException;
+import org.eclipse.core.runtime.CoreException;
-import org.eclipse.jface.text.IDocument;
+import org.eclipse.core.runtime.NullProgressMonitor;
 
 /**
  * Quick fix for catch by value
  */
-public class CatchByReferenceQuickFix extends AbstractCodanCMarkerResolution {
+public class CatchByReferenceQuickFix extends AbstractAstRewriteQuickFix {
 	@Override
 	public String getLabel() {
 		return Messages.CatchByReferenceQuickFix_Message;
 	}
 
 	@Override
-	public void apply(IMarker marker, IDocument document) {
+	public void modifyAST(IIndex index, IMarker marker) {
-		applyCatchByReferenceQuickFix(marker, document, false);
+		IASTSimpleDeclaration declaration = getDeclaration(index, marker);
-	}
+		if (declaration == null) {
+			CheckersUiActivator.log("Could not find declaration"); //$NON-NLS-1$
+			return;
+		}
+
+		IASTTranslationUnit ast = declaration.getTranslationUnit();
+		ASTRewrite rewrite = ASTRewrite.create(ast);
+
+		getNewDeclSpecifier(declaration).ifPresent(ds -> rewrite.replace(declaration.getDeclSpecifier(), ds, null));
+		rewrite.replace(declaration.getDeclarators()[0], getNewDeclarator(declaration), null);
 
-	static void applyCatchByReferenceQuickFix(IMarker marker, IDocument document, boolean addConst) {
 		try {
-			int left = marker.getAttribute(IMarker.CHAR_START, -1);
+			rewrite.rewriteAST().perform(new NullProgressMonitor());
-			int right = marker.getAttribute(IMarker.CHAR_END, -1);
+		} catch (CoreException e) {
-			String inStr = document.get(left, right - left);
-			document.replace(left, right - left, getCatchByReferenceString(inStr, addConst));
-		} catch (BadLocationException e) {
 			CheckersUiActivator.log(e);
 		}
 	}
-	
+
 	/**
-	 * Returns a catch by reference string from a catch by value string
+	 * Calculate the new IASTDeclSpecifier for the changed declaration
+	 * <p>
+	 * Subclasses can override this method to provide custom behavior.
+	 * </p>
+	 * 
+	 * @param declaration The original declaration
+	 * @return A, possibly empty, {@link Optional} containing the new
+	 *         IASTDeclSpecifier
 	 */
-	static private String getCatchByReferenceString(String inStr, boolean addConst) {
+	protected Optional<IASTDeclSpecifier> getNewDeclSpecifier(IASTSimpleDeclaration declaration) {
-		StringBuilder stringBuilder = new StringBuilder(inStr.length() + 10);
+		return Optional.empty();
-		if (addConst) {
-			stringBuilder.append("const ");	 //$NON-NLS-1$
-		}
-		
-		String typename;
-		int space = inStr.lastIndexOf(' ');
-		boolean hasDeclName = space != -1;
-		if (hasDeclName) {
-			typename = inStr.substring(0,space);
-		} else {
-			typename = inStr;
-		}
-		stringBuilder.append(typename);
-		
-		stringBuilder.append(" &"); //$NON-NLS-1$
-		
-		if (hasDeclName) {
-			stringBuilder.append(" "); //$NON-NLS-1$
-			String declname = inStr.substring(space+1);
-			stringBuilder.append(declname);
-		}
-		
-		return stringBuilder.toString();
 	}
+
+	private static IASTDeclarator getNewDeclarator(IASTSimpleDeclaration declaration) {
+		ICPPNodeFactory nodeFactory = ASTNodeFactoryFactory.getDefaultCPPNodeFactory();
+		ICPPASTReferenceOperator reference = nodeFactory.newReferenceOperator(false);
+		IASTDeclarator declarator = declaration.getDeclarators()[0];
+		IASTDeclarator replacement = declarator.copy(CopyStyle.withLocations);
+		replacement.addPointerOperator(reference);
+		return replacement;
+	}
+
+	private IASTSimpleDeclaration getDeclaration(IIndex index, IMarker marker) {
+		try {
+			ITranslationUnit tu = getTranslationUnitViaEditor(marker);
+			IASTTranslationUnit ast = tu.getAST(index, ITranslationUnit.AST_SKIP_ALL_HEADERS);
+			int start = marker.getAttribute(IMarker.CHAR_START, -1);
+			int end = marker.getAttribute(IMarker.CHAR_END, -1);
+			if (start != -1 && end != -1) {
+				IASTNode node = ast.getNodeSelector(null).findNode(start, end - start);
+				while (node != null && !(node instanceof IASTSimpleDeclaration)) {
+					node = node.getParent();
+				}
+				return (IASTSimpleDeclaration) node;
+			}
+		} catch (CoreException e) {
+			CheckersUiActivator.log(e);
+		}
+		return null;
+	}
+
 }

codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CatchByReferenceQuickFixTest.java

@@ -12,6 +12,10 @@ package org.eclipse.cdt.codan.internal.checkers.ui.quickfix;
 
 import org.eclipse.cdt.codan.internal.checkers.CatchByReference;
 import org.eclipse.cdt.codan.ui.AbstractCodanCMarkerResolution;
+import org.eclipse.cdt.core.CCorePlugin;
+import org.eclipse.cdt.core.CCorePreferenceConstants;
+import org.eclipse.core.resources.ProjectScope;
+import org.eclipse.core.runtime.preferences.IEclipsePreferences;
 
 /**
  * @author Tomasz Wesolowski
@@ -29,6 +33,16 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase {
 		return true;
 	}
 
+	private boolean setPlaceConstRight(boolean placeRight) {
+		IEclipsePreferences node = new ProjectScope(cproject.getProject()).getNode(CCorePlugin.PLUGIN_ID);
+		boolean before = node.getBoolean(CCorePreferenceConstants.PLACE_CONST_RIGHT_OF_TYPE,
+				CCorePreferenceConstants.DEFAULT_PLACE_CONST_RIGHT_OF_TYPE);
+		node.putBoolean(CCorePreferenceConstants.PLACE_CONST_RIGHT_OF_TYPE, placeRight);
+		CCorePreferenceConstants.getPreference(CCorePreferenceConstants.PLACE_CONST_RIGHT_OF_TYPE, cproject,
+				CCorePreferenceConstants.DEFAULT_PLACE_CONST_RIGHT_OF_TYPE);
+		return before;
+	}
+
 	@Override
 	protected AbstractCodanCMarkerResolution createQuickFix() {
 		return null; // quick fix to be chosen per test
@@ -45,7 +59,7 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase {
 		setQuickFix(new CatchByReferenceQuickFix());
 		loadcode(getAboveComment());
 		String result = runQuickFixOneFile();
-		assertContainedIn("catch (C & exception)", result); //$NON-NLS-1$
+		assertContainedIn("catch (C &exception)", result); //$NON-NLS-1$
 	}
 
 	// struct C {
@@ -59,7 +73,7 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase {
 		setQuickFix(new CatchByReferenceQuickFix());
 		loadcode(getAboveComment());
 		String result = runQuickFixOneFile();
-		assertContainedIn("catch (C &)", result); //$NON-NLS-1$
+		assertContainedIn("catch (C&)", result); //$NON-NLS-1$
 	}
 
 	// struct C {
@@ -73,7 +87,25 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase {
 		setQuickFix(new CatchByConstReferenceQuickFix());
 		loadcode(getAboveComment());
 		String result = runQuickFixOneFile();
-		assertContainedIn("catch (const C & exception)", result); //$NON-NLS-1$
+		assertContainedIn("catch (const C &exception)", result); //$NON-NLS-1$
+	}
+
+	// struct C {
+	// };
+	// void foo() {
+	//    try {
+	//    } catch (C exception) {
+	//    }
+	// }
+	public void testCatchByConstReferenceHonorsConstPlacementSettings_532120() throws Exception {
+		setQuickFix(new CatchByConstReferenceQuickFix());
+		loadcode(getAboveComment());
+
+		boolean before = setPlaceConstRight(true);
+		String result = runQuickFixOneFile();
+		setPlaceConstRight(before);
+
+		assertContainedIn("catch (C const &exception)", result); //$NON-NLS-1$
 	}
 
 	// struct C {
@@ -87,6 +119,25 @@ public class CatchByReferenceQuickFixTest extends QuickFixTestCase {
 		setQuickFix(new CatchByConstReferenceQuickFix());
 		loadcode(getAboveComment());
 		String result = runQuickFixOneFile();
-		assertContainedIn("catch (const C &)", result); //$NON-NLS-1$
+		assertContainedIn("catch (const C&)", result); //$NON-NLS-1$
 	}
+
+	// struct C {
+	// };
+	// void foo() {
+	//    try {
+	//    } catch (C) {
+	//    }
+	// }
+	public void testCatchByConstReferenceNoDeclNameHonorsConstPlacementSettings_532120() throws Exception {
+		setQuickFix(new CatchByConstReferenceQuickFix());
+		loadcode(getAboveComment());
+
+		boolean before = setPlaceConstRight(true);
+		String result = runQuickFixOneFile();
+		setPlaceConstRight(before);
+
+		assertContainedIn("catch (C const &)", result); //$NON-NLS-1$
+	}
+
 }