From 47ceed3cbe729626bdf4138a03e518328bfc1c0d Mon Sep 17 00:00:00 2001 From: Felix Morgner Date: Wed, 7 Mar 2018 17:48:51 +0100 Subject: [PATCH] Bug 525250: "Create local variable" offered outside of local scopes When invoked from within 'isApplicable(IMarker)', 'isCodanProblem()' did not work as expected, since it used a cached value that was only updated in 'run(IMarker)'. The old API 'isCodanProblem()' has been marked as deprecated and is replaced by 'isCodanProblem(IMarker)', which works directly on the marker, instead of using a cached result. Additionally, two new APIs in 'QuickFixTestCase', called 'calculateQuickFixApplicability()' and 'assertIsApplicableForAllMarkers(boolean)', are introduced. The former can be used to record the applicability of the QuickFix under test for every marker in the test code, while the latter provides a way to assert on the applicability. For finer grained assertions, 'calculateQuickFixApplicability()' returns the calculated map. Change-Id: I7c53fd26afefa37ff086559acea75a7a33ecd5d7 Signed-off-by: Felix Morgner --- .../ui/quickfix/QuickFixAddSemicolon.java | 2 +- .../quickfix/QuickFixCreateLocalVariable.java | 4 +- .../ui/quickfix/QuickFixRenameMember.java | 2 +- ...BreakQuickFixFallthroughAttributeTest.java | 19 +++++++++- .../CreateLocalVariableQuickFixTest.java | 18 +++++++++ .../ui/quickfix/QuickFixTestCase.java | 37 ++++++++++++++++++- .../META-INF/MANIFEST.MF | 2 +- .../ui/AbstractCodanCMarkerResolution.java | 14 +++++++ 8 files changed, 90 insertions(+), 8 deletions(-) diff --git a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixAddSemicolon.java b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixAddSemicolon.java index cff3b481512..31b9b9720b6 100644 --- a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixAddSemicolon.java +++ b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixAddSemicolon.java @@ -42,7 +42,7 @@ public class QuickFixAddSemicolon extends AbstractAstRewriteQuickFix { return; } IASTNode astNode = null; - if (isCodanProblem()) + if (isCodanProblem(marker)) return; // We need to back up in the file diff --git a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixCreateLocalVariable.java b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixCreateLocalVariable.java index da4c0332940..3c9713e2991 100644 --- a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixCreateLocalVariable.java +++ b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixCreateLocalVariable.java @@ -44,7 +44,7 @@ public class QuickFixCreateLocalVariable extends AbstractAstRewriteQuickFix { return; } IASTName astName; - if (isCodanProblem()) { + if (isCodanProblem(marker)) { astName = getASTNameFromMarker(marker, ast); } else { astName = getAstNameFromProblemArgument(marker, ast, 0); @@ -77,7 +77,7 @@ public class QuickFixCreateLocalVariable extends AbstractAstRewriteQuickFix { @Override public boolean isApplicable(IMarker marker) { - if (isCodanProblem()) { + if (isCodanProblem(marker)) { String problemArgument = getProblemArgument(marker, 1); return problemArgument.contains(":func"); //$NON-NLS-1$ } diff --git a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixRenameMember.java b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixRenameMember.java index 1736a825687..8696f4f584c 100644 --- a/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixRenameMember.java +++ b/codan/org.eclipse.cdt.codan.checkers.ui/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixRenameMember.java @@ -42,7 +42,7 @@ public class QuickFixRenameMember extends AbstractAstRewriteQuickFix { return; } IASTName astName; - if (isCodanProblem()) { + if (isCodanProblem(marker)) { astName = getASTNameFromMarker(marker, ast); } else { astName = getAstNameFromProblemArgument(marker, ast, 1); diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CaseBreakQuickFixFallthroughAttributeTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CaseBreakQuickFixFallthroughAttributeTest.java index 23b63dd7551..e507e722e9f 100644 --- a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CaseBreakQuickFixFallthroughAttributeTest.java +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CaseBreakQuickFixFallthroughAttributeTest.java @@ -7,10 +7,27 @@ *******************************************************************************/ package org.eclipse.cdt.codan.internal.checkers.ui.quickfix; +import org.eclipse.cdt.codan.internal.checkers.CaseBreakChecker; import org.eclipse.cdt.codan.ui.AbstractCodanCMarkerResolution; +@SuppressWarnings("restriction") public class CaseBreakQuickFixFallthroughAttributeTest extends QuickFixTestCase { - @SuppressWarnings("restriction") + + private boolean wasEnabled; + + @Override + public void setUp() throws Exception { + super.setUp(); + wasEnabled = (boolean) getPreference(CaseBreakChecker.ER_ID, CaseBreakChecker.PARAM_ENABLE_FALLTHROUGH_QUICKFIX).getValue(); + setPreferenceValue(CaseBreakChecker.ER_ID, CaseBreakChecker.PARAM_ENABLE_FALLTHROUGH_QUICKFIX, true); + } + + @Override + public void tearDown() throws Exception { + setPreferenceValue(CaseBreakChecker.ER_ID, CaseBreakChecker.PARAM_ENABLE_FALLTHROUGH_QUICKFIX, wasEnabled); + super.tearDown(); + } + @Override protected AbstractCodanCMarkerResolution createQuickFix() { return new CaseBreakQuickFixFallthroughAttribute(); diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CreateLocalVariableQuickFixTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CreateLocalVariableQuickFixTest.java index f721c7ade19..36b411b4b39 100644 --- a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CreateLocalVariableQuickFixTest.java +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/CreateLocalVariableQuickFixTest.java @@ -98,4 +98,22 @@ public class CreateLocalVariableQuickFixTest extends QuickFixTestCase { String result = runQuickFixOneFile(); assertContainedIn("char aChar", result); //$NON-NLS-1$ } + + //int global = undefined; + public void testIsNotApplicableInGlobalScope_525250() throws Exception { + loadcode(getAboveComment()); + indexFiles(); + calculateQuickFixApplicability(); + assertIsApplicableForAllMarkers(false); + } + + //class Test{ + // int mem = var; + //}; + public void testIsNotApplicableInClassScope_525250() throws Exception { + loadcode(getAboveComment()); + indexFiles(); + calculateQuickFixApplicability(); + assertIsApplicableForAllMarkers(false); + } } diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixTestCase.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixTestCase.java index 2fd7fa29df2..2ce765c71e4 100644 --- a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixTestCase.java +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/internal/checkers/ui/quickfix/QuickFixTestCase.java @@ -11,6 +11,8 @@ package org.eclipse.cdt.codan.internal.checkers.ui.quickfix; import java.io.IOException; +import java.util.HashMap; +import java.util.Map; import org.eclipse.cdt.codan.core.PreferenceConstants; import org.eclipse.cdt.codan.core.tests.CheckerTestCase; @@ -35,6 +37,7 @@ import org.eclipse.ui.PlatformUI; public abstract class QuickFixTestCase extends CheckerTestCase { AbstractCodanCMarkerResolution quickFix; Display display; + Map isApplicableMap; /** * Dispatch ui events for at least msec - milliseconds @@ -73,6 +76,7 @@ public abstract class QuickFixTestCase extends CheckerTestCase { super.setUp(); quickFix = createQuickFix(); display = PlatformUI.getWorkbench().getDisplay(); + isApplicableMap = new HashMap<>(); closeWelcome(); IPreferenceStore store = CodanUIActivator.getDefault().getPreferenceStore(cproject.getProject()); // turn off editor reconciler @@ -109,6 +113,22 @@ public abstract class QuickFixTestCase extends CheckerTestCase { return new TextSelection(code.indexOf(string), string.length()); } + /** + * Calculate for which markers in the current test the QuickFix under test is + * applicable. + * + * @return A map reflecting for which markers the QuickFix is applicable. + */ + public Map calculateQuickFixApplicability() { + runCodan(); + Display.getDefault().syncExec(() -> { + for (IMarker marker : markers) { + isApplicableMap.put(marker, quickFix.isApplicable(marker)); + } + }); + return isApplicableMap; + } + public String runQuickFixOneFile() throws IOException, CoreException { // need to load before running codan because otherwise marker is lost when doing quick fix 8[] runCodan(); @@ -123,8 +143,11 @@ public abstract class QuickFixTestCase extends CheckerTestCase { public void run() { for (int i = 0; i < markers.length; i++) { IMarker marker = markers[i]; - quickFix.run(marker); - dispatch(0); + isApplicableMap.put(marker, quickFix.isApplicable(marker)); + if (quickFix.isApplicable(marker)) { + quickFix.run(marker); + dispatch(0); + } } PlatformUI.getWorkbench().saveAllEditors(false); } @@ -140,6 +163,16 @@ public abstract class QuickFixTestCase extends CheckerTestCase { assertTrue("Text <" + expected + "> not found in <" + result + ">", result.contains(expected)); //$NON-NLS-1$//$NON-NLS-2$ //$NON-NLS-3$ } + /** + * Assert whether or not the QuickFix under test was applicable for all markers + * in the test code. + */ + public void assertIsApplicableForAllMarkers(boolean expected) { + for (IMarker marker : markers) { + assertEquals(expected, (boolean) isApplicableMap.get(marker)); + } + } + /** * Changes the quick fix to be used * @param quickFix diff --git a/codan/org.eclipse.cdt.codan.ui.cxx/META-INF/MANIFEST.MF b/codan/org.eclipse.cdt.codan.ui.cxx/META-INF/MANIFEST.MF index cb64fc17190..3571e702e84 100644 --- a/codan/org.eclipse.cdt.codan.ui.cxx/META-INF/MANIFEST.MF +++ b/codan/org.eclipse.cdt.codan.ui.cxx/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %Bundle-Name Bundle-SymbolicName: org.eclipse.cdt.codan.ui.cxx;singleton:=true -Bundle-Version: 3.3.0.qualifier +Bundle-Version: 3.4.0.qualifier Bundle-Activator: org.eclipse.cdt.codan.internal.ui.cxx.Activator Bundle-Vendor: %Bundle-Vendor Require-Bundle: org.eclipse.ui, diff --git a/codan/org.eclipse.cdt.codan.ui.cxx/src/org/eclipse/cdt/codan/ui/AbstractCodanCMarkerResolution.java b/codan/org.eclipse.cdt.codan.ui.cxx/src/org/eclipse/cdt/codan/ui/AbstractCodanCMarkerResolution.java index 50c8eca3911..28592ece451 100644 --- a/codan/org.eclipse.cdt.codan.ui.cxx/src/org/eclipse/cdt/codan/ui/AbstractCodanCMarkerResolution.java +++ b/codan/org.eclipse.cdt.codan.ui.cxx/src/org/eclipse/cdt/codan/ui/AbstractCodanCMarkerResolution.java @@ -75,6 +75,20 @@ public abstract class AbstractCodanCMarkerResolution implements ICodanMarkerReso return position; } + /** + * Check if the given marker is associated with a Codan problem + * @since 3.4 + */ + public boolean isCodanProblem(IMarker marker) { + return getProblemId(marker) != null; + } + + /** + * @deprecated Does not work as expected when called + * in {@link #isApplicable(IMarker)}, use + * {@link #isCodanProblem(IMarker)} instead + */ + @Deprecated public boolean isCodanProblem() { return codanProblem; }