MDS/openide
1
0
Fork 0
mirror of https://gitflic.ru/project/openide/openide.git synced 2026-09-27 10:03:11 +07:00
Code Issues Packages Projects Releases Wiki Activity

[java-inspection] IJ-CR-119988 IDEA-264141 Warn if compact constructor calls methods that access fields

Browse Source 
- support chain constructors with one target
- added more tests

GitOrigin-RevId: 4e9d7680cb3e74d3e6343fbe2c32c6b476bb04e5
This commit is contained in:
Mikhail Pyltsin
2023-11-29 14:18:25 +00:00
committed by intellij-monorepo-bot
parent 7bef3fb7b8
commit 5f6d948648
2 changed files with 175 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/jvm/descriptors/PlainDescriptor.java
+81 -52
View File
@@ -23,10 +23,7 @@ import com.siyeh.ig.psiutils.VariableAccessUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;
import java.util.*;
import java.util.function.Predicate;
/**
@@ -168,32 +165,23 @@ public final class PlainDescriptor extends PsiVarDescriptor {
return getAccessOffset(placeMethod) < getWriteOffset(target);
}
private record TargetCallInfo(@NotNull PsiMethod constructor,
@NotNull PsiMethodCallExpression call){}
private static boolean methodCanBeCalledFromConstructorBeforeFieldInitializing(@NotNull PsiField target,
@NotNull PsiMethod method,
@NotNull PsiClass placeClass) {
if (target.hasInitializer() || method.isConstructor() ||
//consider cases with only one constructor to do it faster
placeClass.getConstructors().length != 1) {
if (target.hasInitializer() || method.isConstructor()) {
return false;
}
PsiMethod constructor = placeClass.getConstructors()[0];
PsiCodeBlock constructorBody = constructor.getBody();
if (constructorBody == null ||
constructorBody.getStatements().length == 0) {
TargetCallInfo callInfo = findTargetCallIn(target, placeClass, method);
if (callInfo == null) {
return false;
}
PsiMethodCallExpression callInConstructor = JavaPsiConstructorUtil.findThisOrSuperCallInConstructor(constructor);
if (callInConstructor != null) {
return false;
}
PsiMethodCallExpression methodCallExpression = findCallIn(target, placeClass, method);
if (methodCallExpression == null) {
return false;
}
if (JavaPsiRecordUtil.isCompactConstructor(constructor)) {
if (JavaPsiRecordUtil.isCompactConstructor(callInfo.constructor())) {
return true;
}
if (!VariableAccessUtils.variableIsAssignedAtPoint(target, constructor, methodCallExpression)) {
if (!VariableAccessUtils.variableIsAssignedAtPoint(target, callInfo.constructor, callInfo.call)) {
return true;
}
return false;
@@ -201,9 +189,11 @@ public final class PlainDescriptor extends PsiVarDescriptor {
/**
* @param methods - grouped calls and corresponded methods by their name
* @param constructor - constructor where these calls are placed
* @param hasCallOutside - there are any calls, except instance calls of the same class on `this` qualifier
*/
private record ConstructorMethodInfo(Map<String, Map<PsiMethod, PsiMethodCallExpression>> methods,
private record ConstructorMethodInfo(@NotNull Map<String, Map<PsiMethod, PsiMethodCallExpression>> methods,
@Nullable PsiMethod constructor,
boolean hasCallOutside) {
}
@@ -211,26 +201,49 @@ public final class PlainDescriptor extends PsiVarDescriptor {
* Find a method call expression in the given field's context class.
* It is supposed that context class contains only one constructor for simplification
*
* @param field The field to search for method calls.
* @param contextClass The context class. The first constructor of this class is used to search for method calls.
* @param method The method to look for within the context class.
* @param field The field to search for method calls.
* @param contextClass The context class. The first constructor of this class is used to search for method calls.
* @param method The method to look for within the context class.
* @return The found method call expression, or null if not found.
*/
@Nullable
private static PsiMethodCallExpression findCallIn(@NotNull PsiField field,
@NotNull PsiClass contextClass,
@NotNull PsiMethod method) {
PsiMethod constructor = contextClass.getConstructors()[0];
if (!constructor.isPhysical() || constructor.getBody() == null) {
return null;
private static TargetCallInfo findTargetCallIn(@NotNull PsiField field,
@NotNull PsiClass contextClass,
@NotNull PsiMethod method) {
ConstructorMethodInfo constructorMethodInfo = getConstructorMethodInfo(contextClass);
PsiManager psiManager = contextClass.getManager();
if (field.hasModifierProperty(PsiModifier.FINAL) ||
//simplification, that there is no other way to initialize fields
hasOnlyOneInsideCall(constructorMethodInfo)) {
Map<PsiMethod, PsiMethodCallExpression> methodsByCall = constructorMethodInfo.methods().get(method.getName());
if (methodsByCall == null) {
return null;
}
for (PsiMethod methodByCall : methodsByCall.keySet()) {
if (psiManager.areElementsEquivalent(methodByCall, method)) {
PsiMethodCallExpression call = methodsByCall.get(methodByCall);
PsiMethod constructor = constructorMethodInfo.constructor();
if (call != null && constructor != null) {
return new TargetCallInfo(constructor, call) ;
}
}
}
}
return null;
}
ConstructorMethodInfo cacheValue = CachedValuesManager.getCachedValue(contextClass, () -> {
PsiMethod context = contextClass.getConstructors()[0];
@NotNull
private static ConstructorMethodInfo getConstructorMethodInfo(@NotNull PsiClass contextClass) {
return CachedValuesManager.getCachedValue(contextClass, () -> {
CachedValueProvider.Result<ConstructorMethodInfo> emptyResult =
CachedValueProvider.Result.create(new ConstructorMethodInfo(Map.of(), null, false), PsiModificationTracker.MODIFICATION_COUNT);
PsiMethod context = findOnlyOneNotChainConstructor(contextClass);
if(context == null) return emptyResult;
PsiManager psiManager = context.getManager();
var visitor = new JavaRecursiveElementWalkingVisitor() {
final ConcurrentHashMap<String, Map<PsiMethod, PsiMethodCallExpression>> collectedMethods = new ConcurrentHashMap<>();
final Map<String, Map<PsiMethod, PsiMethodCallExpression>> collectedMethods = new HashMap<>();
boolean callsOutside = false;
@Override
public void visitLambdaExpression(@NotNull PsiLambdaExpression expression) { }
@@ -262,27 +275,43 @@ public final class PlainDescriptor extends PsiVarDescriptor {
}
};
context.accept(visitor);
return CachedValueProvider.Result.create(new ConstructorMethodInfo(visitor.collectedMethods, visitor.callsOutside),
PsiModificationTracker.MODIFICATION_COUNT);
ConstructorMethodInfo info = new ConstructorMethodInfo(visitor.collectedMethods, context, visitor.callsOutside);
return CachedValueProvider.Result.create(info, PsiModificationTracker.MODIFICATION_COUNT);
});
PsiManager psiManager = contextClass.getManager();
if (field.hasModifierProperty(PsiModifier.FINAL) ||
//simplification, that there is no another way to initialize fields
hasOnlyOneInsideCall(cacheValue)) {
Map<PsiMethod, PsiMethodCallExpression> methodsByCall = cacheValue.methods().get(method.getName());
if (methodsByCall == null) {
return null;
}
for (PsiMethod methodByCall : methodsByCall.keySet()) {
if (psiManager.areElementsEquivalent(methodByCall, method)) {
return methodsByCall.get(methodByCall);
}
}
}
return null;
}
private static boolean hasOnlyOneInsideCall(ConstructorMethodInfo constructorMethodInfo) {
/**
* Finds the only constructor in the given context class that is not a chain constructor and doesn't contain super call
*
* @param contextClass The context class to search for constructors.
* @return The found constructor, or null if not found.
*/
@Nullable
private static PsiMethod findOnlyOneNotChainConstructor(@NotNull PsiClass contextClass) {
List<PsiMethod> notChainConstructors = new ArrayList<>();
for (PsiMethod constructor : contextClass.getConstructors()) {
PsiMethodCallExpression callInConstructor = JavaPsiConstructorUtil.findThisOrSuperCallInConstructor(constructor);
if (JavaPsiConstructorUtil.isChainedConstructorCall(callInConstructor)) {
continue;
}
notChainConstructors.add(constructor);
}
if (notChainConstructors.size() != 1) {
return null;
}
PsiMethod targetConstructor = notChainConstructors.get(0);
PsiCodeBlock constructorBody = targetConstructor.getBody();
if (constructorBody == null || constructorBody.getStatements().length == 0) {
return null;
}
if (JavaPsiConstructorUtil.findThisOrSuperCallInConstructor(targetConstructor) != null) {
return null;
}
return targetConstructor;
}
private static boolean hasOnlyOneInsideCall(@NotNull ConstructorMethodInfo constructorMethodInfo) {
Map<String, Map<PsiMethod, PsiMethodCallExpression>> methods = constructorMethodInfo.methods();
if (constructorMethodInfo.hasCallOutside() || methods.size() != 1) {
return false;
java/java-tests/testData/inspection/dataFlow/fixture/CallsBeforeFieldInitializing.java
+94
View File
@@ -122,4 +122,98 @@ class OrdinaryClassWithClassQualifier {
throw new IllegalArgumentException();
}
}
}
class Outer {
public void main(String[] args) {
Inner inner = new Inner();
}
void foo() {
System.out.println("Outer");
}
class Inner extends Outer {
final String s;
Inner() {
Outer.this.foo();
this.s = "hello";
}
@Override
void foo() {
System.out.println("Inner");
System.out.println(s .trim());
}
}
}
class SuperTest {
public static void main(String[] args) {
new Child();
}
public SuperTest() {
init();
}
protected void init() {
}
static class Child extends SuperTest {
String t;
public Child() {
super();
check();
}
@Override
protected void init() {
t = "init";
}
private void check() {
System.out.println(t.length());
}
}
}
class ChainWithOneTarget {
private final String t;
public ChainWithOneTarget() {
this("test");
}
public ChainWithOneTarget(String t) {
test();
this.t = t;
}
private void test() {
System.out.println(t.<warning descr="Method invocation 'length' may produce 'NullPointerException'">length</warning>());
}
}
class ChainWithTwoTarget {
private final String t;
public ChainWithTwoTarget() {
this("test");
test();
}
public ChainWithTwoTarget(String t) {
this.t = t;
}
public ChainWithTwoTarget(String t, int i) {
this.t = t;
test();
}
private void test() {
System.out.println(t.length());
}
}