From 56bff9a5cf93818c53fb38874834fbfcbe3e4eb9 Mon Sep 17 00:00:00 2001 From: Jameson Williams Date: Wed, 10 Apr 2024 16:00:41 -0500 Subject: [PATCH] Refine highlighting and error reporting in Lint rules (#693) --- .../ControllerChangeHandlerIssueDetector.java | 20 ++++++------ .../lint/ControllerIssueDetector.java | 29 +++++++++-------- .../bluelinelabs/conductor/lint/Identify.java | 15 +++++++++ .../ControllerChangeHandlerDetectorTest.java | 25 ++++++++------- .../lint/ControllerDetectorTest.java | 32 +++++++++++-------- 5 files changed, 71 insertions(+), 50 deletions(-) create mode 100644 conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/Identify.java diff --git a/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerIssueDetector.java b/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerIssueDetector.java index a20d77b..0763b1d 100644 --- a/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerIssueDetector.java +++ b/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerIssueDetector.java @@ -18,7 +18,6 @@ import org.jetbrains.uast.UMethod; import java.util.Collections; import java.util.List; -@SuppressWarnings("UnstableApiUsage") public final class ControllerChangeHandlerIssueDetector extends Detector implements Detector.UastScanner { static final Issue ISSUE = @@ -32,7 +31,7 @@ public final class ControllerChangeHandlerIssueDetector extends Detector impleme @Override public List> getApplicableUastTypes() { - return Collections.>singletonList(UClass.class); + return Collections.singletonList(UClass.class); } @Override @@ -47,42 +46,41 @@ public final class ControllerChangeHandlerIssueDetector extends Detector impleme return; } - final boolean hasSuperType = evaluator.extendsClass(node.getPsi(), CLASS_NAME, true); + final boolean hasSuperType = evaluator.extendsClass(node.getJavaPsi(), CLASS_NAME, true); if (!hasSuperType) { return; } if (!evaluator.isPublic(node)) { String message = String.format("This ControllerChangeHandler class should be public (%1$s)", node.getQualifiedName()); - context.report(ISSUE, node, context.getLocation((UElement) node), message); + context.report(ISSUE, node, context.getLocation(Identify.byName(node)), message); return; } if (node.getContainingClass() != null && !evaluator.isStatic(node)) { String message = String.format("This ControllerChangeHandler inner class should be static (%1$s)", node.getQualifiedName()); - context.report(ISSUE, node, context.getLocation((UElement) node), message); + context.report(ISSUE, node, context.getLocation(Identify.byName(node)), message); return; } - boolean hasConstructor = false; + UMethod constructor = null; boolean hasDefaultConstructor = false; for (UMethod method : node.getMethods()) { if (method.isConstructor()) { - hasConstructor = true; - if (evaluator.isPublic(method) && method.getUastParameters().size() == 0) { + constructor = method; + if (evaluator.isPublic(method) && method.getUastParameters().isEmpty()) { hasDefaultConstructor = true; break; } } } - if (hasConstructor && !hasDefaultConstructor) { + if (constructor != null && !hasDefaultConstructor) { String message = String.format( "This ControllerChangeHandler needs to have a public default constructor (`%1$s`)", node.getQualifiedName()); - context.report(ISSUE, node, context.getLocation((UElement) node), message); + context.report(ISSUE, node, context.getLocation(Identify.byName(constructor)), message); } } }; } - } diff --git a/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerIssueDetector.java b/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerIssueDetector.java index 10cb122..1b7e92f 100644 --- a/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerIssueDetector.java +++ b/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/ControllerIssueDetector.java @@ -10,6 +10,7 @@ import com.android.tools.lint.detector.api.Issue; import com.android.tools.lint.detector.api.JavaContext; import com.android.tools.lint.detector.api.Scope; import com.android.tools.lint.detector.api.Severity; +import com.intellij.psi.PsiType; import org.jetbrains.annotations.NotNull; import org.jetbrains.uast.UClass; @@ -20,7 +21,6 @@ import org.jetbrains.uast.UParameter; import java.util.Collections; import java.util.List; -@SuppressWarnings("UnstableApiUsage") public final class ControllerIssueDetector extends Detector implements Detector.UastScanner { static final Issue ISSUE = @@ -48,52 +48,53 @@ public final class ControllerIssueDetector extends Detector implements Detector. return; } - final boolean hasSuperType = evaluator.extendsClass(node.getPsi(), CLASS_NAME, true); + final boolean hasSuperType = evaluator.extendsClass(node.getJavaPsi(), CLASS_NAME, true); if (!hasSuperType) { return; } if (!evaluator.isPublic(node)) { String message = String.format("This Controller class should be public (%1$s)", node.getQualifiedName()); - context.report(ISSUE, node, context.getLocation((UElement) node), message); + context.report(ISSUE, node, context.getLocation(Identify.byName(node)), message); return; } if (node.getContainingClass() != null && !evaluator.isStatic(node)) { String message = String.format("This Controller inner class should be static (%1$s)", node.getQualifiedName()); - context.report(ISSUE, node, context.getLocation((UElement) node), message); + context.report(ISSUE, node, context.getLocation(Identify.byName(node)), message); return; } - boolean hasConstructor = false; + UMethod constructor = null; boolean hasDefaultConstructor = false; boolean hasBundleConstructor = false; for (UMethod method : node.getMethods()) { if (method.isConstructor()) { - hasConstructor = true; + constructor = method; if (evaluator.isPublic(method)) { List parameters = method.getUastParameters(); - if (parameters.size() == 0) { + if (parameters.isEmpty()) { hasDefaultConstructor = true; break; - } else if (parameters.size() == 1 && - (parameters.get(0).getType().equalsToText(SdkConstants.CLASS_BUNDLE)) || - parameters.get(0).getType().equalsToText("Bundle")) { - hasBundleConstructor = true; + } else if (parameters.size() == 1) { + PsiType type = parameters.get(0).getType(); + if (type.equalsToText(SdkConstants.CLASS_BUNDLE) || type.equalsToText("Bundle")) { + hasBundleConstructor = true; + break; + } } } } } - if (hasConstructor && !hasDefaultConstructor && !hasBundleConstructor) { + if (constructor != null && !hasDefaultConstructor && !hasBundleConstructor) { String message = String.format( "This Controller needs to have either a public default constructor or a" + " public single-argument constructor that takes a Bundle. (`%1$s`)", node.getQualifiedName()); - context.report(ISSUE, node, context.getLocation((UElement) node), message); + context.report(ISSUE, node, context.getLocation(Identify.byName(constructor)), message); } } }; } - } diff --git a/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/Identify.java b/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/Identify.java new file mode 100644 index 0000000..2c7fcb5 --- /dev/null +++ b/conductor-lint/src/main/java/com/bluelinelabs/conductor/lint/Identify.java @@ -0,0 +1,15 @@ +package com.bluelinelabs.conductor.lint; + +import com.intellij.psi.PsiElement; +import com.intellij.psi.PsiNameIdentifierOwner; + +final class Identify { + private Identify() {} + + static PsiElement byName(PsiNameIdentifierOwner psiNameIdentifierOwner) { + if (psiNameIdentifierOwner.getNameIdentifier() != null) { + return psiNameIdentifierOwner.getNameIdentifier(); + } + return psiNameIdentifierOwner; + } +} diff --git a/conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerDetectorTest.java b/conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerDetectorTest.java index f9dd6a1..60edaf9 100644 --- a/conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerDetectorTest.java +++ b/conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerChangeHandlerDetectorTest.java @@ -8,15 +8,8 @@ import com.android.tools.lint.checks.infrastructure.TestFile; import org.intellij.lang.annotations.Language; import org.junit.Test; -@SuppressWarnings("UnstableApiUsage") public class ControllerChangeHandlerDetectorTest { - private static final String CONSTRUCTOR = - "src/test/SampleHandler.java:2: Error: This ControllerChangeHandler needs to have a public default constructor (test.SampleHandler) [ValidControllerChangeHandler]\n" - + "public class SampleHandler extends com.bluelinelabs.conductor.ControllerChangeHandler {\n" - + "^\n" - + "1 errors, 0 warnings\n"; - private final TestFile controllerChangeHandlerStub = java( "package com.bluelinelabs.conductor;\n" + "abstract class ControllerChangeHandler {}" @@ -63,7 +56,12 @@ public class ControllerChangeHandlerDetectorTest { .files(controllerChangeHandlerStub, java(source)) .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE) .run() - .expect(CONSTRUCTOR); + .expect("" + + "src/test/SampleHandler.java:3: Error: This ControllerChangeHandler needs to have a public default constructor (test.SampleHandler) [ValidControllerChangeHandler]\n" + + " public SampleHandler(int number) { }\n" + + " ~~~~~~~~~~~~~\n" + + "1 errors, 0 warnings\n" + ); } @Test @@ -94,7 +92,12 @@ public class ControllerChangeHandlerDetectorTest { .files(controllerChangeHandlerStub, java(source)) .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE) .run() - .expect(CONSTRUCTOR); + .expect("" + + "src/test/SampleHandler.java:3: Error: This ControllerChangeHandler needs to have a public default constructor (test.SampleHandler) [ValidControllerChangeHandler]\n" + + " private SampleHandler() { }\n" + + " ~~~~~~~~~~~~~\n" + + "1 errors, 0 warnings\n" + ); } @Test @@ -111,7 +114,7 @@ public class ControllerChangeHandlerDetectorTest { .run() .expect("src/test/SampleHandler.java:2: Error: This ControllerChangeHandler class should be public (test.SampleHandler) [ValidControllerChangeHandler]\n" + "private class SampleHandler extends com.bluelinelabs.conductor.ControllerChangeHandler {\n" - + "^\n" + + " ~~~~~~~~~~~~~\n" + "1 errors, 0 warnings\n"); } @@ -131,7 +134,7 @@ public class ControllerChangeHandlerDetectorTest { .run() .expect("src/test/SampleHandler.java:2: Error: This ControllerChangeHandler class should be public (test.SampleHandler) [ValidControllerChangeHandler]\n" + "private class SampleHandler extends test.BaseChangeHandler {}\n" + - "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" + + " ~~~~~~~~~~~~~\n" + "1 errors, 0 warnings"); } } diff --git a/conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerDetectorTest.java b/conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerDetectorTest.java index 8b63815..a0b6fe8 100644 --- a/conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerDetectorTest.java +++ b/conductor-lint/src/test/java/com/bluelinelabs/conductor/lint/ControllerDetectorTest.java @@ -8,18 +8,12 @@ import com.android.tools.lint.checks.infrastructure.TestFile; import org.intellij.lang.annotations.Language; import org.junit.Test; -@SuppressWarnings("UnstableApiUsage") public class ControllerDetectorTest { - private static final String CONSTRUCTOR_ERROR = - "src/test/SampleController.java:2: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n" - + "public class SampleController extends com.bluelinelabs.conductor.Controller {\n" - + "^\n" - + "1 errors, 0 warnings\n"; private static final String CLASS_ERROR = "src/test/SampleController.java:2: Error: This Controller class should be public (test.SampleController) [ValidController]\n" + "private class SampleController extends com.bluelinelabs.conductor.Controller {\n" - + "^\n" + + " ~~~~~~~~~~~~~~~~\n" + "1 errors, 0 warnings\n"; private final TestFile controllerStub = java( @@ -69,7 +63,12 @@ public class ControllerDetectorTest { .files(controllerStub, java(source)) .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE) .run() - .expect(CONSTRUCTOR_ERROR); + .expect("" + + "src/test/SampleController.java:3: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n" + + " public SampleController(int number) { }\n" + + " ~~~~~~~~~~~~~~~~\n" + + "1 errors, 0 warnings\n" + ); } @Test @@ -106,11 +105,11 @@ public class ControllerDetectorTest { .files(controllerStub, java(baseClass), java(source)) .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE) .run() - .expect( - "src/test/SampleController.java:2: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n" + - "public class SampleController extends BaseController {\n" + - "^\n" + - "1 errors, 0 warnings" + .expect("" + + "src/test/SampleController.java:3: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n" + + " private SampleController() { }\n" + + " ~~~~~~~~~~~~~~~~\n" + + "1 errors, 0 warnings" ); } @@ -126,7 +125,12 @@ public class ControllerDetectorTest { .files(controllerStub, java(source)) .issues(ControllerIssueDetector.ISSUE, ControllerChangeHandlerIssueDetector.ISSUE) .run() - .expect(CONSTRUCTOR_ERROR); + .expect("" + + "src/test/SampleController.java:3: Error: This Controller needs to have either a public default constructor or a public single-argument constructor that takes a Bundle. (test.SampleController) [ValidController]\n" + + " private SampleController() { }\n" + + " ~~~~~~~~~~~~~~~~\n" + + "1 errors, 0 warnings\n" + ); } @Test