diff --git a/astra-core/src/main/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofOperation.java b/astra-core/src/main/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofOperation.java new file mode 100644 index 0000000..ce5ffd5 --- /dev/null +++ b/astra-core/src/main/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofOperation.java @@ -0,0 +1,167 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s4201; + +import java.io.IOException; + +import org.alfasoftware.astra.core.utils.ASTOperation; +import org.alfasoftware.astra.core.utils.AstraUtils; +import org.eclipse.jdt.core.dom.ASTNode; +import org.eclipse.jdt.core.dom.CompilationUnit; +import org.eclipse.jdt.core.dom.Expression; +import org.eclipse.jdt.core.dom.InfixExpression; +import org.eclipse.jdt.core.dom.InstanceofExpression; +import org.eclipse.jdt.core.dom.NullLiteral; +import org.eclipse.jdt.core.dom.ParenthesizedExpression; +import org.eclipse.jdt.core.dom.PrefixExpression; +import org.eclipse.jdt.core.dom.rewrite.ASTRewrite; +import org.eclipse.jface.text.BadLocationException; +import org.eclipse.text.edits.MalformedTreeException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Refactoring operation implementing SonarQube rule java:S4201 + * "Null checks should not be used with instanceof". + * + *

Since {@code instanceof} already returns {@code false} for {@code null}, + * an explicit null check alongside it is redundant and can be removed. + * + *

Handles the following patterns (and their mirror-operand variants): + *

+ * + *

Does NOT rewrite when: + *

+ */ +public class NullCheckInstanceofOperation implements ASTOperation { + + private static final Logger log = LoggerFactory.getLogger(NullCheckInstanceofOperation.class); + + @Override + public void run(CompilationUnit compilationUnit, ASTNode node, ASTRewrite rewriter) + throws IOException, MalformedTreeException, BadLocationException { + + if (!(node instanceof InfixExpression)) { + return; + } + + InfixExpression infix = (InfixExpression) node; + InfixExpression.Operator op = infix.getOperator(); + + if (op != InfixExpression.Operator.CONDITIONAL_AND + && op != InfixExpression.Operator.CONDITIONAL_OR) { + return; + } + + Expression left = infix.getLeftOperand(); + Expression right = infix.getRightOperand(); + + // Try: left side is the null check, right side is the instanceof (or its negation) + String varFromLeft = extractNullCheckVar(left, op); + if (varFromLeft != null && isCompatibleInstanceofSide(right, op, varFromLeft)) { + log.info("Removing redundant null check in [{}]: replacing '{}' with '{}'", + AstraUtils.getNameForCompilationUnit(compilationUnit), infix, right); + rewriter.replace(infix, rewriter.createCopyTarget(right), null); + return; + } + + // Try: right side is the null check, left side is the instanceof (or its negation) + String varFromRight = extractNullCheckVar(right, op); + if (varFromRight != null && isCompatibleInstanceofSide(left, op, varFromRight)) { + log.info("Removing redundant null check in [{}]: replacing '{}' with '{}'", + AstraUtils.getNameForCompilationUnit(compilationUnit), infix, left); + rewriter.replace(infix, rewriter.createCopyTarget(left), null); + } + } + + /** + * If {@code expr} is a null check compatible with {@code operator}, returns the string + * representation of the non-null operand (the variable being checked); otherwise {@code null}. + * + *

Compatible means: + *

+ */ + private String extractNullCheckVar(Expression expr, InfixExpression.Operator operator) { + if (!(expr instanceof InfixExpression)) { + return null; + } + InfixExpression nullCheck = (InfixExpression) expr; + InfixExpression.Operator nullOp = nullCheck.getOperator(); + + boolean isAndOperator = operator == InfixExpression.Operator.CONDITIONAL_AND; + InfixExpression.Operator expectedNullOp = isAndOperator + ? InfixExpression.Operator.NOT_EQUALS + : InfixExpression.Operator.EQUALS; + + if (nullOp != expectedNullOp) { + return null; + } + + Expression l = nullCheck.getLeftOperand(); + Expression r = nullCheck.getRightOperand(); + + if (l instanceof NullLiteral && !(r instanceof NullLiteral)) { + return r.toString(); + } + if (r instanceof NullLiteral && !(l instanceof NullLiteral)) { + return l.toString(); + } + return null; + } + + /** + * Returns true if {@code expr} is the instanceof side compatible with the given operator + * and involving {@code varName}. + * + *

Compatible means: + *

+ */ + private boolean isCompatibleInstanceofSide( + Expression expr, InfixExpression.Operator operator, String varName) { + + if (operator == InfixExpression.Operator.CONDITIONAL_AND) { + Expression unwrapped = unwrapParens(expr); + if (!(unwrapped instanceof InstanceofExpression)) { + return false; + } + return ((InstanceofExpression) unwrapped).getLeftOperand().toString().equals(varName); + } + + // For ||: expect !(varName instanceof SomeType) + Expression unwrapped = unwrapParens(expr); + if (!(unwrapped instanceof PrefixExpression)) { + return false; + } + PrefixExpression prefix = (PrefixExpression) unwrapped; + if (prefix.getOperator() != PrefixExpression.Operator.NOT) { + return false; + } + Expression negated = unwrapParens(prefix.getOperand()); + if (!(negated instanceof InstanceofExpression)) { + return false; + } + return ((InstanceofExpression) negated).getLeftOperand().toString().equals(varName); + } + + private Expression unwrapParens(Expression expr) { + while (expr instanceof ParenthesizedExpression) { + expr = ((ParenthesizedExpression) expr).getExpression(); + } + return expr; + } +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofExample.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofExample.java new file mode 100644 index 0000000..8ea4542 --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofExample.java @@ -0,0 +1,67 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s4201; + +public class NullCheckInstanceofExample { + + // x != null && x instanceof Foo + void notNullAndInstanceof(Object obj) { + if (obj != null && obj instanceof String) { + System.out.println("is string"); + } + } + + // instanceof first, then != null + void instanceofAndNotNull(Object obj) { + if (obj instanceof String && obj != null) { + System.out.println("is string"); + } + } + + // null literal on the left: null != x && x instanceof Foo + void nullNotEqualsAndInstanceof(Object obj) { + if (null != obj && obj instanceof String) { + System.out.println("is string"); + } + } + + // instanceof first, null literal check on right: x instanceof Foo && null != x + void instanceofAndNullNotEquals(Object obj) { + if (obj instanceof String && null != obj) { + System.out.println("is string"); + } + } + + // x == null || !(x instanceof Foo) + void nullOrNotInstanceof(Object obj) { + if (obj == null || !(obj instanceof String)) { + System.out.println("not a string"); + } + } + + // !(instanceof) first, then == null + void notInstanceofOrNull(Object obj) { + if (!(obj instanceof String) || obj == null) { + System.out.println("not a string"); + } + } + + // null literal on the left: null == x || !(x instanceof Foo) + void nullEqualsOrNotInstanceof(Object obj) { + if (null == obj || !(obj instanceof String)) { + System.out.println("not a string"); + } + } + + // !(instanceof) first, null literal check: !(x instanceof Foo) || null == x + void notInstanceofOrNullEquals(Object obj) { + if (!(obj instanceof String) || null == obj) { + System.out.println("not a string"); + } + } + + // null check inside a larger && chain: null check still removed from the inner sub-expression + void nullCheckInLargerChain(Object obj, boolean extra) { + if (obj != null && obj instanceof String && extra) { + System.out.println("extended chain"); + } + } +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofExampleAfter.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofExampleAfter.java new file mode 100644 index 0000000..a083095 --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofExampleAfter.java @@ -0,0 +1,67 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s4201; + +public class NullCheckInstanceofExampleAfter { + + // x != null && x instanceof Foo + void notNullAndInstanceof(Object obj) { + if (obj instanceof String) { + System.out.println("is string"); + } + } + + // instanceof first, then != null + void instanceofAndNotNull(Object obj) { + if (obj instanceof String) { + System.out.println("is string"); + } + } + + // null literal on the left: null != x && x instanceof Foo + void nullNotEqualsAndInstanceof(Object obj) { + if (obj instanceof String) { + System.out.println("is string"); + } + } + + // instanceof first, null literal check on right: x instanceof Foo && null != x + void instanceofAndNullNotEquals(Object obj) { + if (obj instanceof String) { + System.out.println("is string"); + } + } + + // x == null || !(x instanceof Foo) + void nullOrNotInstanceof(Object obj) { + if (!(obj instanceof String)) { + System.out.println("not a string"); + } + } + + // !(instanceof) first, then == null + void notInstanceofOrNull(Object obj) { + if (!(obj instanceof String)) { + System.out.println("not a string"); + } + } + + // null literal on the left: null == x || !(x instanceof Foo) + void nullEqualsOrNotInstanceof(Object obj) { + if (!(obj instanceof String)) { + System.out.println("not a string"); + } + } + + // !(instanceof) first, null literal check: !(x instanceof Foo) || null == x + void notInstanceofOrNullEquals(Object obj) { + if (!(obj instanceof String)) { + System.out.println("not a string"); + } + } + + // null check inside a larger && chain: null check still removed from the inner sub-expression + void nullCheckInLargerChain(Object obj, boolean extra) { + if (obj instanceof String && extra) { + System.out.println("extended chain"); + } + } +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofNoopExample.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofNoopExample.java new file mode 100644 index 0000000..3ae1970 --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofNoopExample.java @@ -0,0 +1,33 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s4201; + +public class NullCheckInstanceofNoopExample { + + // Different variables — must not be rewritten + void differentVariables(Object obj, Object other) { + if (obj != null && other instanceof String) { + System.out.println("different vars"); + } + } + + // x == null || x instanceof Foo — semantics differ (true when null, but instanceof is false for null) + void nullOrInstanceof(Object obj) { + if (obj == null || obj instanceof String) { + System.out.println("null or string"); + } + } + + // x != null && !(x instanceof Foo) — semantics differ + void notNullAndNotInstanceof(Object obj) { + if (obj != null && !(obj instanceof String)) { + System.out.println("not null and not string"); + } + } + + // Standalone null check with no instanceof — nothing to do + void justNullCheck(Object obj) { + if (obj != null) { + System.out.println("not null"); + } + } + +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofNoopExampleAfter.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofNoopExampleAfter.java new file mode 100644 index 0000000..7d98be9 --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/NullCheckInstanceofNoopExampleAfter.java @@ -0,0 +1,33 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s4201; + +public class NullCheckInstanceofNoopExampleAfter { + + // Different variables — must not be rewritten + void differentVariables(Object obj, Object other) { + if (obj != null && other instanceof String) { + System.out.println("different vars"); + } + } + + // x == null || x instanceof Foo — semantics differ (true when null, but instanceof is false for null) + void nullOrInstanceof(Object obj) { + if (obj == null || obj instanceof String) { + System.out.println("null or string"); + } + } + + // x != null && !(x instanceof Foo) — semantics differ + void notNullAndNotInstanceof(Object obj) { + if (obj != null && !(obj instanceof String)) { + System.out.println("not null and not string"); + } + } + + // Standalone null check with no instanceof — nothing to do + void justNullCheck(Object obj) { + if (obj != null) { + System.out.println("not null"); + } + } + +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/TestNullCheckInstanceofOperation.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/TestNullCheckInstanceofOperation.java new file mode 100644 index 0000000..dd3e31c --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s4201/TestNullCheckInstanceofOperation.java @@ -0,0 +1,33 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s4201; + +import java.util.Set; + +import org.alfasoftware.astra.core.refactoring.AbstractRefactorTest; +import org.junit.Test; + +public class TestNullCheckInstanceofOperation extends AbstractRefactorTest { + + private static final Set OPERATION = + Set.of(new NullCheckInstanceofOperation()); + + /** + * All rewritable forms: + * - {@code x != null && x instanceof Foo} (null check first or second, literal null first or second) + * - {@code x == null || !(x instanceof Foo)} (null check first or second, literal null first or second) + */ + @Test + public void testRedundantNullChecks() { + assertRefactor(NullCheckInstanceofExample.class, OPERATION); + } + + /** + * Cases that must NOT be rewritten: + * - different variables in null check and instanceof + * - semantically non-equivalent combinations ({@code == null || instanceof}, {@code != null && !instanceof}) + * - standalone null check with no instanceof + */ + @Test + public void testNoopCases() { + assertRefactor(NullCheckInstanceofNoopExample.class, OPERATION); + } +}