From d6f8415af33ea8e3c2bad1882db948a652f15fde Mon Sep 17 00:00:00 2001 From: Steve Elliott Date: Tue, 15 Sep 2026 11:53:13 -0400 Subject: [PATCH] Add FindThreadStartInConstructor (Sonar S2693) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Search recipe for the shape flagged by sonar-java ThreadStartedInConstructorCheck (RSPEC-S2693): Thread.start() reached during construction of a non-final class. The new thread can observe a partially-initialised object, and any subclass' fields are guaranteed unset because the superclass constructor starts the thread before the subclass' own initialisation runs. Fits alongside FindVirtualThreadOpportunities in org.openrewrite.java.migrate.lang — both are "your thread management could be modernised" search recipes; S2693's remedy is to move the start() out of the construction path and use an ExecutorService. Detection walks getCursor().getPath() from a Thread.start() call outward to find the enclosing frame: - J.Lambda -> not construction (deferred) - J.Block with isStatic() -> static initializer, not construction - J.MethodDeclaration - constructor -> check enclosing class - regular method -> not construction - J.NewClass with body -> anonymous class, effectively final - J.ClassDeclaration (first hit) -> instance field init / init block -> check class MethodMatcher uses matchOverrides=true so Thread subclasses fire on myThread.start() too. Records and enums are treated as effectively final. Tagged RSPEC-S2693. No CWE — sonar-java doesn't map this rule to one either. --- .../lang/FindThreadStartInConstructor.java | 125 ++++++++++ .../resources/META-INF/rewrite/recipes.csv | 1 + .../FindThreadStartInConstructorTest.java | 231 ++++++++++++++++++ 3 files changed, 357 insertions(+) create mode 100644 src/main/java/org/openrewrite/java/migrate/lang/FindThreadStartInConstructor.java create mode 100644 src/test/java/org/openrewrite/java/migrate/lang/FindThreadStartInConstructorTest.java diff --git a/src/main/java/org/openrewrite/java/migrate/lang/FindThreadStartInConstructor.java b/src/main/java/org/openrewrite/java/migrate/lang/FindThreadStartInConstructor.java new file mode 100644 index 0000000000..41554a8fed --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/lang/FindThreadStartInConstructor.java @@ -0,0 +1,125 @@ +/* + * Copyright 2026 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.lang; + +import lombok.EqualsAndHashCode; +import lombok.Value; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Preconditions; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.search.UsesMethod; +import org.openrewrite.java.tree.J; +import org.openrewrite.marker.SearchResult; + +import java.util.HashSet; +import java.util.Iterator; +import java.util.Set; + +import static java.util.Arrays.asList; + +@Value +@EqualsAndHashCode(callSuper = false) +public class FindThreadStartInConstructor extends Recipe { + + String displayName = "Find `Thread.start()` calls made during construction of a non-final class"; + + String description = "Finds `Thread.start()` invocations reached during construction of a " + + "non-`final` class — from a constructor body, an instance field initializer, or an " + + "instance initializer block. Starting a thread before construction completes lets the " + + "new thread observe a partially-initialised object; the problem is compounded when a " + + "subclass extends the class, because the superclass constructor starts the thread " + + "before the subclass' own fields have been initialised. Move the `start()` call to a " + + "separate method callers invoke after construction, or declare the class `final`."; + + Set tags = new HashSet<>(asList("RSPEC-S2693")); + + private static final MethodMatcher THREAD_START = new MethodMatcher("java.lang.Thread start()", true); + + @Override + public TreeVisitor getVisitor() { + return Preconditions.check( + new UsesMethod<>(THREAD_START), + new JavaIsoVisitor() { + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { + J.MethodInvocation mi = super.visitMethodInvocation(method, ctx); + if (!THREAD_START.matches(mi)) { + return mi; + } + if (isDuringConstructionOfNonFinalClass()) { + return SearchResult.found(mi, + "`Thread.start()` called during construction of a non-final class. " + + "The new thread can observe a partially-initialised object, and " + + "any subclass' fields are guaranteed unset. Move the call to a " + + "separate method or declare the class `final`."); + } + return mi; + } + + /** + * Walks up from the invocation site looking for the first frame that decides + * the context: a lambda / static initializer / regular method → not construction; + * a constructor → in construction; a class boundary reached before any of those + * → we're in an instance field initializer or instance init block, also in + * construction. Anonymous classes are treated as effectively final. + */ + private boolean isDuringConstructionOfNonFinalClass() { + for (Iterator it = getCursor().getPath(); it.hasNext(); ) { + Object p = it.next(); + if (p instanceof J.Lambda) { + return false; + } + if (p instanceof J.Block && ((J.Block) p).isStatic()) { + return false; + } + if (p instanceof J.MethodDeclaration) { + J.MethodDeclaration md = (J.MethodDeclaration) p; + if (!md.isConstructor()) { + return false; + } + return !enclosingClassIsFinal(); + } + if (p instanceof J.NewClass && ((J.NewClass) p).getBody() != null) { + // Inside an anonymous class body — effectively final, can't be extended. + return false; + } + if (p instanceof J.ClassDeclaration) { + return !isEffectivelyFinal((J.ClassDeclaration) p); + } + } + return false; + } + + private boolean enclosingClassIsFinal() { + J.ClassDeclaration cd = getCursor().firstEnclosing(J.ClassDeclaration.class); + return cd != null && isEffectivelyFinal(cd); + } + + private boolean isEffectivelyFinal(J.ClassDeclaration cd) { + if (cd.hasModifier(J.Modifier.Type.Final)) { + return true; + } + J.ClassDeclaration.Kind.Type kind = cd.getKind(); + return kind == J.ClassDeclaration.Kind.Type.Record || + kind == J.ClassDeclaration.Kind.Type.Enum; + } + } + ); + } +} diff --git a/src/main/resources/META-INF/rewrite/recipes.csv b/src/main/resources/META-INF/rewrite/recipes.csv index 6dd73c0427..af5d7289b4 100644 --- a/src/main/resources/META-INF/rewrite/recipes.csv +++ b/src/main/resources/META-INF/rewrite/recipes.csv @@ -365,6 +365,7 @@ maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.l This is a strictly behavior-preserving transformation: argument expressions are already evaluated before the delegate constructor body runs, and such an argument can never reference the instance under construction, so hoisting them into preceding statements changes neither the order of side effects nor the set of legal references. Arguments are extracted in their original left-to-right order, and trivial arguments (literals and local variable references, which have no side effects) are left in place. Statements that follow the constructor invocation are deliberately *not* moved, as reordering them relative to the delegate constructor's side effects could change behavior.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.FindNonVirtualExecutors,Find non-virtual `ExecutorService` creation,Find all places where static `java.util.concurrent.Executors` method creates a non-virtual `java.util.concurrent.ExecutorService`. This recipe can be used to search fro `ExecutorService` that can be replaced by Virtual Thread executor.,7,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,,"[{""name"":""org.openrewrite.java.table.MethodCalls"",""displayName"":""Method calls"",""instanceName"":""Method calls"",""description"":""The text of matching method invocations."",""columns"":[{""name"":""sourceFile"",""type"":""String"",""displayName"":""Source file"",""description"":""The source file that the method call occurred in.""},{""name"":""method"",""type"":""String"",""displayName"":""Method call"",""description"":""The text of the method call.""},{""name"":""className"",""type"":""String"",""displayName"":""Class name"",""description"":""The class name of the method call.""},{""name"":""methodName"",""type"":""String"",""displayName"":""Method name"",""description"":""The method name of the method call.""},{""name"":""argumentTypes"",""type"":""String"",""displayName"":""Argument types"",""description"":""The argument types of the method call.""}]}]" +maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.FindThreadStartInConstructor,Find `Thread.start()` calls made during construction of a non-final class,"Finds `Thread.start()` invocations reached during construction of a non-`final` class — from a constructor body, an instance field initializer, or an instance initializer block. Starting a thread before construction completes lets the new thread observe a partially-initialised object; the problem is compounded when a subclass extends the class, because the superclass constructor starts the thread before the subclass' own fields have been initialised. Move the `start()` call to a separate method callers invoke after construction, or declare the class `final`.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.FindVirtualThreadOpportunities,Find Virtual Thread opportunities,Find opportunities to convert existing code to use Virtual Threads.,10,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,,"[{""name"":""org.openrewrite.java.table.MethodCalls"",""displayName"":""Method calls"",""instanceName"":""Method calls"",""description"":""The text of matching method invocations."",""columns"":[{""name"":""sourceFile"",""type"":""String"",""displayName"":""Source file"",""description"":""The source file that the method call occurred in.""},{""name"":""method"",""type"":""String"",""displayName"":""Method call"",""description"":""The text of the method call.""},{""name"":""className"",""type"":""String"",""displayName"":""Class name"",""description"":""The class name of the method call.""},{""name"":""methodName"",""type"":""String"",""displayName"":""Method name"",""description"":""The method name of the method call.""},{""name"":""argumentTypes"",""type"":""String"",""displayName"":""Argument types"",""description"":""The argument types of the method call.""}]}]" maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.IfElseIfConstructToSwitch,If-else-if-else to switch,"Replace if-else-if-else with switch statements. In order to be replaced with a switch, all conditions must be on the same variable and there must be at least three cases.",1,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, maven,org.openrewrite.recipe:rewrite-migrate-java,org.openrewrite.java.migrate.lang.JavaLangAPIs,Use modernized `java.lang` APIs,"Certain Java lang APIs have become deprecated and their usages changed, necessitating usage changes.",16,,`java.lang` APIs,Modernize,Java,,,Modernize your code to best use the project's current JDK version. Take advantage of newly available APIs and reduce the dependency of your code on third party dependencies where there is equivalent functionality in the Java standard library.,Basic building blocks for transforming Java code.,, diff --git a/src/test/java/org/openrewrite/java/migrate/lang/FindThreadStartInConstructorTest.java b/src/test/java/org/openrewrite/java/migrate/lang/FindThreadStartInConstructorTest.java new file mode 100644 index 0000000000..cd5c3a2692 --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/lang/FindThreadStartInConstructorTest.java @@ -0,0 +1,231 @@ +/* + * Copyright 2026 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.lang; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class FindThreadStartInConstructorTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new FindThreadStartInConstructor()); + } + + @DocumentExample + @Test + void findsStartInConstructor() { + rewriteRun( + //language=java + java( + """ + class Worker { + private final Thread thread; + + Worker(Runnable r) { + thread = new Thread(r); + thread.start(); + } + } + """, + """ + class Worker { + private final Thread thread; + + Worker(Runnable r) { + thread = new Thread(r); + /*~~(`Thread.start()` called during construction of a non-final class. The new thread can observe a partially-initialised object, and any subclass' fields are guaranteed unset. Move the call to a separate method or declare the class `final`.)~~>*/thread.start(); + } + } + """ + ) + ); + } + + @Test + void findsStartInInstanceInitializerBlock() { + rewriteRun( + //language=java + java( + """ + class Worker { + private final Thread t = new Thread(); + { + t.start(); + } + } + """, + """ + class Worker { + private final Thread t = new Thread(); + { + /*~~(`Thread.start()` called during construction of a non-final class. The new thread can observe a partially-initialised object, and any subclass' fields are guaranteed unset. Move the call to a separate method or declare the class `final`.)~~>*/t.start(); + } + } + """ + ) + ); + } + + @Test + void findsStartOnThreadSubclassInConstructor() { + rewriteRun( + //language=java + java( + """ + class MyThread extends Thread {} + + class Worker { + private final MyThread t; + + Worker() { + t = new MyThread(); + t.start(); + } + } + """, + """ + class MyThread extends Thread {} + + class Worker { + private final MyThread t; + + Worker() { + t = new MyThread(); + /*~~(`Thread.start()` called during construction of a non-final class. The new thread can observe a partially-initialised object, and any subclass' fields are guaranteed unset. Move the call to a separate method or declare the class `final`.)~~>*/t.start(); + } + } + """ + ) + ); + } + + @Test + void allowsStartInConstructorOfFinalClass() { + rewriteRun( + //language=java + java( + """ + final class Worker { + private final Thread thread; + + Worker(Runnable r) { + thread = new Thread(r); + thread.start(); + } + } + """ + ) + ); + } + + @Test + void allowsStartInRegularMethod() { + rewriteRun( + //language=java + java( + """ + class Worker { + void run(Runnable r) { + Thread t = new Thread(r); + t.start(); + } + } + """ + ) + ); + } + + @Test + void allowsStartInStaticInitializer() { + rewriteRun( + //language=java + java( + """ + class Worker { + private static final Thread background; + static { + background = new Thread(); + background.start(); + } + } + """ + ) + ); + } + + @Test + void allowsStartInRecord() { + // Records are implicitly final; the subclass concern doesn't apply. + rewriteRun( + //language=java + java( + """ + record Worker(Thread t) { + Worker { + t.start(); + } + } + """ + ) + ); + } + + @Test + void allowsStartInAnonymousClassMethod() { + // Anonymous classes can't be extended, so the S2693 subclass concern doesn't apply. + rewriteRun( + //language=java + java( + """ + class Worker { + Runnable make(Thread t) { + return new Runnable() { + @Override + public void run() { + t.start(); + } + }; + } + } + """ + ) + ); + } + + @Test + void allowsStartInLambdaBodyDeferredFromConstructor() { + // The lambda body executes later, outside the constructor scope. + rewriteRun( + //language=java + java( + """ + class Worker { + private final Runnable deferred; + + Worker(Thread t) { + deferred = () -> t.start(); + } + } + """ + ) + ); + } +}