From 9ae32a8adbdc8da29252356e995a8b5c04414ab0 Mon Sep 17 00:00:00 2001 From: Bas Leijdekkers Date: Mon, 9 Jun 2025 14:52:36 +0200 Subject: [PATCH] Java: better mutability check for record components (IDEA-371018) GitOrigin-RevId: e4f351bcc297d41857406d177df00ff2696a7bfd --- .../codeInspection/dataFlow/DfaPsiUtil.java | 31 ++++--- .../codeInspection/dataFlow/Mutability.java | 84 ++++++++++--------- ...eturnOfFieldWithMutableTypeInspection.java | 17 +++- ...ignmentOrReturnOfFieldWithMutableType.java | 12 ++- 4 files changed, 90 insertions(+), 54 deletions(-) diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java index a04a4024f17f..d9d748f20b33 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaPsiUtil.java @@ -1,4 +1,4 @@ -// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.AnnotationUtil; @@ -37,6 +37,8 @@ import com.intellij.openapi.util.Ref; import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; import com.intellij.psi.controlFlow.ControlFlowUtil; +import com.intellij.psi.impl.light.LightCompactConstructorParameter; +import com.intellij.psi.impl.light.LightRecordField; import com.intellij.psi.search.LocalSearchScope; import com.intellij.psi.search.searches.DeepestSuperMethodsSearch; import com.intellij.psi.search.searches.ReferencesSearch; @@ -470,32 +472,41 @@ public final class DfaPsiUtil { return result; } - private static MultiMap getAllConstructorFieldInitializers(final PsiClass psiClass) { + public static List findAllConstructorInitializers(PsiRecordComponent component) { + PsiClass containingClass = component.getContainingClass(); + return containingClass == null || containingClass instanceof PsiCompiledElement + ? Collections.emptyList() + : new ArrayList<>(getAllConstructorFieldInitializers(containingClass).get(component)); + } + + private static MultiMap getAllConstructorFieldInitializers(PsiClass psiClass) { if (psiClass instanceof PsiCompiledElement) { return MultiMap.empty(); } return CachedValuesManager.getCachedValue(psiClass, new CachedValueProvider<>() { @Override - public @NotNull Result> compute() { + public @NotNull Result> compute() { final Set fieldNames = new HashSet<>(); for (PsiField field : psiClass.getFields()) { ContainerUtil.addIfNotNull(fieldNames, field.getName()); } - final MultiMap result = new MultiMap<>(); + final MultiMap result = new MultiMap<>(); JavaRecursiveElementWalkingVisitor visitor = new JavaRecursiveElementWalkingVisitor() { @Override public void visitAssignmentExpression(@NotNull PsiAssignmentExpression assignment) { super.visitAssignmentExpression(assignment); - PsiExpression lExpression = assignment.getLExpression(); PsiExpression rExpression = assignment.getRExpression(); if (rExpression != null && - lExpression instanceof PsiReferenceExpression && - fieldNames.contains(((PsiReferenceExpression)lExpression).getReferenceName())) { - PsiElement target = ((PsiReferenceExpression)lExpression).resolve(); - if (target instanceof PsiField && ((PsiField)target).getContainingClass() == psiClass) { - result.putValue((PsiField)target, rExpression); + assignment.getLExpression() instanceof PsiReferenceExpression ref && + fieldNames.contains(ref.getReferenceName())) { + PsiElement target = ref.resolve(); + if (target instanceof PsiField field && field.getContainingClass() == psiClass) { + result.putValue(field instanceof LightRecordField f ? f.getRecordComponent() : field, rExpression); + } + else if (target instanceof LightCompactConstructorParameter parameter) { + result.putValue(parameter.getRecordComponent(), rExpression); } } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java index 61967bf4994d..7f8e09a82826 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/Mutability.java @@ -1,4 +1,4 @@ -// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.AnnotationUtil; @@ -19,7 +19,6 @@ import com.intellij.psi.util.CachedValueProvider; import com.intellij.psi.util.CachedValuesManager; import com.intellij.psi.util.PsiModificationTracker; import com.intellij.testFramework.LightVirtualFile; -import com.intellij.util.ObjectUtils; import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.callMatcher.CallMatcher; import com.siyeh.ig.psiutils.ClassUtils; @@ -153,21 +152,21 @@ public enum Mutability { } private static @NotNull Mutability calcMutability(@NotNull PsiModifierListOwner owner) { - if (owner instanceof PsiParameter && owner.getParent() instanceof PsiParameterList list) { - PsiMethod method = ObjectUtils.tryCast(list.getParent(), PsiMethod.class); - if (method != null) { - int index = list.getParameterIndex((PsiParameter)owner); - JavaMethodContractUtil.ContractInfo contractInfo = JavaMethodContractUtil.getContractInfo(method); - if (contractInfo.isExplicit()) { - MutationSignature signature = contractInfo.getMutationSignature(); - if (signature.mutatesArg(index)) { - return MUTABLE; - } else if (signature.preservesArg(index)) { - return MUST_NOT_MODIFY; - } + if (owner instanceof PsiParameter parameter + && owner.getParent() instanceof PsiParameterList list + && list.getParent() instanceof PsiMethod method) { + JavaMethodContractUtil.ContractInfo contractInfo = JavaMethodContractUtil.getContractInfo(method); + if (contractInfo.isExplicit()) { + int index = list.getParameterIndex(parameter); + MutationSignature signature = contractInfo.getMutationSignature(); + if (signature.mutatesArg(index)) { + return MUTABLE; + } + else if (signature.preservesArg(index)) { + return MUST_NOT_MODIFY; } - return UNKNOWN; } + return UNKNOWN; } if (AnnotationUtil.isAnnotated(owner, Collections.singleton(UNMODIFIABLE_ANNOTATION), AnnotationUtil.CHECK_HIERARCHY | @@ -186,33 +185,40 @@ public enum Mutability { if (initializers.isEmpty() && !owner.hasModifierProperty(PsiModifier.STATIC)) { initializers = DfaPsiUtil.findAllConstructorInitializers(field); } - initializers = StreamEx.of(initializers).flatMap(ExpressionUtils::nonStructuralChildren).toList(); - if (initializers.isEmpty()) return UNKNOWN; - Mutability mutability = UNMODIFIABLE; - for (PsiExpression initializer : initializers) { - Mutability newMutability = UNKNOWN; - if (ClassUtils.isImmutable(initializer.getType())) { - newMutability = UNMODIFIABLE; - } else if (initializer instanceof PsiMethodCallExpression call) { - if (STREAM_COLLECT.test(call)) { - PsiExpression collector = call.getArgumentList().getExpressions()[0]; - newMutability = UNMODIFIABLE_COLLECTORS.matches(collector) ? UNMODIFIABLE : UNKNOWN; - } else if (STREAM_TO_LIST.test(call)) { - newMutability = UNMODIFIABLE; - } else { - PsiMethod method = call.resolveMethod(); - newMutability = method == null ? UNKNOWN : getMutability(method); - } - } - mutability = mutability.join(newMutability); - if (!mutability.isUnmodifiable()) break; - } - return mutability; + return calcMutability(initializers); } - return owner instanceof PsiMethodImpl ? JavaSourceInference.inferMutability((PsiMethodImpl)owner) : UNKNOWN; + else if (owner instanceof PsiRecordComponent component) { + return calcMutability(DfaPsiUtil.findAllConstructorInitializers(component)); + } + return owner instanceof PsiMethodImpl method ? JavaSourceInference.inferMutability(method) : UNKNOWN; + } + + private static @NotNull Mutability calcMutability(List expressions) { + expressions = StreamEx.of(expressions).flatMap(ExpressionUtils::nonStructuralChildren).toList(); + if (expressions.isEmpty()) return UNKNOWN; + Mutability mutability = UNMODIFIABLE; + for (PsiExpression initializer : expressions) { + Mutability newMutability = UNKNOWN; + if (ClassUtils.isImmutable(initializer.getType())) { + newMutability = UNMODIFIABLE; + } else if (initializer instanceof PsiMethodCallExpression call) { + if (STREAM_COLLECT.test(call)) { + PsiExpression collector = call.getArgumentList().getExpressions()[0]; + newMutability = UNMODIFIABLE_COLLECTORS.matches(collector) ? UNMODIFIABLE : UNKNOWN; + } else if (STREAM_TO_LIST.test(call)) { + newMutability = UNMODIFIABLE; + } else { + PsiMethod method = call.resolveMethod(); + newMutability = method == null ? UNKNOWN : getMutability(method); + } + } + mutability = mutability.join(newMutability); + if (!mutability.isUnmodifiable()) break; + } + return mutability; } public static Mutability fromDfType(DfType dfType) { - return dfType instanceof DfReferenceType ? ((DfReferenceType)dfType).getMutability() : UNKNOWN; + return dfType instanceof DfReferenceType referenceType ? referenceType.getMutability() : UNKNOWN; } } diff --git a/java/java-impl/src/com/siyeh/ig/encapsulation/AssignmentOrReturnOfFieldWithMutableTypeInspection.java b/java/java-impl/src/com/siyeh/ig/encapsulation/AssignmentOrReturnOfFieldWithMutableTypeInspection.java index 579bae0d39a2..5185ffbcf8ab 100644 --- a/java/java-impl/src/com/siyeh/ig/encapsulation/AssignmentOrReturnOfFieldWithMutableTypeInspection.java +++ b/java/java-impl/src/com/siyeh/ig/encapsulation/AssignmentOrReturnOfFieldWithMutableTypeInspection.java @@ -147,10 +147,19 @@ public final class AssignmentOrReturnOfFieldWithMutableTypeInspection extends Ba final String type = TypeUtils.expressionHasTypeOrSubtype(returnValue, MUTABLE_TYPES); if (type == null && !(returnValue.getType() instanceof PsiArrayType)) return; if (CollectionUtils.isConstantEmptyArray(field) || - field.hasModifierProperty(PsiModifier.FINAL) && field.hasModifierProperty(PsiModifier.STATIC) && field.getType() instanceof PsiArrayType && field.getInitializer() == null || ClassUtils.isImmutable(field.getType()) || - Mutability.getMutability(field).isUnmodifiable()) return; - registerError(returnValue, field, returnValue, type, Boolean.FALSE); + Mutability.getMutability(field).isUnmodifiable()) { + return; + } + if (field.hasModifierProperty(PsiModifier.FINAL) + && field.hasModifierProperty(PsiModifier.STATIC) + && field.getType() instanceof PsiArrayType + && field.getInitializer() == null) { + return; + } + PsiElement nameElement = ref.getReferenceNameElement(); + if (nameElement == null) return; + registerError(nameElement, field, returnValue, type, Boolean.FALSE); } @Override @@ -161,7 +170,7 @@ public final class AssignmentOrReturnOfFieldWithMutableTypeInspection extends Ba boolean reportAssignment = !ContainerUtil.or(recordClass.getConstructors(), c -> JavaPsiRecordUtil.isExplicitCanonicalConstructor(c)); for (PsiRecordComponent component : recordHeader.getRecordComponents()) { final PsiType type = component.getType(); - if (ClassUtils.isImmutable(type)) continue; + if (ClassUtils.isImmutable(type) || Mutability.getMutability(component).isUnmodifiable()) continue; final boolean mutable = type instanceof PsiArrayType || ContainerUtil.exists(MUTABLE_TYPES, typeName -> InheritanceUtil.isInheritor(type, typeName)); if (!mutable) continue; diff --git a/java/java-tests/testData/ig/com/siyeh/igtest/encapsulation/assignment_or_return_of_field_with_mutable_type/AssignmentOrReturnOfFieldWithMutableType.java b/java/java-tests/testData/ig/com/siyeh/igtest/encapsulation/assignment_or_return_of_field_with_mutable_type/AssignmentOrReturnOfFieldWithMutableType.java index 9965b8e7f46c..c51d3cc110fd 100644 --- a/java/java-tests/testData/ig/com/siyeh/igtest/encapsulation/assignment_or_return_of_field_with_mutable_type/AssignmentOrReturnOfFieldWithMutableType.java +++ b/java/java-tests/testData/ig/com/siyeh/igtest/encapsulation/assignment_or_return_of_field_with_mutable_type/AssignmentOrReturnOfFieldWithMutableType.java @@ -68,7 +68,7 @@ class ReturnOfCollectionFieldInspection public Set foo() { - return m_foo; + return this.m_foo; } public List fooBar() @@ -228,4 +228,14 @@ record Data4(Collection strings) { } } record Fooz(ImmutableList ints) { +} +record Safe1(List values) { + Safe1 { + values = List.copyOf(values); + } +} +record Safe2(List values) { + Safe2(List values) { + this.values = List.copyOf(values); + } } \ No newline at end of file