Test string contexts

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ExprContext.java

@@ -9,9 +9,12 @@ import org.checkerframework.checker.nullness.qual.Nullable;
 
 import net.sourceforge.pmd.annotation.Experimental;
 import net.sourceforge.pmd.internal.util.AssertionUtil;
+import net.sourceforge.pmd.lang.java.types.JClassType;
 import net.sourceforge.pmd.lang.java.types.JTypeMirror;
 import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult;
 import net.sourceforge.pmd.lang.java.types.TypeConversion;
+import net.sourceforge.pmd.lang.java.types.TypeSystem;
+import net.sourceforge.pmd.lang.java.types.TypesFromReflection;
 
 /**
  * Context of an expression. This determines the target type of poly
@@ -87,6 +90,10 @@ public abstract class ExprContext {
         return kind == CtxKind.Numeric;
     }
 
+    public boolean isString() {
+        return kind == CtxKind.String;
+    }
+
     boolean canGiveContextToPoly(boolean lambda) {
         return true;
     }
@@ -103,6 +110,11 @@ public abstract class ExprContext {
         return new RegularCtx(targetType, CtxKind.OtherNonPoly);
     }
 
+    static ExprContext newStringCtx(TypeSystem ts) {
+        JClassType stringType = (JClassType) TypesFromReflection.fromReflect(String.class, ts);
+        return new RegularCtx(stringType, CtxKind.String);
+    }
+
     static ExprContext newNumericContext(JTypeMirror targetType) {
         if (targetType.isPrimitive()) {
             assert targetType.isNumeric() : targetType;
@@ -153,6 +165,11 @@ public abstract class ExprContext {
         public boolean isInvocationContext() {
             return true;
         }
+
+        @Override
+        public String toString() {
+            return "InvocCtx{arg=" + arg + ", node=" + node + '}';
+        }
     }
 
     /**
@@ -201,6 +218,14 @@ public abstract class ExprContext {
          */
         Numeric,
 
+        /**
+         * String contexts, which convert the operand to a string using {@link String#valueOf(Object)},
+         * or the equivalent for a primitive type. They accept operands of any type.
+         * This is the context for the operands of a string concatenation expression,
+         * and for the message of an assert statement.
+         */
+        String,
+
         /** Kind for a standalone ternary (both branches are then in this context). */
         Ternary,
 
@@ -212,10 +237,7 @@ public abstract class ExprContext {
          * but not for poly expressions. These do not flow through ternary branches.
          * These include:
          * <ul>
-         * <li>TODO String contexts, which convert the operand to a string using {@link String#valueOf(Object)},
-         * or the equivalent for a primitive type. They accept operands of any type.
-         * This is the context for the operands of a string concatenation expression,
-         * and for the message of an assert statement.
+         * <li>
          * <li>Boolean contexts, which unbox their operand to a boolean.
          * They accept operands of type boolean or Boolean. This is the
          * context for e.g. the condition of an {@code if} statement, an
@@ -261,5 +283,10 @@ public abstract class ExprContext {
         public @Nullable JTypeMirror getTargetType() {
             return targetType;
         }
+
+        @Override
+        public String toString() {
+            return "RegularCtx{kind=" + kind + ", targetType=" + targetType + '}';
+        }
     }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/PolyResolution.java

@@ -22,14 +22,13 @@ import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ExprContext.InvocCtx;
 import net.sourceforge.pmd.lang.java.ast.ExprContext.RegularCtx;
 import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
-import net.sourceforge.pmd.lang.java.types.JClassType;
 import net.sourceforge.pmd.lang.java.types.JMethodSig;
 import net.sourceforge.pmd.lang.java.types.JPrimitiveType;
 import net.sourceforge.pmd.lang.java.types.JTypeMirror;
 import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult;
 import net.sourceforge.pmd.lang.java.types.TypeOps;
 import net.sourceforge.pmd.lang.java.types.TypeSystem;
-import net.sourceforge.pmd.lang.java.types.TypesFromReflection;
+import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 import net.sourceforge.pmd.lang.java.types.internal.infer.ExprMirror.BranchingMirror;
 import net.sourceforge.pmd.lang.java.types.internal.infer.ExprMirror.FunctionalExprMirror;
 import net.sourceforge.pmd.lang.java.types.internal.infer.ExprMirror.InvocationMirror;
@@ -54,10 +53,10 @@ final class PolyResolution {
         this.infer = infer;
         this.ts = infer.getTypeSystem();
         this.exprMirrors = new JavaExprMirrors(infer);
-        JClassType stringType = (JClassType) TypesFromReflection.fromReflect(String.class, ts);
-        booleanCtx = ExprContext.newNonPolyContext(ts.BOOLEAN);
-        stringCtx = ExprContext.newNonPolyContext(stringType);
-        intCtx = ExprContext.newNumericContext(ts.INT);
+
+        this.stringCtx = ExprContext.newStringCtx(ts);
+        this.booleanCtx = ExprContext.newNonPolyContext(ts.BOOLEAN);
+        this.intCtx = ExprContext.newNumericContext(ts.INT);
     }
 
     JTypeMirror computePolyType(final TypeNode e) {
@@ -500,11 +499,16 @@ final class PolyResolution {
                     return ExprContext.newNonPolyContext(otherOperand.getTypeMirror().unbox());
                 }
                 return RegularCtx.NO_CTX;
+            case ADD:
+                if (TypeTestUtil.isA(String.class, ctxType)) {
+                    // string concat expr
+                    return stringCtx;
+                }
+                // fallthrough
             case LE:
             case GE:
             case GT:
             case LT:
-            case ADD:
             case SUB:
             case MUL:
             case DIV:

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryCastRule.java

@@ -35,6 +35,7 @@ import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.lang.java.types.JTypeMirror;
 import net.sourceforge.pmd.lang.java.types.TypeConversion;
 import net.sourceforge.pmd.lang.java.types.TypeOps;
+import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 
 /**
  * Detects casts where the operand is already a subtype of the context
@@ -107,7 +108,7 @@ public class UnnecessaryCastRule extends AbstractJavaRulechainRule {
                 && operandType.isSubtypeOf(coercionType);
         }
 
-        return !isCastDeterminingContext(castExpr, context, coercionType)
+        return !isCastDeterminingContext(castExpr, context, coercionType, operandType)
             && castIsUnnecessaryToMatchContext(context, coercionType, operandType);
     }
 
@@ -126,14 +127,10 @@ public class UnnecessaryCastRule extends AbstractJavaRulechainRule {
         JTypeMirror contextType = context.getTargetType();
         if (contextType == null) {
             return false; // should not occur in valid code
-        }
-
-        if (!TypeConversion.isConvertibleUsingBoxing(operandType, coercionType)) {
+        } else if (!TypeConversion.isConvertibleUsingBoxing(operandType, coercionType)) {
             // narrowing cast
             return false;
-        }
-
-        if (!context.acceptsType(operandType)) {
+        } else if (!context.acceptsType(operandType)) {
             // then removing the cast would produce uncompilable code
             return false;
         }
@@ -149,30 +146,32 @@ public class UnnecessaryCastRule extends AbstractJavaRulechainRule {
      * that the cast is necessary, because there's some primitive conversions
      * happening, or some other corner case.
      */
-    private static boolean isCastDeterminingContext(ASTCastExpression castExpr, ExprContext context, @NonNull JTypeMirror coercionType) {
+    private static boolean isCastDeterminingContext(ASTCastExpression castExpr, ExprContext context, @NonNull JTypeMirror coercionType, JTypeMirror operandType) {
 
         if (castExpr.getParent() instanceof ASTConditionalExpression && castExpr.getIndexInParent() != 0) {
             // a branch of a ternary
             return true;
-        }
 
-        if (context.isNumeric() && castExpr.getParent() instanceof ASTInfixExpression) {
+        } else if (context.isString() && isInfixExprWithOperator(castExpr.getParent(), ADD)) {
+
+            // inside string concatenation
+            return !TypeTestUtil.isA(String.class, JavaRuleUtil.getOtherOperandIfInInfixExpr(castExpr))
+                && !TypeTestUtil.isA(String.class, operandType);
+
+        } else if (context.isNumeric() && castExpr.getParent() instanceof ASTInfixExpression) {
+            // numeric expr
             ASTInfixExpression parent = (ASTInfixExpression) castExpr.getParent();
 
             if (isInfixExprWithOperator(parent, SHIFT_OPS)) {
                 // if so, then the cast is determining the width of expr
                 // the right operand is always int
                 if (castExpr == parent.getLeftOperand()) {
-                    JTypeMirror operandType = castExpr.getOperand().getTypeMirror();
                     return !TypeOps.isStrictSubtype(operandType.unbox(), operandType.getTypeSystem().INT);
                 } else {
                     return false;
                 }
             } else if (isInfixExprWithOperator(parent, BINARY_PROMOTED_OPS)) {
                 ASTExpression otherOperand = JavaRuleUtil.getOtherOperandIfInInfixExpr(castExpr);
-                if (otherOperand instanceof ASTCastExpression) {
-                    return true; // remove FPs
-                }
                 JTypeMirror otherType = otherOperand.getTypeMirror();
 
                 // Ie, the type that is taken by the binary promotion

pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/types/internal/infer/ConversionContextTests.kt

@@ -6,9 +6,11 @@
 package net.sourceforge.pmd.lang.java.types.internal.infer
 
 import io.kotest.assertions.withClue
+import io.kotest.matchers.shouldBe
 import net.sourceforge.pmd.lang.ast.test.component6
 import net.sourceforge.pmd.lang.ast.test.shouldBe
 import net.sourceforge.pmd.lang.java.ast.*
+import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil
 import net.sourceforge.pmd.lang.java.types.STRING
 import net.sourceforge.pmd.lang.java.types.parseWithTypeInferenceSpy
 import net.sourceforge.pmd.lang.java.types.shouldHaveType
@@ -218,4 +220,30 @@ class ConversionContextTests : ProcessorTestSpec({
             }
         }
     }
+    parserTest("String contexts") {
+
+        val (acu, spy) = parser.parseWithTypeInferenceSpy("""
+            class Scratch {
+                static void m(int i) {
+                    eat(" " + i);
+                    eat(i + " ");
+                    eat(" " + " ");
+                    eat(" " + i + i);
+                }
+                void eat(Object d) {}
+            }
+        """)
+
+        val concats = acu.descendants(ASTInfixExpression::class.java).toList()
+
+        spy.shouldBeOk {
+            concats.forEach {
+                withClue(it) {
+                    JavaRuleUtil.isStringConcatExpr(it) shouldBe true
+                    it.leftOperand.conversionContext::getTargetType shouldBe ts.STRING
+                    it.rightOperand.conversionContext::getTargetType shouldBe ts.STRING
+                }
+            }
+        }
+    }
 })

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryCast.xml

@@ -746,10 +746,55 @@ class Scratch {
         <expected-problems>0</expected-problems>
         <code><![CDATA[
             class Scratch {
+
                 static void m(int i, byte b, char c, short s) {
                     i = i * (short) c;
                 }
             }
             ]]></code>
     </test-code>
+    <test-code>
+        <description>char cast to int in string context is ok</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            class Scratch {
+
+                static void m(String s, char c) {
+                    // the toString is different -> necessary (string contexts aren't implemented yet)
+                    s = "(" + (int) c + ")" + c;
+                }
+            }
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>String cast determining context</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>5</expected-linenumbers>
+        <code><![CDATA[
+            class Scratch {
+                static void m(Object a, Object b) {
+                    String s = a + (String) b   // necessary, determines context
+                            + a + (String) b    // technically unnecessary, but narrowing
+                            + a + (String) null // unnecessary
+                            ;
+                }
+            }
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>Cast on both sides of arithmetic</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
+        <expected-messages>
+            <message>Unnecessary cast (int)</message>
+        </expected-messages>
+        <code><![CDATA[
+            class Scratch {
+                static void m(short s, Object b) {
+                    b = (int) s
+                            + (long) s;
+                }
+            }
+            ]]></code>
+    </test-code>
 </test-data>