diff --git a/AGENTS.md b/AGENTS.md index 27916a372ab..d6df7c6bc7c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -125,6 +125,41 @@ JavaQuickFix quickFix = JavaQuickFix.newQuickFix("Remove redundant check") reportIssue(tree, "Message").withQuickFix(() -> quickFix); ``` +### Per-File State Cleanup + +Rules that accumulate state in collections or mutable fields during file analysis **must clear that state between files**. A single rule instance is reused across all files in a module. + +For `IssuableSubscriptionVisitor` subclasses, override `clearState()` — it is called automatically before and after each file: + +```java +@Rule(key = "S1234") +public class MyCheck extends IssuableSubscriptionVisitor { + + private final Set seen = new HashSet<>(); + + @Override + protected void clearState() { + seen.clear(); + } +} +``` + +For `BaseTreeVisitor` / `JavaFileScanner` implementations, clear state in `scanFile()` (typically in a `finally` block): + +```java +@Override +public void scanFile(JavaFileScannerContext context) { + this.context = context; + try { + scan(context.getTree()); + } finally { + myCollection.clear(); + } +} +``` + +**Do not** clear lazy-initialized caches derived from immutable configuration (e.g. parsed `@RuleProperty` values) — those are intentionally computed once per rule instance. + ### Dependency-Aware Rules When a rule should only apply when a specific library is present, or when its behaviour depends on a dependency version, implement `DependencyVersionAware` alongside the visitor base class: diff --git a/java-checks/pom.xml b/java-checks/pom.xml index 4d6679c1b10..c156f27fdd7 100644 --- a/java-checks/pom.xml +++ b/java-checks/pom.xml @@ -83,6 +83,12 @@ junit-jupiter test + + com.tngtech.archunit + archunit-junit5 + 1.5.0 + test + org.mockito mockito-core diff --git a/java-checks/src/main/java/org/sonar/java/checks/AbstractInSynchronizeChecker.java b/java-checks/src/main/java/org/sonar/java/checks/AbstractInSynchronizeChecker.java index ce7a27b183b..f865390c5bd 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/AbstractInSynchronizeChecker.java +++ b/java-checks/src/main/java/org/sonar/java/checks/AbstractInSynchronizeChecker.java @@ -40,6 +40,7 @@ public void setContext(JavaFileScannerContext context) { @Override public void leaveFile(JavaFileScannerContext context) { withinSynchronizedBlock.clear(); + super.leaveFile(context); } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/AccessibilityChangeOnRecordsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/AccessibilityChangeOnRecordsCheck.java index 13890f19f03..658adac8a7f 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/AccessibilityChangeOnRecordsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/AccessibilityChangeOnRecordsCheck.java @@ -108,4 +108,10 @@ private static Optional getIdentifierSymbol(MethodInvocationTree mit) { IdentifierTree identifier = (IdentifierTree) callee; return Optional.of(identifier.symbol()); } + @Override + protected void clearState() { + primaryTargets.clear(); + secondaryTargets.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/BoxedBooleanExpressionsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/BoxedBooleanExpressionsCheck.java index c29ee75521e..39637682d0f 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/BoxedBooleanExpressionsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/BoxedBooleanExpressionsCheck.java @@ -88,6 +88,9 @@ public class BoxedBooleanExpressionsCheck extends BaseTreeVisitor implements Jav @Override public void scanFile(JavaFileScannerContext context) { + ifStatementCache.clear(); + firstNullCheckCache.clear(); + safeSymbols.clear(); this.context = context; try { if (context.getSemanticModel() != null) { diff --git a/java-checks/src/main/java/org/sonar/java/checks/CatchUsesExceptionWithContextCheck.java b/java-checks/src/main/java/org/sonar/java/checks/CatchUsesExceptionWithContextCheck.java index 0c9852fa668..b0a2943f563 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/CatchUsesExceptionWithContextCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/CatchUsesExceptionWithContextCheck.java @@ -106,16 +106,19 @@ public class CatchUsesExceptionWithContextCheck extends BaseTreeVisitor implemen private Deque usageStatusStack; private Set exceptions; private Set exceptionIdentifiers; - private Set excludedCatchTrees = new HashSet<>(); + private final Set excludedCatchTrees = new HashSet<>(); @Override public void scanFile(JavaFileScannerContext context) { this.context = context; usageStatusStack = new ArrayDeque<>(); - if (context.getSemanticModel() != null) { - scan(context.getTree()); + try { + if (context.getSemanticModel() != null) { + scan(context.getTree()); + } + } finally { + excludedCatchTrees.clear(); } - excludedCatchTrees.clear(); } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/ClassVariableVisibilityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/ClassVariableVisibilityCheck.java index 8cd68cb9308..e9cde2e8f94 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/ClassVariableVisibilityCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/ClassVariableVisibilityCheck.java @@ -43,7 +43,12 @@ public class ClassVariableVisibilityCheck extends BaseTreeVisitor implements Jav @Override public void scanFile(JavaFileScannerContext context) { this.context = context; - scan(context.getTree()); + isClassStack.clear(); + try { + scan(context.getTree()); + } finally { + isClassStack.clear(); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/DateEnumsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/DateEnumsCheck.java index 85661fe36bf..44c9c18d730 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/DateEnumsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/DateEnumsCheck.java @@ -165,8 +165,7 @@ public class DateEnumsCheck extends AbstractMethodDetection implements JavaVersi private final Map> issuesByFile = new HashMap<>(); @Override - public void setContext(JavaFileScannerContext context) { - super.setContext(context); + protected void clearState() { importSupplier = null; currentFileTotalUsageCount = 0; currentFileNoEnumUsageCount = 0; @@ -175,7 +174,6 @@ public void setContext(JavaFileScannerContext context) { @Override public void leaveFile(JavaFileScannerContext context) { - importSupplier = null; CachedFileData restored = restoredFiles.remove(context.getInputFile()); if (restored != null) { projectTotalUsageCount -= restored.totalCount(); @@ -200,9 +198,7 @@ public void leaveFile(JavaFileScannerContext context) { } } - currentFileTotalUsageCount = 0; - currentFileNoEnumUsageCount = 0; - currentFileIssues.clear(); + super.leaveFile(context); } @Override @@ -354,6 +350,7 @@ public void endOfAnalysis(ModuleScannerContext context) { } projectTotalUsageCount = 0; projectTotalNoEnumUsageCount = 0; + currentFileIssues.clear(); issuesByFile.clear(); restoredFiles.clear(); } diff --git a/java-checks/src/main/java/org/sonar/java/checks/HardcodedURICheck.java b/java-checks/src/main/java/org/sonar/java/checks/HardcodedURICheck.java index ddd434ceebb..9407d0c4db6 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/HardcodedURICheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/HardcodedURICheck.java @@ -93,8 +93,7 @@ private record VariableData(Symbol symbol, String identifier, ExpressionTree ini private final List hardCodedUri = new ArrayList<>(); @Override - public void setContext(JavaFileScannerContext context) { - super.setContext(context); + protected void clearState() { annotationsStack.clear(); identifiersUsedInAnnotations.clear(); hardCodedUri.clear(); @@ -116,7 +115,8 @@ public void leaveFile(JavaFileScannerContext context) { } } - for (VariableData v : hardCodedUri) { + try { + for (VariableData v : hardCodedUri) { // equals to an identifier with unknown semantic, we cannot compare their symbols if (idNamesWithoutSemantic.contains(v.identifier())) { continue; @@ -128,6 +128,9 @@ public void leaveFile(JavaFileScannerContext context) { continue; } reportHardcodedURI(v.initializer()); + } + } finally { + super.leaveFile(context); } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/IndentationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/IndentationCheck.java index c75c8d95503..f09528d2045 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/IndentationCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/IndentationCheck.java @@ -67,7 +67,11 @@ public void scanFile(JavaFileScannerContext context) { excludeIssueAtLine = 0; this.context = context; fileLines = context.getFileLines(); - scan(context.getTree()); + try { + scan(context.getTree()); + } finally { + fileLines = null; + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/InnerStaticClassesCheck.java b/java-checks/src/main/java/org/sonar/java/checks/InnerStaticClassesCheck.java index a81094d04dc..f383958a40a 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/InnerStaticClassesCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/InnerStaticClassesCheck.java @@ -44,7 +44,14 @@ public class InnerStaticClassesCheck extends BaseTreeVisitor implements JavaFile @Override public void scanFile(final JavaFileScannerContext context) { this.context = context; - scan(context.getTree()); + outerClasses.clear(); + atLeastOneReference.clear(); + try { + scan(context.getTree()); + } finally { + outerClasses.clear(); + atLeastOneReference.clear(); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/LazyArgEvaluationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/LazyArgEvaluationCheck.java index 19cd9f66300..29bae332432 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/LazyArgEvaluationCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/LazyArgEvaluationCheck.java @@ -136,10 +136,15 @@ private static String[] testMethodNames(String[] lowerCaseNames) { @Override public void scanFile(JavaFileScannerContext context) { this.context = context; - if (context.getSemanticModel() == null) { - return; + treeStack.clear(); + try { + if (context.getSemanticModel() == null) { + return; + } + scan(context.getTree()); + } finally { + treeStack.clear(); } - scan(context.getTree()); } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/MissingPackageInfoCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MissingPackageInfoCheck.java index fdb811311d1..0744db6cc96 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MissingPackageInfoCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MissingPackageInfoCheck.java @@ -51,5 +51,7 @@ public void endOfAnalysis(ModuleScannerContext context) { for (String missingPackageInfo : missingPackageWithoutPackageFile) { context.addIssueOnProject(this, "Add a 'package-info.java' file to document the '" + missingPackageInfo + "' package"); } + missingPackageWithoutPackageFile.clear(); + knownPackageWithPackageFile.clear(); } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/MutableMembersUsageCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MutableMembersUsageCheck.java index 9bcfbce860a..cbe6b54eca1 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MutableMembersUsageCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MutableMembersUsageCheck.java @@ -313,10 +313,17 @@ private void addMethodReturningPrivateMutable(String methodSignature, Identifier @Override public void scanFile(final JavaFileScannerContext context) { - scan(context.getTree()); - dataPropagationGraph.reportMutableStoreReachableByOutsideCall(context, this); - dataPropagationGraph.reportMutableFieldReachingToOutside(context, this); - dataPropagationGraph.clear(); + parametersStack.clear(); + methodSignatureStack.clear(); + try { + scan(context.getTree()); + dataPropagationGraph.reportMutableStoreReachableByOutsideCall(context, this); + dataPropagationGraph.reportMutableFieldReachingToOutside(context, this); + } finally { + parametersStack.clear(); + methodSignatureStack.clear(); + dataPropagationGraph.clear(); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/NestedIfStatementsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/NestedIfStatementsCheck.java index 2578268d7c0..bdc52bbef55 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/NestedIfStatementsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/NestedIfStatementsCheck.java @@ -53,7 +53,11 @@ public class NestedIfStatementsCheck extends BaseTreeVisitor implements JavaFile public void scanFile(final JavaFileScannerContext context) { this.context = context; this.nestingLevel = new ArrayDeque<>(); - scan(context.getTree()); + try { + scan(context.getTree()); + } finally { + nestingLevel.clear(); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/OperatorPrecedenceCheck.java b/java-checks/src/main/java/org/sonar/java/checks/OperatorPrecedenceCheck.java index f2a68104a4f..ae59b2313ce 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/OperatorPrecedenceCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/OperatorPrecedenceCheck.java @@ -105,8 +105,13 @@ private static void put(Iterable firstSet, Iterable second public void scanFile(JavaFileScannerContext context) { this.context = context; reportedLines.clear(); - scan(context.getTree()); - reportedLines.clear(); + stack.clear(); + try { + scan(context.getTree()); + } finally { + stack.clear(); + reportedLines.clear(); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/RawExceptionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/RawExceptionCheck.java index 55967f1a051..a72e3338844 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/RawExceptionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/RawExceptionCheck.java @@ -64,7 +64,12 @@ public class RawExceptionCheck extends BaseTreeVisitor implements JavaFileScanne public void scanFile(JavaFileScannerContext context) { this.context = (FluentReporting) context; this.javaVersion = context.getJavaVersion(); - scan(context.getTree()); + exceptionsThrownByMethodInvocations.clear(); + try { + scan(context.getTree()); + } finally { + exceptionsThrownByMethodInvocations.clear(); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/RedundantTypeCastCheck.java b/java-checks/src/main/java/org/sonar/java/checks/RedundantTypeCastCheck.java index 03176e0d98f..69a46c55fcf 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/RedundantTypeCastCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/RedundantTypeCastCheck.java @@ -15,7 +15,6 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ package org.sonar.java.checks; - import java.util.Arrays; import java.util.List; import org.sonar.check.Rule; @@ -111,4 +110,9 @@ private static boolean matchesWarning(JWarning warning, TypeCastTree tree) { return tree.equals(warningTree); } + @Override + protected void clearState() { + warnings = null; + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/SelfAssignmentCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SelfAssignmentCheck.java index 8a33ed45f8d..608514a5305 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SelfAssignmentCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SelfAssignmentCheck.java @@ -15,7 +15,6 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ package org.sonar.java.checks; - import java.util.Arrays; import java.util.HashSet; import java.util.List; @@ -139,4 +138,9 @@ private void updateWarnings(AssignmentExpressionTree tree) { warnings.removeIf(warning -> tree.equals(warning.syntaxTree())); } + @Override + protected void clearState() { + warnings.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/ServletMethodsExceptionsThrownCheck.java b/java-checks/src/main/java/org/sonar/java/checks/ServletMethodsExceptionsThrownCheck.java index 9ee3742cfe7..7c0f541ef70 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/ServletMethodsExceptionsThrownCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/ServletMethodsExceptionsThrownCheck.java @@ -15,7 +15,6 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ package org.sonar.java.checks; - import java.util.ArrayDeque; import java.util.Arrays; import java.util.Collections; @@ -144,4 +143,10 @@ private boolean isNotCaught(Type type) { .noneMatch(type::isSubtypeOf); } + @Override + protected void clearState() { + shouldCheck.clear(); + tryCatches.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/SeveralBreakOrContinuePerLoopCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SeveralBreakOrContinuePerLoopCheck.java index ed338e48495..8fabfc7d476 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SeveralBreakOrContinuePerLoopCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SeveralBreakOrContinuePerLoopCheck.java @@ -47,7 +47,14 @@ public class SeveralBreakOrContinuePerLoopCheck extends BaseTreeVisitor implemen public void scanFile(JavaFileScannerContext context) { this.context = context; loopCount = 0; - scan(context.getTree()); + breakAndContinueCounter.clear(); + currentScopeIsSwitch.clear(); + try { + scan(context.getTree()); + } finally { + breakAndContinueCounter.clear(); + currentScopeIsSwitch.clear(); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/ShiftOnIntOrLongCheck.java b/java-checks/src/main/java/org/sonar/java/checks/ShiftOnIntOrLongCheck.java index 79e05b43585..e06f7f14ba0 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/ShiftOnIntOrLongCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/ShiftOnIntOrLongCheck.java @@ -57,8 +57,12 @@ public void setContext(JavaFileScannerContext context) { @Override public void leaveFile(JavaFileScannerContext context) { - for (int i = 0; i < shiftTrees.size(); i++) { - checkShiftTree(shiftTrees.get(i), i); + try { + for (int i = 0; i < shiftTrees.size(); i++) { + checkShiftTree(shiftTrees.get(i), i); + } + } finally { + shiftTrees.clear(); } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/StaticFieldInitializationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/StaticFieldInitializationCheck.java index c6d0fee683f..a309b145d23 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/StaticFieldInitializationCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/StaticFieldInitializationCheck.java @@ -54,19 +54,19 @@ public List nodesToVisit() { return nodesToVisit; } + @Override + protected void clearState() { + classWithSynchronizedMethod.clear(); + withinStaticInitializer.clear(); + methodUsesLocks.clear(); + } + @Override public void setContext(JavaFileScannerContext context) { + super.setContext(context); classWithSynchronizedMethod.push(false); withinStaticInitializer.push(false); methodUsesLocks.push(false); - super.setContext(context); - } - - @Override - public void leaveFile(JavaFileScannerContext context) { - withinStaticInitializer.clear(); - methodUsesLocks.clear(); - classWithSynchronizedMethod.clear(); } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/StaticMethodCheck.java b/java-checks/src/main/java/org/sonar/java/checks/StaticMethodCheck.java index 0d36d886a78..58e23ee3c4c 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/StaticMethodCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/StaticMethodCheck.java @@ -92,8 +92,13 @@ public class StaticMethodCheck extends BaseTreeVisitor implements JavaFileScanne @Override public void scanFile(JavaFileScannerContext context) { this.context = context; - if (context.getSemanticModel() != null) { - scan(context.getTree()); + methodReferences.clear(); + try { + if (context.getSemanticModel() != null) { + scan(context.getTree()); + } + } finally { + methodReferences.clear(); } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/SynchronizedClassUsageCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SynchronizedClassUsageCheck.java index 7e88b5b51f9..eb55e3cb70c 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SynchronizedClassUsageCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/SynchronizedClassUsageCheck.java @@ -15,7 +15,6 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ package org.sonar.java.checks; - import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collections; @@ -199,4 +198,10 @@ private boolean isDeprecatedType(Type symbolType) { } } + @Override + protected void clearState() { + exclusions.clear(); + visited.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/ThrowCheckedExceptionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/ThrowCheckedExceptionCheck.java index cf59da7acd7..a847c524ea1 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/ThrowCheckedExceptionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/ThrowCheckedExceptionCheck.java @@ -15,7 +15,6 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ package org.sonar.java.checks; - import java.util.Arrays; import java.util.Deque; import java.util.LinkedList; @@ -80,4 +79,9 @@ private static boolean isCompatibleWithThrows(Type exceptionType, List private static boolean isOverriding(MethodTree methodTree) { return Boolean.TRUE.equals(methodTree.isOverriding()); } + @Override + protected void clearState() { + methods.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/TypeParametersShadowingCheck.java b/java-checks/src/main/java/org/sonar/java/checks/TypeParametersShadowingCheck.java index e4063da1465..a07c8e21829 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/TypeParametersShadowingCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/TypeParametersShadowingCheck.java @@ -42,7 +42,11 @@ public class TypeParametersShadowingCheck extends BaseTreeVisitor implements Jav public void scanFile(JavaFileScannerContext context) { this.context = context; currentTypeParametersInScope = new HashMap<>(); - scan(context.getTree()); + try { + scan(context.getTree()); + } finally { + currentTypeParametersInScope.clear(); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/UnreachableCatchCheck.java b/java-checks/src/main/java/org/sonar/java/checks/UnreachableCatchCheck.java index 6961cfb645f..dbe82afb9ab 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/UnreachableCatchCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/UnreachableCatchCheck.java @@ -149,4 +149,9 @@ private static List childrenExceptionTypes(List targets, Map imp } return quickFix.build(); } + @Override + protected void clearState() { + imports.clear(); + importsNames.clear(); + duplicatedImports.clear(); + usedInJavaDoc.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java index 2693852b653..5820454c7f2 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java @@ -15,7 +15,6 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ package org.sonar.java.checks; - import java.util.HashSet; import java.util.List; import java.util.Optional; @@ -180,4 +179,9 @@ public void visitMemberSelectExpression(MemberSelectExpressionTree tree) { } + @Override + protected void clearState() { + visitedUnaryExpressions.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/WaitInWhileLoopCheck.java b/java-checks/src/main/java/org/sonar/java/checks/WaitInWhileLoopCheck.java index 74d5362ddb0..a872fe937c4 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/WaitInWhileLoopCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/WaitInWhileLoopCheck.java @@ -36,14 +36,14 @@ public class WaitInWhileLoopCheck extends AbstractMethodDetection { private Deque inWhileLoop = new LinkedList<>(); @Override - public void setContext(JavaFileScannerContext context) { - inWhileLoop.push(false); - super.setContext(context); + protected void clearState() { + inWhileLoop.clear(); } @Override - public void leaveFile(JavaFileScannerContext context) { - inWhileLoop.clear(); + public void setContext(JavaFileScannerContext context) { + super.setContext(context); + inWhileLoop.push(false); } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/design/BrainMethodCheck.java b/java-checks/src/main/java/org/sonar/java/checks/design/BrainMethodCheck.java index 8efe36b6f5f..ec651236805 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/design/BrainMethodCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/design/BrainMethodCheck.java @@ -128,6 +128,7 @@ public void endOfAnalysis(ModuleScannerContext context) { IssueFound issueFound = issuesFound.get(i); defaultContext.reportIssue(issueFound.analyzerMessage); } + issuesFound.clear(); } private static class IssueFound { diff --git a/java-checks/src/main/java/org/sonar/java/checks/design/ClassImportCouplingCheck.java b/java-checks/src/main/java/org/sonar/java/checks/design/ClassImportCouplingCheck.java index bc1c5658435..b1723453c67 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/design/ClassImportCouplingCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/design/ClassImportCouplingCheck.java @@ -51,9 +51,12 @@ public class ClassImportCouplingCheck extends AbstractCouplingChecker { @Override public void scanFile(JavaFileScannerContext context) { - super.scanFile(context); - imports = null; - secondaryLocations = null; + try { + super.scanFile(context); + } finally { + imports = null; + secondaryLocations = null; + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/security/CookieHttpOnlyCheck.java b/java-checks/src/main/java/org/sonar/java/checks/security/CookieHttpOnlyCheck.java index 5c3fdb06483..3e79c4e7dbe 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/security/CookieHttpOnlyCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/security/CookieHttpOnlyCheck.java @@ -146,24 +146,27 @@ private static final class ClassName { .build()); @Override - public void setContext(JavaFileScannerContext context) { + protected void clearState() { ignoredVariables.clear(); symbolConstructorMapToReport.clear(); settersToReport.clear(); newClassToReport.clear(); - super.setContext(context); } @Override public void leaveFile(JavaFileScannerContext context) { - for (TypeTree typeTree : symbolConstructorMapToReport.values()) { - reportIssue(typeTree, MESSAGE); - } - for (MethodInvocationTree mit : settersToReport) { - reportIssue(mit.arguments(), MESSAGE); - } - for (TypeTree typeTree : newClassToReport) { - reportIssue(typeTree, MESSAGE); + try { + for (TypeTree typeTree : symbolConstructorMapToReport.values()) { + reportIssue(typeTree, MESSAGE); + } + for (MethodInvocationTree mit : settersToReport) { + reportIssue(mit.arguments(), MESSAGE); + } + for (TypeTree typeTree : newClassToReport) { + reportIssue(typeTree, MESSAGE); + } + } finally { + super.leaveFile(context); } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/security/DebugFeatureEnabledCheck.java b/java-checks/src/main/java/org/sonar/java/checks/security/DebugFeatureEnabledCheck.java index eed4c082639..fb8dec3df25 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/security/DebugFeatureEnabledCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/security/DebugFeatureEnabledCheck.java @@ -15,7 +15,6 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ package org.sonar.java.checks.security; - import java.util.Arrays; import java.util.Deque; import java.util.LinkedList; @@ -132,4 +131,9 @@ private boolean enclosingClassExtendsThrowable() { return enclosingClass.peek() != null && enclosingClass.peek().type().isSubtypeOf("java.lang.Throwable"); } + @Override + protected void clearState() { + enclosingClass.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/security/SecureCookieCheck.java b/java-checks/src/main/java/org/sonar/java/checks/security/SecureCookieCheck.java index abbdea47430..99583497d57 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/security/SecureCookieCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/security/SecureCookieCheck.java @@ -148,24 +148,27 @@ public List nodesToVisit() { } @Override - public void setContext(JavaFileScannerContext context) { + protected void clearState() { unsecuredCookies.clear(); deletionCandidateCookies.clear(); cookieConstructors.clear(); maxAgeZeroSeen.clear(); deletionCandidateSecureCalls.clear(); enclosingClass.clear(); - super.setContext(context); } @Override public void leaveFile(JavaFileScannerContext context) { - cookieConstructors.forEach(r -> reportIssue(r.identifier(), MESSAGE)); - deletionCandidateSecureCalls.forEach((symbol, mit) -> { - if (!maxAgeZeroSeen.contains(symbol)) { - reportIssue(mit.arguments(), MESSAGE); - } - }); + try { + cookieConstructors.forEach(r -> reportIssue(r.identifier(), MESSAGE)); + deletionCandidateSecureCalls.forEach((symbol, mit) -> { + if (!maxAgeZeroSeen.contains(symbol)) { + reportIssue(mit.arguments(), MESSAGE); + } + }); + } finally { + super.leaveFile(context); + } } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/security/ZipEntryCheck.java b/java-checks/src/main/java/org/sonar/java/checks/security/ZipEntryCheck.java index 1b79764b7b9..b7715e9ed1f 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/security/ZipEntryCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/security/ZipEntryCheck.java @@ -15,7 +15,6 @@ * along with this program; if not, see https://sonarsource.com/license/ssal/ */ package org.sonar.java.checks.security; - import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -108,4 +107,9 @@ public void leaveNode(Tree tree) { private void report(MethodInvocationTree mit) { reportIssue(ExpressionUtils.methodName(mit), ISSUE_MESSAGE); } + @Override + protected void clearState() { + calls.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/SpringBeansShouldBeAccessibleCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/SpringBeansShouldBeAccessibleCheck.java index fdaf4d7a914..c2b853304a3 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/SpringBeansShouldBeAccessibleCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/SpringBeansShouldBeAccessibleCheck.java @@ -106,6 +106,9 @@ public void endOfAnalysis(ModuleScannerContext context) { // support sub-packages .filter(entry -> packagesScannedBySpringAtProjectLevel.stream().noneMatch(entry.getKey()::contains)) .forEach(entry -> entry.getValue().forEach(defaultContext::reportIssue)); + messagesPerPackage.clear(); + packagesScannedBySpringAtProjectLevel.clear(); + packagesScannedBySpringAtFileLevel.clear(); } @Override diff --git a/java-checks/src/main/java/org/sonar/java/checks/synchronization/DoubleCheckedLockingCheck.java b/java-checks/src/main/java/org/sonar/java/checks/synchronization/DoubleCheckedLockingCheck.java index 871add77f45..fb0fd4ae35f 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/synchronization/DoubleCheckedLockingCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/synchronization/DoubleCheckedLockingCheck.java @@ -200,4 +200,10 @@ public CriticalSection(SynchronizedStatementTree synchronizedTree, int ifStackDe } } + @Override + protected void clearState() { + ifFieldStack.clear(); + synchronizedStmtStack.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/synchronization/TwoLocksWaitCheck.java b/java-checks/src/main/java/org/sonar/java/checks/synchronization/TwoLocksWaitCheck.java index af57ee70405..1b3ca2379a5 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/synchronization/TwoLocksWaitCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/synchronization/TwoLocksWaitCheck.java @@ -159,4 +159,9 @@ void decrement() { } } + @Override + protected void clearState() { + synchronizedStack.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/tests/MockingAllMethodsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/tests/MockingAllMethodsCheck.java index 5dc6cfab069..7d4b8acbf5e 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/tests/MockingAllMethodsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/tests/MockingAllMethodsCheck.java @@ -106,4 +106,10 @@ private static boolean isNonPrivateMethod(Symbol symbol) { return symbol.isMethodSymbol() && !symbol.isPrivate() && declaration != null && !declaration.is(Tree.Kind.CONSTRUCTOR); } + @Override + protected void clearState() { + mockedMethodsPerObject.clear(); + whenCalls.clear(); + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/unused/UnusedPrivateFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/unused/UnusedPrivateFieldCheck.java index 206372ce7d4..700100539da 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/unused/UnusedPrivateFieldCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/unused/UnusedPrivateFieldCheck.java @@ -89,21 +89,16 @@ public List nodesToVisit() { return Arrays.asList(Tree.Kind.CLASS, Tree.Kind.RECORD, Tree.Kind.METHOD, Tree.Kind.EXPRESSION_STATEMENT, Tree.Kind.IDENTIFIER); } - @Override - public void setContext(JavaFileScannerContext context) { - clearState(); - super.setContext(context); - } - @Override public void leaveFile(JavaFileScannerContext context) { if (!hasNativeMethod) { classes.forEach(this::checkClassFields); } - clearState(); + super.leaveFile(context); } - private void clearState() { + @Override + protected void clearState() { classes.clear(); assignments.clear(); unknownIdentifiers.clear(); diff --git a/java-checks/src/test/java/org/sonar/java/checks/CheckStateCleanupTest.java b/java-checks/src/test/java/org/sonar/java/checks/CheckStateCleanupTest.java new file mode 100644 index 00000000000..ae90c1c682f --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/CheckStateCleanupTest.java @@ -0,0 +1,163 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import com.tngtech.archunit.base.DescribedPredicate; +import com.tngtech.archunit.core.domain.JavaClass; +import com.tngtech.archunit.core.domain.JavaCodeUnit; +import com.tngtech.archunit.core.domain.JavaField; +import com.tngtech.archunit.core.domain.JavaFieldAccess; +import com.tngtech.archunit.core.domain.JavaMethod; +import com.tngtech.archunit.core.domain.JavaMethodCall; +import com.tngtech.archunit.core.importer.ClassFileImporter; +import com.tngtech.archunit.core.importer.ImportOption; +import com.tngtech.archunit.lang.ArchCondition; +import com.tngtech.archunit.lang.ConditionEvents; +import com.tngtech.archunit.lang.SimpleConditionEvent; +import java.lang.reflect.Modifier; +import java.util.Collection; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.Test; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.ModuleScannerContext; +import org.sonar.plugins.java.api.internal.EndOfAnalysis; + +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; + +class CheckStateCleanupTest { + + private static final DescribedPredicate CHECK_CLASSES = new DescribedPredicate<>("check classes") { + @Override + public boolean test(JavaClass input) { + return input.isAnnotatedWith(Rule.class); + } + }; + + private static final ArchCondition CLEAR_COLLECTION_FIELDS = new ArchCondition<>("clear collection fields at file boundaries") { + @Override + public void check(JavaClass checkClass, ConditionEvents events) { + Set collectionFields = checkClass.getFields().stream() + .filter(field -> !Modifier.isStatic(field.reflect().getModifiers())) + .filter(CheckStateCleanupTest::isCollectionField) + .filter(field -> !isPropertyCache(field)) + .collect(java.util.stream.Collectors.toSet()); + Set lifecycleMethods = lifecycleMethods(checkClass); + for (JavaField field : collectionFields) { + if (lifecycleMethods.isEmpty()) { + events.add(SimpleConditionEvent.violated(checkClass, + checkClass.getFullName() + " has collection state but no lifecycle method")); + } else if (lifecycleMethods.stream().noneMatch(m -> clearsField(m, field, checkClass, new HashSet<>()))) { + events.add(SimpleConditionEvent.violated(checkClass, + checkClass.getFullName() + " does not clear " + field.getName() + " in any lifecycle method")); + } + } + } + }; + + @Test + void collection_fields_are_cleared_at_file_boundaries() { + classes().that(CHECK_CLASSES).should(CLEAR_COLLECTION_FIELDS).check(new ClassFileImporter() + .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) + .importPackages("org.sonar.java.checks")); + } + + private static boolean isCollectionField(JavaField field) { + return field.getRawType().isAssignableTo(Collection.class) || field.getRawType().isAssignableTo(Map.class); + } + + private static boolean isPropertyCache(JavaField field) { + var valueTypes = field.getAllInvolvedRawTypes().stream() + .filter(type -> !type.isAssignableTo(Collection.class) && !type.isAssignableTo(Map.class)) + .toList(); + return !valueTypes.isEmpty() && valueTypes.stream().allMatch(CheckStateCleanupTest::isPropertyType); + } + + private static boolean isPropertyType(JavaClass type) { + String name = type.getName(); + return name.equals(String.class.getName()) + || name.equals("java.math.BigDecimal") + || name.equals("java.util.regex.Pattern") + || name.equals("org.sonar.api.utils.WildcardPattern") + || name.equals("java.lang.Boolean") + || name.endsWith("PrimitiveCheck") + || type.isAssignableTo(Number.class); + } + + private static Set lifecycleMethods(JavaClass checkClass) { + if (checkClass.isAssignableTo(EndOfAnalysis.class)) { + return methodsNamed(checkClass, "endOfAnalysis", ModuleScannerContext.class); + } + if (checkClass.isAssignableTo(IssuableSubscriptionVisitor.class)) { + Set methods = noArgMethodsNamed(checkClass, "clearState"); + methods.addAll(methodsNamed(checkClass, "setContext", JavaFileScannerContext.class)); + methods.addAll(methodsNamed(checkClass, "leaveFile", JavaFileScannerContext.class)); + return methods; + } + return methodsNamed(checkClass, "scanFile", JavaFileScannerContext.class); + } + + private static Set methodsNamed(JavaClass checkClass, String name, Class parameterType) { + Set methods = new HashSet<>(); + for (JavaMethod method : checkClass.getMethods()) { + if (method.getName().equals(name) + && method.getRawParameterTypes().size() == 1 + && method.getRawParameterTypes().get(0).isEquivalentTo(parameterType)) { + methods.add(method); + } + } + return methods; + } + + private static Set noArgMethodsNamed(JavaClass checkClass, String name) { + Set methods = new HashSet<>(); + for (JavaMethod method : checkClass.getMethods()) { + if (method.getName().equals(name) && method.getRawParameterTypes().isEmpty()) { + methods.add(method); + } + } + return methods; + } + + private static boolean clearsField(JavaCodeUnit method, JavaField field, JavaClass checkClass, Set visited) { + if (!visited.add(method)) { + return false; + } + boolean clearsField = method.getMethodCallsFromSelf().stream() + .filter(CheckStateCleanupTest::isClearCall) + .anyMatch(call -> field.getAccessesToSelf().stream() + .anyMatch(access -> access.getOrigin().equals(method) && access.getLineNumber() == call.getLineNumber())); + boolean resetsField = field.getAccessesToSelf().stream() + .anyMatch(access -> access.getOrigin().equals(method) && access.getAccessType() == JavaFieldAccess.AccessType.SET); + if (clearsField || resetsField) { + return true; + } + return method.getMethodCallsFromSelf().stream() + .map(JavaMethodCall::getTarget) + .map(target -> target.resolveMember().orElse(null)) + .filter(javaMethod -> javaMethod != null && javaMethod.getOwner().equals(checkClass)) + .anyMatch(calledMethod -> clearsField(calledMethod, field, checkClass, visited)); + } + + private static boolean isClearCall(JavaMethodCall call) { + return call.getName().equals("clear") + && (call.getTargetOwner().isAssignableTo(Collection.class) || call.getTargetOwner().isAssignableTo(Map.class)); + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/MissingPackageInfoCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MissingPackageInfoCheckTest.java index 5f73965f384..513c27c10c2 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/MissingPackageInfoCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/MissingPackageInfoCheckTest.java @@ -20,7 +20,6 @@ import java.io.IOException; import java.io.InputStream; import java.security.NoSuchAlgorithmException; -import java.util.Set; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -80,9 +79,7 @@ void no_package_info() { .withCheck(check) .verifyIssueOnProject(EXPECTED_MESSAGE); - Set set = check.missingPackageWithoutPackageFile; - assertThat(set).hasSize(1); - assertThat(set.iterator().next()).isEqualTo(EXPECTED_PACKAGE); + assertThat(check.missingPackageWithoutPackageFile).isEmpty(); } @Test diff --git a/java-frontend/src/main/java/org/sonar/java/model/VisitorsBridge.java b/java-frontend/src/main/java/org/sonar/java/model/VisitorsBridge.java index be59a31d334..cbee2624114 100644 --- a/java-frontend/src/main/java/org/sonar/java/model/VisitorsBridge.java +++ b/java-frontend/src/main/java/org/sonar/java/model/VisitorsBridge.java @@ -428,15 +428,50 @@ public boolean scanWithoutParsing(InputFileScannerContext fileScannerContext) th @Override public void scanFile(JavaFileScannerContext javaFileScannerContext) { PerformanceMeasure.Duration issuableSubscriptionVisitorsDuration = PerformanceMeasure.start("IssuableSubscriptionVisitors"); + CheckFailureException primaryFailure = null; try { forEach(subscriptionVisitors, s -> s.setContext(javaFileScannerContext)); visit(javaFileScannerContext.getTree()); forEach(subscriptionVisitors, s -> s.leaveFile(javaFileScannerContext)); } catch (CheckFailureException e) { - interruptIfFailFast(e); + primaryFailure = e; } finally { - issuableSubscriptionVisitorsDuration.stop(); + CheckFailureException cleanupFailure; + try { + cleanupFailure = clearAllStates(); + } finally { + issuableSubscriptionVisitorsDuration.stop(); + } + CheckFailureException toReport = primaryFailure != null ? primaryFailure : cleanupFailure; + if (toReport != null) { + interruptIfFailFast(toReport); + } + } + } + + /** + * Clears the per-file state of every issuable subscription visitor, continuing even if some of them fail. + * + * @return the first cleanup failure, or {@code null} if all cleanups succeeded + */ + @Nullable + private CheckFailureException clearAllStates() { + CheckFailureException cleanupFailure = null; + for (SubscriptionVisitor visitor : subscriptionVisitors) { + if (visitor instanceof IssuableSubscriptionVisitor issuableVisitor) { + PerformanceMeasure.Duration visitorDuration = PerformanceMeasure.start(visitor); + try { + runScanner(issuableVisitor::clearStateAfterFile, visitor); + } catch (CheckFailureException e) { + if (cleanupFailure == null) { + cleanupFailure = e; + } + } finally { + visitorDuration.stop(); + } + } } + return cleanupFailure; } @Override diff --git a/java-frontend/src/main/java/org/sonar/plugins/java/api/IssuableSubscriptionVisitor.java b/java-frontend/src/main/java/org/sonar/plugins/java/api/IssuableSubscriptionVisitor.java index 00f8e7447a8..1754f769e55 100644 --- a/java-frontend/src/main/java/org/sonar/plugins/java/api/IssuableSubscriptionVisitor.java +++ b/java-frontend/src/main/java/org/sonar/plugins/java/api/IssuableSubscriptionVisitor.java @@ -26,6 +26,15 @@ */ public abstract class IssuableSubscriptionVisitor extends SubscriptionVisitor { + /** + * Override this method to release any per-file state (clear collections, null out AST references, etc.). + * It is called automatically before each file analysis (in {@link #setContext}) and after each file + * analysis (by the visitor runner), even if analysis fails or {@link #leaveFile} is overridden. + */ + protected void clearState() { + // Default: nothing to clear. + } + /** * Override this method only if you need to execute some instructions prior to start the analysis of a file. * Don't forget to call super.setContext()`. @@ -34,8 +43,8 @@ public abstract class IssuableSubscriptionVisitor extends SubscriptionVisitor { */ @Override public void setContext(JavaFileScannerContext context) { + clearState(); super.setContext(context); - // Explicitly declares the method to make it appears in the IssuableSubscriptionVisitor's members. } /** @@ -62,15 +71,9 @@ public final void scanFile(JavaFileScannerContext context) { throw new UnsupportedOperationException("IssuableSubscriptionVisitor should not drive visit of file. Use leaveFile() instead."); } - /** - * Override this method only if you need to execute some instructions after the end of the analysis of a file. - * - * @param context the analysis context. - */ - @Override - public void leaveFile(JavaFileScannerContext context) { - // Explicitly declares the method to make it appears in the IssuableSubscriptionVisitor's members. - // Default behaviour is to do nothing + /** Releases per-file state after analysis, independently of the leaveFile lifecycle callback. */ + public final void clearStateAfterFile() { + clearState(); } /** diff --git a/java-frontend/src/test/java/org/sonar/java/model/VisitorsBridgeTest.java b/java-frontend/src/test/java/org/sonar/java/model/VisitorsBridgeTest.java index b28ad3e1bff..76df3e194d3 100644 --- a/java-frontend/src/test/java/org/sonar/java/model/VisitorsBridgeTest.java +++ b/java-frontend/src/test/java/org/sonar/java/model/VisitorsBridgeTest.java @@ -29,6 +29,8 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.event.Level; import org.sonar.api.batch.fs.InputFile; import org.sonar.api.batch.sensor.SensorContext; @@ -169,6 +171,71 @@ void swallow_exception_when_hidden_property_set_to_false_with_IssuableSubscripti .containsOnly("IV1_ThrowingNPEVisitingClass - IV1"); } + @Test + void clear_state_when_leave_file_does_not_delegate() { + StatefulVisitorWithoutLeaveFileDelegation visitor = new StatefulVisitorWithoutLeaveFileDelegation(false); + + visitorsBridge(visitor, false).visitFile(COMPILATION_UNIT_TREE, false); + + assertThat(visitor.state).isFalse(); + assertThat(visitor.leaveCalled).isTrue(); + assertThat(visitor.clearCount).isEqualTo(2); + } + + @Test + void clear_state_when_visit_fails_before_leave_file() { + StatefulVisitorWithoutLeaveFileDelegation visitor = new StatefulVisitorWithoutLeaveFileDelegation(true); + + VisitorsBridge visitorsBridge = visitorsBridge(visitor, true); + + assertThatThrownBy(() -> visitorsBridge.visitFile(COMPILATION_UNIT_TREE, false)) + .isInstanceOf(AnalysisException.class); + + assertThat(visitor.state).isFalse(); + assertThat(visitor.leaveCalled).isFalse(); + assertThat(visitor.clearCount).isEqualTo(2); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void clear_all_visitors_state_when_cleanup_fails(boolean failOnException) { + RuntimeException firstFailure = new IllegalStateException("First cleanup failure"); + ThrowingCleanupVisitor firstVisitor = new ThrowingCleanupVisitor(firstFailure); + ThrowingCleanupVisitor secondVisitor = new ThrowingCleanupVisitor(new IllegalStateException("Second cleanup failure")); + StatefulVisitorWithoutLeaveFileDelegation lastVisitor = new StatefulVisitorWithoutLeaveFileDelegation(false); + VisitorsBridge visitorsBridge = visitorsBridge(List.of(firstVisitor, secondVisitor, lastVisitor), failOnException); + + if (failOnException) { + assertThatThrownBy(() -> visitorsBridge.visitFile(COMPILATION_UNIT_TREE, false)) + .isInstanceOf(AnalysisException.class) + .hasRootCause(firstFailure); + } else { + assertThatCode(() -> visitorsBridge.visitFile(COMPILATION_UNIT_TREE, false)).doesNotThrowAnyException(); + } + + assertThat(lastVisitor.state).isFalse(); + assertThat(lastVisitor.clearCount).isEqualTo(2); + assertThat(firstVisitor.cleanupCount).isEqualTo(2); + assertThat(secondVisitor.cleanupCount).isEqualTo(2); + assertThat(logTester.logs(Level.ERROR).stream() + .filter(message -> message.contains(ThrowingCleanupVisitor.class.getName()))) + .hasSize(2); + } + + @Test + void cleanup_failure_does_not_mask_visit_failure() { + StatefulVisitorWithoutLeaveFileDelegation failingVisitor = new StatefulVisitorWithoutLeaveFileDelegation(true); + ThrowingCleanupVisitor cleanupVisitor = new ThrowingCleanupVisitor(new IllegalStateException("Cleanup failure")); + VisitorsBridge visitorsBridge = visitorsBridge(List.of(failingVisitor, cleanupVisitor), true); + + assertThatThrownBy(() -> visitorsBridge.visitFile(COMPILATION_UNIT_TREE, false)) + .isInstanceOf(AnalysisException.class) + .hasRootCause(NPE); + + assertThat(failingVisitor.clearCount).isEqualTo(2); + assertThat(cleanupVisitor.cleanupCount).isEqualTo(2); + } + @Test void swallow_exception_when_hidden_property_set_to_false_with_all_kinds_of_visisitors() { assertThatCode(() -> { @@ -753,6 +820,65 @@ public void leaveNode(Tree tree) { } } + private static class StatefulVisitorWithoutLeaveFileDelegation extends IssuableSubscriptionVisitor { + private final boolean failOnVisit; + private boolean state; + private boolean leaveCalled; + private int clearCount; + + private StatefulVisitorWithoutLeaveFileDelegation(boolean failOnVisit) { + this.failOnVisit = failOnVisit; + } + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.COMPILATION_UNIT); + } + + @Override + public void setContext(JavaFileScannerContext context) { + super.setContext(context); + state = true; + } + + @Override + public void visitNode(Tree tree) { + if (failOnVisit) { + throw NPE; + } + } + + @Override + public void leaveFile(JavaFileScannerContext context) { + leaveCalled = true; + } + + @Override + protected void clearState() { + state = false; + clearCount++; + } + } + + private static class ThrowingCleanupVisitor extends StatefulVisitorWithoutLeaveFileDelegation { + private final RuntimeException failure; + private int cleanupCount; + + private ThrowingCleanupVisitor(RuntimeException failure) { + super(false); + this.failure = failure; + } + + @Override + protected void clearState() { + super.clearState(); + cleanupCount++; + if (cleanupCount == 2) { + throw failure; + } + } + } + private static class VisitorWithIncompatibleVersion extends IssuableSubscriptionVisitor implements EndOfAnalysis, JavaVersionAwareVisitor { @Override public List nodesToVisit() {