Skip to content

Commit

Permalink
Improve recipes and excluded them from C# codebase if needed (#396)
Browse files Browse the repository at this point in the history 
* Improve recipes and excluded them from csharp codebase if needed

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Improve recipes and excluded them from csharp codebase if needed

* Update src/main/java/org/openrewrite/staticanalysis/NewStringBuilderBufferWithCharArgument.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Apply suggestions from code review

* Apply suggestions from code review

---------

Co-authored-by: Tim te Beek <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
  • Loading branch information
@jevanlingen @timtebeek @github-actions
3 people authored Nov 28, 2024
1 parent 14f7766 commit 0b00944
Show file tree
Hide file tree
Showing 22 changed files with 511 additions and 595 deletions.
20 changes: 11 additions & 9 deletions src/main/java/org/openrewrite/staticanalysis/AtomicPrimitiveEqualsUsesGet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,18 @@

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

public class AtomicPrimitiveEqualsUsesGet extends Recipe {

private static final Set<String> ATOMIC_PRIMITIVE_TYPES = new HashSet<>(Arrays.asList(
"java.util.concurrent.atomic.AtomicBoolean",
"java.util.concurrent.atomic.AtomicInteger",
"java.util.concurrent.atomic.AtomicLong"
));
public static final String ATOMIC_ATOMIC_BOOLEAN = "java.util.concurrent.atomic.AtomicBoolean";
public static final String ATOMIC_ATOMIC_INTEGER = "java.util.concurrent.atomic.AtomicInteger";
public static final String ATOMIC_ATOMIC_LONG = "java.util.concurrent.atomic.AtomicLong";

private static final List<String> ATOMIC_PRIMITIVE_TYPES = Arrays.asList(
ATOMIC_ATOMIC_BOOLEAN, ATOMIC_ATOMIC_INTEGER, ATOMIC_ATOMIC_LONG
);

@Override
public String getDisplayName() {
Expand All @@ -59,9 +61,9 @@ public Set<String> getTags() {
@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(Preconditions.or(
new UsesType<>("java.util.concurrent.atomic.AtomicBoolean", false),
new UsesType<>("java.util.concurrent.atomic.AtomicInteger", false),
new UsesType<>("java.util.concurrent.atomic.AtomicLong", false)
new UsesType<>(ATOMIC_ATOMIC_BOOLEAN, false),
new UsesType<>(ATOMIC_ATOMIC_INTEGER, false),
new UsesType<>(ATOMIC_ATOMIC_LONG, false)
), new JavaVisitor<ExecutionContext>() {
private final MethodMatcher aiMethodMatcher = new MethodMatcher("java.lang.Object equals(java.lang.Object)");

Expand Down
55 changes: 27 additions & 28 deletions src/main/java/org/openrewrite/staticanalysis/CaseInsensitiveComparisonsDoNotChangeCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
import java.util.Set;

public class CaseInsensitiveComparisonsDoNotChangeCase extends Recipe {

private static final MethodMatcher COMPARE_IGNORE_CASE_METHOD_MATCHER = new MethodMatcher("java.lang.String equalsIgnoreCase(java.lang.String)");
private static final MethodMatcher TO_LOWER_CASE_METHOD_MATCHER = new MethodMatcher("java.lang.String toLowerCase()");
private static final MethodMatcher TO_UPPER_CASE_METHOD_MATCHER = new MethodMatcher("java.lang.String toUpperCase()");

@Override
public String getDisplayName() {
return "CaseInsensitive comparisons do not alter case";
Expand All @@ -53,38 +58,32 @@ public Duration getEstimatedEffortPerOccurrence() {

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(new UsesMethod<>("java.lang.String equalsIgnoreCase(java.lang.String)"), new CaseInsensitiveComparisonVisitor<>());
}

private static class CaseInsensitiveComparisonVisitor<ExecutionContext> extends JavaIsoVisitor<ExecutionContext> {
private static final MethodMatcher COMPARE_IGNORE_CASE_METHOD_MATCHER = new MethodMatcher("java.lang.String equalsIgnoreCase(java.lang.String)");
private static final MethodMatcher TO_LOWER_CASE_METHOD_MATCHER = new MethodMatcher("java.lang.String toLowerCase()");
private static final MethodMatcher TO_UPPER_CASE_METHOD_MATCHER = new MethodMatcher("java.lang.String toUpperCase()");

@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) {
J.MethodInvocation mi = super.visitMethodInvocation(method, executionContext);
if (COMPARE_IGNORE_CASE_METHOD_MATCHER.matches(mi)) {
mi = mi.withArguments(ListUtils.map(mi.getArguments(), arg -> {
if (arg instanceof J.MethodInvocation && isChangeCaseMethod(arg)) {
return ((J.MethodInvocation) arg).getSelect();
return Preconditions.check(new UsesMethod<>(COMPARE_IGNORE_CASE_METHOD_MATCHER), new JavaIsoVisitor<ExecutionContext>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
J.MethodInvocation mi = super.visitMethodInvocation(method, ctx);
if (COMPARE_IGNORE_CASE_METHOD_MATCHER.matches(mi)) {
mi = mi.withArguments(ListUtils.map(mi.getArguments(), arg -> {
if (arg instanceof J.MethodInvocation && isChangeCaseMethod(arg)) {
return ((J.MethodInvocation) arg).getSelect();
}
return arg;
}));
if (isChangeCaseMethod(mi.getSelect())) {
J.MethodInvocation mChangeCase = (J.MethodInvocation) mi.getSelect();
mi = mi.withSelect(mChangeCase.getSelect());
}
return arg;
}));
if (isChangeCaseMethod(mi.getSelect())) {
J.MethodInvocation mChangeCase = (J.MethodInvocation) mi.getSelect();
mi = mi.withSelect(mChangeCase.getSelect());
}
return mi;
}
return mi;
}

private boolean isChangeCaseMethod(@Nullable J j) {
if (j instanceof J.MethodInvocation) {
J.MethodInvocation mi = (J.MethodInvocation) j;
return TO_LOWER_CASE_METHOD_MATCHER.matches(mi) || TO_UPPER_CASE_METHOD_MATCHER.matches(mi);
private boolean isChangeCaseMethod(@Nullable J j) {
if (j instanceof J.MethodInvocation) {
J.MethodInvocation mi = (J.MethodInvocation) j;
return TO_LOWER_CASE_METHOD_MATCHER.matches(mi) || TO_UPPER_CASE_METHOD_MATCHER.matches(mi);
}
return false;
}
return false;
}
});
}
}
156 changes: 67 additions & 89 deletions src/main/java/org/openrewrite/staticanalysis/CovariantEquals.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,23 @@
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.JavaTemplate;
import org.openrewrite.java.MethodMatcher;
import org.openrewrite.java.search.DeclaresMethod;
import org.openrewrite.java.service.AnnotationService;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.staticanalysis.csharp.CSharpFileChecker;

import java.time.Duration;
import java.util.Collections;
import java.util.Comparator;
import java.util.Set;
import java.util.stream.Stream;

@Incubating(since = "7.0.0")
public class CovariantEquals extends Recipe {

private static final MethodMatcher EQUALS_MATCHER = new MethodMatcher("* equals(..)");
private static final MethodMatcher EQUALS_OBJECT_MATCHER = new MethodMatcher("* equals(java.lang.Object)");
private static final AnnotationMatcher OVERRIDE_ANNOTATION = new AnnotationMatcher("@java.lang.Override");

@Override
public String getDisplayName() {
return "Covariant equals";
Expand All @@ -49,105 +53,79 @@ public Set<String> getTags() {
return Collections.singleton("RSPEC-S2162");
}

@Override
public Duration getEstimatedEffortPerOccurrence() {
return Duration.ofMinutes(5);
}

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
MethodMatcher objectEquals = new MethodMatcher("* equals(java.lang.Object)");
return new JavaIsoVisitor<ExecutionContext>() {

TreeVisitor<?, ExecutionContext> conditions = Preconditions.and(
new DeclaresMethod<>(EQUALS_MATCHER),
Preconditions.not(new DeclaresMethod<>(EQUALS_OBJECT_MATCHER)),
Preconditions.not(new CSharpFileChecker<>())
);
return Preconditions.check(conditions, Repeat.repeatUntilStable(new JavaIsoVisitor<ExecutionContext>() {
@Override
public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) {
J.ClassDeclaration cd = super.visitClassDeclaration(classDecl, ctx);
Stream<J.MethodDeclaration> mds = cd.getBody().getStatements().stream()
.filter(J.MethodDeclaration.class::isInstance)
.map(J.MethodDeclaration.class::cast);
if (cd.getKind() != J.ClassDeclaration.Kind.Type.Interface && mds.noneMatch(m -> objectEquals.matches(m, classDecl))) {
cd = (J.ClassDeclaration) new ChangeCovariantEqualsMethodVisitor(cd).visit(cd, ctx, getCursor().getParentOrThrow());
assert cd != null;
}
return cd;
}

class ChangeCovariantEqualsMethodVisitor extends JavaIsoVisitor<ExecutionContext> {
private final AnnotationMatcher OVERRIDE_ANNOTATION = new AnnotationMatcher("@java.lang.Override");

private final J.ClassDeclaration enclosingClass;

public ChangeCovariantEqualsMethodVisitor(J.ClassDeclaration enclosingClass) {
this.enclosingClass = enclosingClass;
public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) {
J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx);
J.ClassDeclaration enclosingClass = getCursor().dropParentUntil(p -> p instanceof J.ClassDeclaration).getValue();

/*
* Looking for "public boolean equals(EnclosingClassType)" as the method signature match.
* We'll replace it with "public boolean equals(Object)"
*/
JavaType.FullyQualified type = enclosingClass.getType();
if (type == null || type instanceof JavaType.Unknown) {
return m;
}

@Override
public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) {
J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx);
updateCursor(m);

/*
* Looking for "public boolean equals(EnclosingClassType)" as the method signature match.
* We'll replace it with "public boolean equals(Object)"
*/
JavaType.FullyQualified type = enclosingClass.getType();
if (type == null || type instanceof JavaType.Unknown) {
return m;
}

String ecfqn = type.getFullyQualifiedName();
if (m.hasModifier(J.Modifier.Type.Public) &&
m.getReturnTypeExpression() != null &&
String ecfqn = type.getFullyQualifiedName();
if (m.hasModifier(J.Modifier.Type.Public) && m.getReturnTypeExpression() != null &&
JavaType.Primitive.Boolean.equals(m.getReturnTypeExpression().getType()) &&
new MethodMatcher(ecfqn + " equals(" + ecfqn + ")").matches(m, enclosingClass)) {

if (!service(AnnotationService.class).matches(getCursor(), OVERRIDE_ANNOTATION)) {
m = JavaTemplate.builder("@Override").build()
.apply(updateCursor(m),
m.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName)));
}

/*
* Change parameter type to Object, and maybe change input parameter name representing the other object.
* This is because we prepend these type-checking replacement statements to the existing "equals(..)" body.
* Therefore we don't want to collide with any existing variable names.
*/
J.VariableDeclarations.NamedVariable oldParamName = ((J.VariableDeclarations) m.getParameters().get(0)).getVariables().get(0);
String paramName = "obj".equals(oldParamName.getSimpleName()) ? "other" : "obj";
m = JavaTemplate.builder("Object #{}").build()
if (!service(AnnotationService.class).matches(getCursor(), OVERRIDE_ANNOTATION)) {
m = JavaTemplate.builder("@Override").build()
.apply(updateCursor(m),
m.getCoordinates().replaceParameters(),
paramName);

/*
* We'll prepend this type-check and type-cast to the beginning of the existing
* equals(..) method body statements, and let the existing equals(..) method definition continue
* with the logic doing what it was doing.
*/
String equalsBodyPrefixTemplate = "if (#{} == this) return true;\n" +
"if (#{} == null || getClass() != #{}.getClass()) return false;\n" +
"#{} #{} = (#{}) #{};\n";
JavaTemplate equalsBodySnippet = JavaTemplate.builder(equalsBodyPrefixTemplate).contextSensitive().build();

assert m.getBody() != null;
Object[] params = new Object[]{
paramName,
paramName,
paramName,
enclosingClass.getSimpleName(),
oldParamName.getSimpleName(),
enclosingClass.getSimpleName(),
paramName
};

m = equalsBodySnippet.apply(new Cursor(getCursor().getParent(), m),
m.getBody().getStatements().get(0).getCoordinates().before(),
params);
m.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName)));
}

return m;
/*
* Change parameter type to Object, and maybe change input parameter name representing the other object.
* This is because we prepend these type-checking replacement statements to the existing "equals(..)" body.
* Therefore we don't want to collide with any existing variable names.
*/
J.VariableDeclarations.NamedVariable oldParamName = ((J.VariableDeclarations) m.getParameters().get(0)).getVariables().get(0);
String paramName = "obj".equals(oldParamName.getSimpleName()) ? "other" : "obj";
m = JavaTemplate.builder("Object #{}").build()
.apply(updateCursor(m),
m.getCoordinates().replaceParameters(),
paramName);

/*
* We'll prepend this type-check and type-cast to the beginning of the existing
* equals(..) method body statements, and let the existing equals(..) method definition continue
* with the logic doing what it was doing.
*/
String equalsBodyPrefixTemplate = "if (#{} == this) return true;\n" +
"if (#{} == null || getClass() != #{}.getClass()) return false;\n" +
"#{} #{} = (#{}) #{};\n";
JavaTemplate equalsBodySnippet = JavaTemplate.builder(equalsBodyPrefixTemplate).contextSensitive().build();

assert m.getBody() != null;
Object[] params = new Object[]{
paramName,
paramName,
paramName,
enclosingClass.getSimpleName(),
oldParamName.getSimpleName(),
enclosingClass.getSimpleName(),
paramName
};

m = equalsBodySnippet.apply(new Cursor(getCursor().getParent(), m),
m.getBody().getStatements().get(0).getCoordinates().before(),
params);
}

return m;
}
};
}));
}
}
28 changes: 13 additions & 15 deletions src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,22 +53,20 @@ public Duration getEstimatedEffortPerOccurrence() {

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return new EqualsAvoidsNullFromCompilationUnitStyle();
}

private static class EqualsAvoidsNullFromCompilationUnitStyle extends JavaIsoVisitor<ExecutionContext> {
@Override
public J visit(@Nullable Tree tree, ExecutionContext ctx) {
if (tree instanceof JavaSourceFile) {
JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree);
EqualsAvoidsNullStyle style = cu.getStyle(EqualsAvoidsNullStyle.class);
if (style == null) {
style = Checkstyle.equalsAvoidsNull();
return new JavaIsoVisitor<ExecutionContext>() {
@Override
public J visit(@Nullable Tree tree, ExecutionContext ctx) {
if (tree instanceof JavaSourceFile) {
JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree);
EqualsAvoidsNullStyle style = cu.getStyle(EqualsAvoidsNullStyle.class);
if (style == null) {
style = Checkstyle.equalsAvoidsNull();
}
return new EqualsAvoidsNullVisitor<>(style).visitNonNull(cu, ctx);
}
return new EqualsAvoidsNullVisitor<>(style).visitNonNull(cu, ctx);
//noinspection DataFlowIssue
return (J) tree;
}
//noinspection DataFlowIssue
return (J) tree;
}
};
}
}
Loading

0 comments on commit 0b00944

Please sign in to comment.