Skip to content

Commit c02f72d

Browse files
greg-at-modernegreg-at-moderne
authored
InstanceOfPatternMatch to avoid providing invalid variable definitions pointing to itself (#849)
* Avoid invalid x=x variable declarations * A more involved case * Order of clauses shouldn't decide
1 parent 1b97fad commit c02f72d

2 files changed

Lines changed: 295 additions & 20 deletions

File tree

‎src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java‎

Lines changed: 96 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ private static class InstanceOfPatternReplacements {
162162
private final Map<J.InstanceOf, Set<J>> contexts = new HashMap<>();
163163
private final Map<J.InstanceOf, Set<Cursor>> contextScopes = new HashMap<>();
164164
private final Map<J.TypeCast, J.InstanceOf> replacements = new HashMap<>();
165-
private final Map<J.InstanceOf, VariableAndTypeTree> variablesToDelete = new HashMap<>();
165+
private final Map<J.InstanceOf, List<VariableAndTypeTree>> variablesToDelete = new HashMap<>();
166166

167167
public void registerInstanceOf(J.InstanceOf instanceOf, Set<J> contexts) {
168168
Expression expression = instanceOf.getExpression();
@@ -201,9 +201,11 @@ public void registerTypeCast(J.TypeCast typeCast, Cursor cursor) {
201201
if (isAcceptableTypeCast(typeCast.getType()) && isTheSameAsOtherTypeCasts(typeCast, instanceOf) && isAcceptableParentTypeCast(parent) &&
202202
cursor.firstEnclosing(J.Try.Resource.class) == null) {
203203
if (parent.getValue() instanceof J.VariableDeclarations.NamedVariable &&
204-
!variablesToDelete.containsKey(instanceOf)) {
205-
variablesToDelete.put(instanceOf, new VariableAndTypeTree(parent.getValue(),
206-
requireNonNull(requireNonNull(parent.firstEnclosing(J.VariableDeclarations.class)).getTypeExpression())));
204+
shouldDeleteVariableDeclaration(parent, instanceOf)) {
205+
variablesToDelete
206+
.computeIfAbsent(instanceOf, i -> new ArrayList<>())
207+
.add(new VariableAndTypeTree(parent.getValue(),
208+
requireNonNull(requireNonNull(parent.firstEnclosing(J.VariableDeclarations.class)).getTypeExpression())));
207209
} else {
208210
replacements.put(typeCast, instanceOf);
209211
}
@@ -284,6 +286,21 @@ public boolean isEmpty() {
284286
return replacements.isEmpty() && variablesToDelete.isEmpty();
285287
}
286288

289+
private boolean shouldDeleteVariableDeclaration(Cursor parent, J.InstanceOf instanceOf) {
290+
J.VariableDeclarations.NamedVariable namedVariable = parent.getValue();
291+
TypeTree typeExpression = requireNonNull(requireNonNull(parent.firstEnclosing(J.VariableDeclarations.class)).getTypeExpression());
292+
List<VariableAndTypeTree> existingVariables = variablesToDelete.get(instanceOf);
293+
if (existingVariables == null || existingVariables.isEmpty()) {
294+
return true;
295+
}
296+
VariableAndTypeTree primaryVariable = existingVariables.get(0);
297+
JavaType currentType = typeExpression.getType();
298+
JavaType primaryType = primaryVariable.getType().getType();
299+
return namedVariable.getSimpleName().equals(primaryVariable.getVariable().getSimpleName()) &&
300+
((currentType != null && primaryType != null && hasSameRawType(currentType, primaryType)) ||
301+
SemanticallyEqual.areEqual(typeExpression, primaryVariable.getType()));
302+
}
303+
287304
public J.InstanceOf processInstanceOf(J.InstanceOf instanceOf, Cursor cursor, Set<String> usedNames) {
288305
if (!contextScopes.containsKey(instanceOf)) {
289306
return instanceOf;
@@ -320,27 +337,22 @@ public J.InstanceOf processInstanceOf(J.InstanceOf instanceOf, Cursor cursor, Se
320337
}
321338

322339
private TypeTree computeTypeTreeFromTypeCasts(J.InstanceOf instanceOf) {
323-
TypeTree typeCastTypeTree = replacements
324-
.entrySet()
325-
.stream()
326-
.filter(e -> e.getValue() == instanceOf)
327-
.findFirst()
328-
.map(e -> e.getKey().getClazz().getTree())
329-
.orElse(null);
330-
if (typeCastTypeTree == null) {
331-
VariableAndTypeTree variable = variablesToDelete.get(instanceOf);
332-
if (variable != null) {
333-
typeCastTypeTree = variable.getType();
334-
}
335-
}
336-
return typeCastTypeTree;
340+
List<TypeTree> candidateTypes = Stream.concat(
341+
replacements.entrySet().stream()
342+
.filter(e -> e.getValue() == instanceOf)
343+
.map(e -> e.getKey().getClazz().getTree()),
344+
variablesToDelete.getOrDefault(instanceOf, emptyList()).stream()
345+
.map(VariableAndTypeTree::getType)
346+
)
347+
.collect(toList());
348+
return chooseTypeTree(candidateTypes, (TypeTree) instanceOf.getClazz());
337349
}
338350

339351
private String patternVariableName(J.InstanceOf instanceOf, Cursor cursor, Set<String> usedNames) {
340352
VariableNameStrategy strategy;
341353
JavaType type = ((TypeTree) instanceOf.getClazz()).getType();
342354
if (root instanceof J.If) {
343-
VariableAndTypeTree variableData = variablesToDelete.get(instanceOf);
355+
VariableAndTypeTree variableData = getPrimaryVariableToDelete(instanceOf);
344356
if (variableData != null) {
345357
// under the assumption that the code compiled previously we don't need to check for duplicates
346358
return VariableNameStrategy.exact(variableData.getVariable().getSimpleName()).variableName(type);
@@ -366,6 +378,68 @@ private String patternVariableName(J.InstanceOf instanceOf, Cursor cursor, Set<S
366378
return VariableNameUtils.generateVariableName(baseName, cursor, INCREMENT_NUMBER);
367379
}
368380

381+
private @Nullable VariableAndTypeTree getPrimaryVariableToDelete(J.InstanceOf instanceOf) {
382+
List<VariableAndTypeTree> variables = variablesToDelete.get(instanceOf);
383+
if (variables == null || variables.isEmpty()) {
384+
return null;
385+
}
386+
return variables.get(0);
387+
}
388+
389+
private TypeTree chooseTypeTree(List<TypeTree> candidates, TypeTree originalTypeTree) {
390+
if (candidates.isEmpty()) {
391+
return originalTypeTree;
392+
}
393+
394+
JavaType originalType = originalTypeTree.getType();
395+
TypeTree firstCandidate = candidates.get(0);
396+
JavaType firstType = firstCandidate.getType();
397+
if (originalType == null || firstType == null) {
398+
return originalTypeTree;
399+
}
400+
401+
boolean allSameRawType = candidates.stream()
402+
.map(TypeTree::getType)
403+
.allMatch(type -> type != null &&
404+
hasSameRawType(type, originalType) &&
405+
hasSameRawType(type, firstType));
406+
if (!allSameRawType) {
407+
return originalTypeTree;
408+
}
409+
410+
if (candidates.size() == 1) {
411+
return candidates.get(0);
412+
}
413+
414+
List<TypeTree> wildcardCandidates = candidates.stream()
415+
.filter(this::isUnboundedWildcardParameterized)
416+
.collect(toList());
417+
418+
if (!wildcardCandidates.isEmpty()) {
419+
if (wildcardCandidates.stream().allMatch(candidate -> SemanticallyEqual.areEqual(candidate, wildcardCandidates.get(0)))) {
420+
return wildcardCandidates.get(0);
421+
}
422+
return originalTypeTree;
423+
}
424+
425+
if (candidates.stream().allMatch(candidate -> SemanticallyEqual.areEqual(candidate, firstCandidate))) {
426+
return firstCandidate;
427+
}
428+
429+
return originalTypeTree;
430+
}
431+
432+
private boolean isUnboundedWildcardParameterized(TypeTree typeTree) {
433+
JavaType type = typeTree.getType();
434+
if (!(type instanceof JavaType.Parameterized)) {
435+
return false;
436+
}
437+
return requireNonNull(((JavaType.Parameterized) type).getTypeParameters()).stream()
438+
.allMatch(typeParameter -> typeParameter instanceof JavaType.GenericTypeVariable &&
439+
((JavaType.GenericTypeVariable) typeParameter).getBounds().isEmpty() &&
440+
"?".equals(((JavaType.GenericTypeVariable) typeParameter).getName()));
441+
}
442+
369443
public @Nullable J processTypeCast(J.TypeCast typeCast, Cursor cursor) {
370444
J.InstanceOf instanceOf = replacements.get(typeCast);
371445
if (instanceOf != null && instanceOf.getPattern() != null) {
@@ -386,7 +460,9 @@ private String patternVariableName(J.InstanceOf instanceOf, Cursor cursor, Set<S
386460
}
387461

388462
public @Nullable J processVariableDeclarations(J.VariableDeclarations multiVariable) {
389-
return multiVariable.getVariables().stream().anyMatch(v -> variablesToDelete.values().stream().anyMatch(vd -> vd.getVariable() == v)) ? null : multiVariable;
463+
return multiVariable.getVariables().stream().anyMatch(v -> variablesToDelete.values().stream()
464+
.flatMap(Collection::stream)
465+
.anyMatch(vd -> vd.getVariable() == v)) ? null : multiVariable;
390466
}
391467
}
392468

‎src/test/java/org/openrewrite/staticanalysis/InstanceOfPatternMatchTest.java‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -521,6 +521,205 @@ void test(Object o) {
521521
);
522522
}
523523

524+
@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/305")
525+
@Test
526+
void doesNotIntroduceSelfAssignmentInElseIfBranches() {
527+
rewriteRun(
528+
//language=java
529+
java(
530+
"""
531+
class A {
532+
String test(Object obj, String fieldName) {
533+
if (obj instanceof Contact) {
534+
if (fieldName.equals("FIRST_NAME")) {
535+
Contact cont = (Contact) obj;
536+
return getContactLink(cont, null, cont.getFirstName());
537+
} else if (fieldName.equals("LAST_NAME")) {
538+
Contact cont = (Contact) obj;
539+
return getContactLink(cont, null, cont.getLastName());
540+
} else if (fieldName.equals("FULL_NAME")) {
541+
Contact cont = (Contact) obj;
542+
return getContactLink(cont, null, cont.getDisplayName());
543+
}
544+
}
545+
return "";
546+
}
547+
548+
String getContactLink(Contact contact, Object unused, String value) {
549+
return value;
550+
}
551+
552+
static class Contact {
553+
String getFirstName() {
554+
return "";
555+
}
556+
557+
String getLastName() {
558+
return "";
559+
}
560+
561+
String getDisplayName() {
562+
return "";
563+
}
564+
}
565+
}
566+
""",
567+
"""
568+
class A {
569+
String test(Object obj, String fieldName) {
570+
if (obj instanceof Contact cont) {
571+
if (fieldName.equals("FIRST_NAME")) {
572+
return getContactLink(cont, null, cont.getFirstName());
573+
} else if (fieldName.equals("LAST_NAME")) {
574+
return getContactLink(cont, null, cont.getLastName());
575+
} else if (fieldName.equals("FULL_NAME")) {
576+
return getContactLink(cont, null, cont.getDisplayName());
577+
}
578+
}
579+
return "";
580+
}
581+
582+
String getContactLink(Contact contact, Object unused, String value) {
583+
return value;
584+
}
585+
586+
static class Contact {
587+
String getFirstName() {
588+
return "";
589+
}
590+
591+
String getLastName() {
592+
return "";
593+
}
594+
595+
String getDisplayName() {
596+
return "";
597+
}
598+
}
599+
}
600+
"""
601+
)
602+
);
603+
}
604+
605+
@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/305")
606+
@Test
607+
void doesNotIntroduceSelfAssignmentWhenAliasTypeSpellingsDiffer() {
608+
rewriteRun(
609+
//language=java
610+
java(
611+
"""
612+
import java.util.List;
613+
614+
class A {
615+
void test(Object obj, boolean b) {
616+
if (obj instanceof List) {
617+
if (b) {
618+
List<?> list = (List<?>) obj;
619+
System.out.println(list.size());
620+
} else {
621+
List list = (List) obj;
622+
System.out.println(list.size());
623+
}
624+
}
625+
}
626+
}
627+
""",
628+
"""
629+
import java.util.List;
630+
631+
class A {
632+
void test(Object obj, boolean b) {
633+
if (obj instanceof List<?> list) {
634+
if (b) {
635+
System.out.println(list.size());
636+
} else {
637+
System.out.println(list.size());
638+
}
639+
}
640+
}
641+
}
642+
"""
643+
)
644+
);
645+
}
646+
647+
@Test
648+
void prefersStableTypeSpellingRegardlessOfBranchOrder() {
649+
rewriteRun(
650+
//language=java
651+
java(
652+
"""
653+
import java.util.List;
654+
655+
class A {
656+
void test(Object obj, boolean b) {
657+
if (obj instanceof List) {
658+
if (b) {
659+
List list = (List) obj;
660+
System.out.println(list.size());
661+
} else {
662+
List<?> list = (List<?>) obj;
663+
System.out.println(list.size());
664+
}
665+
}
666+
}
667+
}
668+
""",
669+
"""
670+
import java.util.List;
671+
672+
class A {
673+
void test(Object obj, boolean b) {
674+
if (obj instanceof List<?> list) {
675+
if (b) {
676+
System.out.println(list.size());
677+
} else {
678+
System.out.println(list.size());
679+
}
680+
}
681+
}
682+
}
683+
"""
684+
),
685+
//language=java
686+
java(
687+
"""
688+
import java.util.List;
689+
690+
class B {
691+
void test(Object obj, boolean b) {
692+
if (obj instanceof List) {
693+
if (b) {
694+
List<?> list = (List<?>) obj;
695+
System.out.println(list.size());
696+
} else {
697+
List list = (List) obj;
698+
System.out.println(list.size());
699+
}
700+
}
701+
}
702+
}
703+
""",
704+
"""
705+
import java.util.List;
706+
707+
class B {
708+
void test(Object obj, boolean b) {
709+
if (obj instanceof List<?> list) {
710+
if (b) {
711+
System.out.println(list.size());
712+
} else {
713+
System.out.println(list.size());
714+
}
715+
}
716+
}
717+
}
718+
"""
719+
)
720+
);
721+
}
722+
524723
@Test
525724
void conflictingVariableInBody() {
526725
rewriteRun(

0 commit comments

Comments
 (0)
⚡