Skip to content

Commit e50e420

Browse files
adangeladangel
committed
[java] Fix #5070 - confusing argument to varargs method FP when types are unknown (#5374)
Merge pull request #5374 from oowekyala:issue5070-confusingArgToVarargs-unresolved
2 parents df2d20e + 8fcfb83 commit e50e420

8 files changed

Lines changed: 176 additions & 11 deletions

File tree

‎docs/pages/release_notes.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ This is a {{ site.pmd.release_type }} release.
4646
* [#5315](https://github.com/pmd/pmd/issues/5315): \[java] UnnecessaryImport false positive for on-demand imports
4747
* java-design
4848
* [#4763](https://github.com/pmd/pmd/issues/4763): \[java] SimplifyBooleanReturns - wrong suggested solution
49+
* java-errorprone
50+
* [#5070](https://github.com/pmd/pmd/issues/5070): \[java] ConfusingArgumentToVarargsMethod FP when types are unresolved
4951
* java-performance
5052
* [#5287](https://github.com/pmd/pmd/issues/5287): \[java] TooFewBranchesForSwitch false-positive with switch using list of case constants
5153
* [#5314](https://github.com/pmd/pmd/issues/5314): \[java] InsufficientStringBufferDeclarationRule: Lack of handling for char type parameters
@@ -128,6 +130,7 @@ This is a {{ site.pmd.release_type }} release.
128130
* [#5371](https://github.com/pmd/pmd/pull/5371): \[doc] Improve docs on adding Antlr languages - [Juan Martín Sotuyo Dodero](https://github.com/jsotuyod) (@jsotuyod)
129131
* [#5372](https://github.com/pmd/pmd/pull/5372): \[java] Fix #5315 - UnusedImport FP with import on demand - [Clément Fournier](https://github.com/oowekyala) (@oowekyala)
130132
* [#5373](https://github.com/pmd/pmd/pull/5373): \[java] Fix #4763 - wrong message for SimplifyBooleanReturns - [Clément Fournier](https://github.com/oowekyala) (@oowekyala)
133+
* [#5374](https://github.com/pmd/pmd/pull/5374): \[java] Fix #5070 - confusing argument to varargs method FP when types are unknown - [Clément Fournier](https://github.com/oowekyala) (@oowekyala)
131134

132135
### 📦 Dependency updates
133136
<!-- content will be automatically generated, see /do-release.sh -->

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public Object visit(ASTConstructorCall ctorCall, Object data) {
7171
ASTTypeArguments targs = newTypeNode.getTypeArguments();
7272
if (targs != null && targs.isDiamond()
7373
// if unresolved we can't know whether the class is generic or not
74-
|| TypeOps.isUnresolved(newType)) {
74+
|| TypeOps.hasUnresolvedSymbol(newType)) {
7575
return null;
7676
}
7777

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/ConfusingArgumentToVarargsMethodRule.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import net.sourceforge.pmd.lang.java.types.JArrayType;
1515
import net.sourceforge.pmd.lang.java.types.JTypeMirror;
1616
import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult;
17+
import net.sourceforge.pmd.lang.java.types.TypeOps;
1718
import net.sourceforge.pmd.lang.java.types.TypePrettyPrint;
1819

1920
public class ConfusingArgumentToVarargsMethodRule extends AbstractJavaRulechainRule {
@@ -45,7 +46,8 @@ public Object visit(ASTArgumentList argList, Object data) {
4546
ASTExpression varargsArg = argList.getLastChild();
4647
assert varargsArg != null;
4748
if (varargsArg.getTypeMirror().isSubtypeOf(expectedComponent)
48-
&& !varargsArg.getTypeMirror().equals(lastFormal)) {
49+
&& !varargsArg.getTypeMirror().equals(lastFormal)
50+
&& !TypeOps.isSpecialUnresolvedOrArray(varargsArg.getTypeMirror())) {
4951
// confusing
5052

5153
String message;

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeOps.java‎

Lines changed: 50 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,7 @@ Convertibility isConvertible(@NonNull JTypeMirror t, @NonNull JTypeMirror s, boo
634634
return isConvertible(t, lower);
635635
}
636636
// otherwise fallthrough
637-
} else if (hasUnresolvedSymbol(t) && t instanceof JClassType) {
637+
} else if (hasUnresolvedSymbol(t)) {
638638
// This also considers types with an unresolved symbol
639639
// subtypes of (nearly) anything. This allows them to
640640
// pass bound checks on type variables.
@@ -1797,7 +1797,7 @@ public static Set<JTypeMirror> mostSpecific(Collection<? extends JTypeMirror> se
17971797
vLoop:
17981798
for (JTypeMirror v : set) {
17991799
for (JTypeMirror w : set) {
1800-
if (!w.equals(v) && !hasUnresolvedSymbol(w)) {
1800+
if (!w.equals(v) && !hasUnresolvedSymbolOrArray(w)) {
18011801
Convertibility isConvertible = isConvertiblePure(w, v);
18021802
if (isConvertible.bySubtyping()
18031803
// This last case covers unchecked conversion. It is made antisymmetric by the
@@ -2079,7 +2079,8 @@ public static boolean areRelated(@NonNull JTypeMirror t, JTypeMirror s) {
20792079

20802080
/**
20812081
* Returns true if the type is {@link TypeSystem#UNKNOWN},
2082-
* {@link TypeSystem#ERROR}, or its symbol is unresolved.
2082+
* {@link TypeSystem#ERROR}, or a class type with unresolved
2083+
* symbol.
20832084
*
20842085
* @param t Non-null type
20852086
*
@@ -2089,28 +2090,70 @@ public static boolean isUnresolved(@NonNull JTypeMirror t) {
20892090
return isSpecialUnresolved(t) || hasUnresolvedSymbol(t);
20902091
}
20912092

2093+
/**
2094+
* Returns true if the type is {@link TypeSystem#UNKNOWN},
2095+
* or {@link TypeSystem#ERROR}, or a class type with unresolved
2096+
* symbol, or an array of such types.
2097+
*
2098+
* @param t Non-null type
2099+
*
2100+
* @throws NullPointerException if the parameter is null
2101+
*/
2102+
public static boolean isUnresolvedOrArray(@NonNull JTypeMirror t) {
2103+
return isSpecialUnresolvedOrArray(t) || hasUnresolvedSymbolOrArray(t);
2104+
}
2105+
2106+
/**
2107+
* Returns true if the type is {@link TypeSystem#UNKNOWN},
2108+
* or {@link TypeSystem#ERROR}.
2109+
*
2110+
* @param t Non-null type
2111+
*
2112+
* @throws NullPointerException if the parameter is null
2113+
*/
20922114
public static boolean isSpecialUnresolved(@NonNull JTypeMirror t) {
20932115
TypeSystem ts = t.getTypeSystem();
20942116
return t == ts.UNKNOWN || t == ts.ERROR;
20952117
}
20962118

2119+
/**
2120+
* Returns true if the type is {@link TypeSystem#UNKNOWN},
2121+
* or {@link TypeSystem#ERROR}, or an array of such types.
2122+
*
2123+
* @param t Non-null type
2124+
*
2125+
* @throws NullPointerException if the parameter is null
2126+
*/
2127+
public static boolean isSpecialUnresolvedOrArray(@Nullable JTypeMirror t) {
2128+
return t == null
2129+
|| isSpecialUnresolved(t)
2130+
|| t instanceof JArrayType && isSpecialUnresolved(((JArrayType) t).getElementType());
2131+
}
2132+
20972133
/**
20982134
* Return true if the argument is a {@link JClassType} with
2099-
* {@linkplain JClassSymbol#isUnresolved() an unresolved symbol} or
2100-
* a {@link JArrayType} whose element type matches the first criterion.
2135+
* {@linkplain JClassSymbol#isUnresolved() an unresolved symbol}.
21012136
*/
21022137
public static boolean hasUnresolvedSymbol(@Nullable JTypeMirror t) {
2138+
return t instanceof JClassType && t.getSymbol().isUnresolved();
2139+
}
2140+
2141+
/**
2142+
* Return true if the argument is a {@link JClassType} with
2143+
* {@linkplain JClassSymbol#isUnresolved() an unresolved symbol},
2144+
* or an array whose element type has an unresolved symbol.
2145+
*/
2146+
public static boolean hasUnresolvedSymbolOrArray(@Nullable JTypeMirror t) {
21032147
if (!(t instanceof JClassType)) {
21042148
return t instanceof JArrayType && hasUnresolvedSymbol(((JArrayType) t).getElementType());
21052149
}
2106-
return t.getSymbol() != null && t.getSymbol().isUnresolved();
2150+
return hasUnresolvedSymbol(t);
21072151
}
21082152

21092153
public static boolean isUnresolvedOrNull(@Nullable JTypeMirror t) {
21102154
return t == null || isUnresolved(t);
21112155
}
21122156

2113-
21142157
public static @Nullable JTypeMirror getArrayComponent(@Nullable JTypeMirror t) {
21152158
return t instanceof JArrayType ? ((JArrayType) t).getComponentType() : null;
21162159
}

‎pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/PhaseOverloadSet.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ private boolean doInfer(JMethodSig m1, JMethodSig m2) {
210210

211211
private @NonNull OptionalBool unresolvedTypeFallback(JTypeMirror si, JTypeMirror ti, ExprMirror argExpr) {
212212
JTypeMirror standalone = argExpr.getStandaloneType();
213-
if (standalone != null && TypeOps.isUnresolved(standalone)) {
213+
if (TypeOps.hasUnresolvedSymbolOrArray(standalone)) {
214214
if (standalone.equals(si)) {
215215
return YES;
216216
} else if (standalone.equals(ti)) {

‎pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/types/TypeGenerationUtil.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ val TypeSystem.allTypesGen: Arb<JTypeMirror>
3838
fun TypeSystem.subtypesArb() =
3939
Arb.pair(refTypeGen, refTypeGen)
4040
.filter { (t, s) -> t.isConvertibleTo(s).bySubtyping() }
41-
.filter { (t, s) -> !TypeOps.hasUnresolvedSymbol(t) && !TypeOps.hasUnresolvedSymbol(s) }
41+
.filter { (t, s) -> !TypeOps.hasUnresolvedSymbolOrArray(t) && !TypeOps.hasUnresolvedSymbolOrArray(s) }
4242

4343
infix fun Boolean.implies(v: () -> Boolean): Boolean = !this || v()
4444

‎pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/types/TypeOpsTest.kt‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.java.types
77
import io.kotest.core.spec.style.FunSpec
88
import io.kotest.matchers.collections.shouldContainExactly
99
import io.kotest.matchers.collections.shouldContainExactlyInAnyOrder
10+
import io.kotest.matchers.shouldBe
1011
import io.kotest.property.Arb
1112
import io.kotest.property.arbitrary.shuffle
1213
import io.kotest.property.checkAll
@@ -131,6 +132,86 @@ class Foo {
131132
methodId shouldHaveType barT[`?`]
132133
}
133134
}
135+
136+
test("isSpecialUnresolved") {
137+
val unresolved = ts.declaration(ts.createUnresolvedAsmSymbol("Unknown"))
138+
TypeOps.isSpecialUnresolved(unresolved) shouldBe false
139+
TypeOps.isSpecialUnresolved(unresolved.toArray()) shouldBe false
140+
TypeOps.isSpecialUnresolved(unresolved.toArray(3)) shouldBe false
141+
142+
for (ty in listOf(ts.UNKNOWN, ts.ERROR)) {
143+
TypeOps.isSpecialUnresolved(ty) shouldBe true
144+
TypeOps.isSpecialUnresolved(ty.toArray()) shouldBe false
145+
TypeOps.isSpecialUnresolved(ty.toArray(3)) shouldBe false
146+
}
147+
}
148+
149+
test("isSpecialUnresolvedOrArray") {
150+
val unresolved = ts.declaration(ts.createUnresolvedAsmSymbol("Unknown"))
151+
TypeOps.isSpecialUnresolvedOrArray(unresolved) shouldBe false
152+
TypeOps.isSpecialUnresolvedOrArray(unresolved.toArray()) shouldBe false
153+
TypeOps.isSpecialUnresolvedOrArray(unresolved.toArray(3)) shouldBe false
154+
155+
for (ty in listOf(ts.UNKNOWN, ts.ERROR)) {
156+
TypeOps.isSpecialUnresolvedOrArray(ty) shouldBe true
157+
TypeOps.isSpecialUnresolvedOrArray(ty.toArray()) shouldBe true
158+
TypeOps.isSpecialUnresolvedOrArray(ty.toArray(3)) shouldBe true
159+
}
160+
}
161+
162+
test("hasUnresolvedSymbol") {
163+
val unresolved = ts.declaration(ts.createUnresolvedAsmSymbol("Unknown"))
164+
TypeOps.hasUnresolvedSymbol(unresolved) shouldBe true
165+
TypeOps.hasUnresolvedSymbol(unresolved.toArray()) shouldBe false
166+
TypeOps.hasUnresolvedSymbol(unresolved.toArray(3)) shouldBe false
167+
168+
for (ty in listOf(ts.UNKNOWN, ts.ERROR)) {
169+
TypeOps.hasUnresolvedSymbol(ty) shouldBe false
170+
TypeOps.hasUnresolvedSymbol(ty.toArray()) shouldBe false
171+
TypeOps.hasUnresolvedSymbol(ty.toArray(3)) shouldBe false
172+
}
173+
}
174+
175+
176+
test("hasUnresolvedSymbolOrArray") {
177+
val unresolved = ts.declaration(ts.createUnresolvedAsmSymbol("Unknown"))
178+
TypeOps.hasUnresolvedSymbolOrArray(unresolved) shouldBe true
179+
TypeOps.hasUnresolvedSymbolOrArray(unresolved.toArray()) shouldBe true
180+
TypeOps.hasUnresolvedSymbolOrArray(unresolved.toArray(3)) shouldBe true
181+
182+
for (ty in listOf(ts.UNKNOWN, ts.ERROR)) {
183+
TypeOps.hasUnresolvedSymbolOrArray(ty) shouldBe false
184+
TypeOps.hasUnresolvedSymbolOrArray(ty.toArray()) shouldBe false
185+
TypeOps.hasUnresolvedSymbolOrArray(ty.toArray(3)) shouldBe false
186+
}
187+
}
188+
189+
190+
test("isUnresolved") {
191+
val unresolved = ts.declaration(ts.createUnresolvedAsmSymbol("Unknown"))
192+
TypeOps.isUnresolved(unresolved) shouldBe true
193+
TypeOps.isUnresolved(unresolved.toArray()) shouldBe false
194+
TypeOps.isUnresolved(unresolved.toArray(3)) shouldBe false
195+
196+
for (ty in listOf(ts.UNKNOWN, ts.ERROR)) {
197+
TypeOps.isUnresolved(ty) shouldBe true
198+
TypeOps.isUnresolved(ty.toArray()) shouldBe false
199+
TypeOps.isUnresolved(ty.toArray(3)) shouldBe false
200+
}
201+
}
202+
203+
test("isUnresolvedOrArray") {
204+
val unresolved = ts.declaration(ts.createUnresolvedAsmSymbol("Unknown"))
205+
TypeOps.isUnresolvedOrArray(unresolved) shouldBe true
206+
TypeOps.isUnresolvedOrArray(unresolved.toArray()) shouldBe true
207+
TypeOps.isUnresolvedOrArray(unresolved.toArray(3)) shouldBe true
208+
209+
for (ty in listOf(ts.UNKNOWN, ts.ERROR)) {
210+
TypeOps.isUnresolvedOrArray(ty) shouldBe true
211+
TypeOps.isUnresolvedOrArray(ty.toArray()) shouldBe true
212+
TypeOps.isUnresolvedOrArray(ty.toArray(3)) shouldBe true
213+
}
214+
}
134215
}
135216
}
136217

‎pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/ConfusingArgumentToVarargsMethod.xml‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,4 +106,40 @@
106106
]]></code>
107107
</test-code>
108108

109+
110+
<test-code>
111+
<description>#5070 Types of args are unresolved</description>
112+
<expected-problems>1</expected-problems>
113+
<expected-linenumbers>7</expected-linenumbers>
114+
<code><![CDATA[
115+
public class Foo {
116+
static {
117+
// if we don't know the type of ARRAY we should not report a violation
118+
foo(Somewhere.ARRAY);
119+
// here we know the type is Unknown[] even if Unknown is unresolved,
120+
// we know it's not Object[] so we report
121+
foo(UNKNOWN);
122+
}
123+
static Unknown[] UNKNOWN;
124+
static void foo(Object... args) {}
125+
}
126+
]]></code>
127+
</test-code>
128+
129+
130+
<test-code>
131+
<description>#5070 Types of args is array of unknown</description>
132+
<expected-problems>0</expected-problems>
133+
<code><![CDATA[
134+
public class Foo {
135+
static {
136+
var x = arr(UNKNOWN); // x: (*unknown*)[]
137+
foo(x);
138+
}
139+
static <T> T[] arr(T[] x) { return x; }
140+
static void foo(Object... args) {}
141+
}
142+
]]></code>
143+
</test-code>
144+
109145
</test-data>

0 commit comments

Comments
 (0)