diff --git a/recipe-writing-lessons.md b/recipe-writing-lessons.md index 8da5868b7..ce73c1e56 100644 --- a/recipe-writing-lessons.md +++ b/recipe-writing-lessons.md @@ -319,3 +319,37 @@ mark escape. Key learnings: the method type still declares the legacy type. With every reference re-typed, `maybeRemoveImport` removes the import with no downstream cleanup needed. `ReplaceSynchronizedType` is the shared base that implements this for `Hashtable`/`Vector`/`StringBuffer`. + +## Data flow / taint tracking (rewrite-analysis) + +### `Dataflow.findSinks` cannot track sources inside `catch` blocks +`org.openrewrite.analysis.dataflow.Dataflow#findSinks` gates its results on control-flow +*reachability*: it computes the control-flow graph of the enclosing method and prunes any +flow whose nodes are not reachable on the normal control-flow path. A `catch` block is only +entered via an exceptional edge, which the control-flow graph does not model, so every +expression inside a `catch` is considered unreachable and the flow is pruned to empty — +`findSinks` returns `Option.none()` even though `DataFlowNode.of(...)` and `spec.isSource(...)` +both succeed. + +To taint-track a source that lives inside a `catch` block (e.g. connecting a caught exception +to a newly thrown one), drive the flow engine directly and skip the reachability filter: +```java +DataFlowNode.of(cursor).forEach(node -> { + FlowGraph graph = ForwardFlow.findAllFlows(node, spec, FlowGraph.Factory.defaultFactory()); + // BFS/DFS over graph.getEdges(); collect each node.getCursor().getValue() that is an Expression +}); +``` +This yields the full forward taint graph (every expression the source flows into) without the +control-flow reachability gate. See `FindNewExceptionWithoutCause`. + +### Matching a reference to a specific local/caught variable +`JavaType.Variable.equals` is structural (compares `name` + `owner`), so a reference's +`J.Identifier#getFieldType()` reliably equals the declaration's `NamedVariable#getVariableType()`. +Fall back to simple-name comparison only when type attribution is missing. + +### `@Nullable` on a nested type is a type-use annotation +Write `JavaType.@Nullable Variable`, not `@Nullable JavaType.Variable`. The latter annotates the +scoping construct `JavaType` and fails to compile ("scoping construct cannot be annotated with +type-use annotation"), which crashes the whole Lombok annotation-processing round and produces a +misleading cascade of "does not override abstract method getDescription()" errors across every +Lombok-annotated recipe. diff --git a/src/main/java/org/openrewrite/staticanalysis/FindNewExceptionWithoutCause.java b/src/main/java/org/openrewrite/staticanalysis/FindNewExceptionWithoutCause.java new file mode 100644 index 000000000..de477d7d5 --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/FindNewExceptionWithoutCause.java @@ -0,0 +1,251 @@ +/* + * 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.staticanalysis;
+
+import lombok.EqualsAndHashCode;
+import lombok.Value;
+import org.jspecify.annotations.Nullable;
+import org.openrewrite.Cursor;
+import org.openrewrite.ExecutionContext;
+import org.openrewrite.Preconditions;
+import org.openrewrite.Recipe;
+import org.openrewrite.TreeVisitor;
+import org.openrewrite.analysis.dataflow.DataFlowNode;
+import org.openrewrite.analysis.dataflow.TaintFlowSpec;
+import org.openrewrite.analysis.dataflow.analysis.FlowGraph;
+import org.openrewrite.analysis.dataflow.analysis.ForwardFlow;
+import org.openrewrite.java.JavaIsoVisitor;
+import org.openrewrite.java.tree.Expression;
+import org.openrewrite.java.tree.J;
+import org.openrewrite.java.tree.JavaSourceFile;
+import org.openrewrite.java.tree.JavaType;
+import org.openrewrite.marker.SearchResult;
+import org.openrewrite.staticanalysis.groovy.GroovyFileChecker;
+import org.openrewrite.staticanalysis.java.JavaFileChecker;
+import org.openrewrite.staticanalysis.kotlin.KotlinFileChecker;
+import org.openrewrite.staticanalysis.table.ExceptionsWithoutCause;
+
+import java.util.ArrayDeque;
+import java.util.Deque;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+@Value
+@EqualsAndHashCode(callSuper = false)
+public class FindNewExceptionWithoutCause extends Recipe {
+
+ private static final String TAINTED_KEY = "caughtExceptionTaintedExpressions";
+ private static final String CAUGHT_KEY = "caughtExceptionVariable";
+
+ transient ExceptionsWithoutCause report = new ExceptionsWithoutCause(this);
+
+ @Override
+ public String getDisplayName() {
+ return "Find new exceptions thrown without the caught exception";
+ }
+
+ @Override
+ public String getDescription() {
+ return "Finds `catch` blocks that throw a newly created exception without referencing the caught exception, " +
+ "which discards the original exception's stack trace and message. Data flow (taint) tracking is used " +
+ "to establish whether the caught exception—or any value derived from it—reaches the thrown exception, " +
+ "so indirect references through local variables and string concatenation are not falsely reported. " +
+ "This mirrors PMD's `PreserveStackTrace` rule.";
+ }
+
+ @Override
+ public TreeVisitor, ExecutionContext> getVisitor() {
+ return Preconditions.check(Preconditions.or(
+ new JavaFileChecker<>(),
+ new GroovyFileChecker<>(),
+ new KotlinFileChecker<>()
+ ), new JavaIsoVisitor
+ * 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.staticanalysis.table;
+
+import com.fasterxml.jackson.annotation.JsonIgnoreType;
+import lombok.Value;
+import org.openrewrite.Column;
+import org.openrewrite.DataTable;
+import org.openrewrite.Recipe;
+
+@JsonIgnoreType
+public class ExceptionsWithoutCause extends DataTable
+ * 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.staticanalysis;
+
+import org.junit.jupiter.api.Test;
+import org.openrewrite.DocumentExample;
+import org.openrewrite.staticanalysis.table.ExceptionsWithoutCause;
+import org.openrewrite.test.RecipeSpec;
+import org.openrewrite.test.RewriteTest;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.openrewrite.java.Assertions.java;
+
+@SuppressWarnings({"ThrowablePrintStackTrace", "unused", "RedundantThrows", "CallToPrintStackTrace", "UnnecessaryLocalVariable", "CaughtExceptionImmediatelyRethrown"})
+class FindNewExceptionWithoutCauseTest implements RewriteTest {
+
+ @Override
+ public void defaults(RecipeSpec spec) {
+ spec.recipe(new FindNewExceptionWithoutCause());
+ }
+
+ @DocumentExample
+ @Test
+ void throwsNewExceptionWithoutReferencingCaught() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed");
+ }
+ }
+ }
+ """,
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ throw /*~~>*/new RuntimeException("Failed");
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void caughtExceptionUsedForLoggingButNotChained() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw new RuntimeException("Failed");
+ }
+ }
+ }
+ """,
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw /*~~>*/new RuntimeException("Failed");
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void doNotChangeWhenCaughtExceptionPassedAsCause() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed", e);
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void doNotChangeWhenCaughtMessageReferencedDirectly() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ throw new IllegalStateException(e.getMessage());
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void doNotChangeWhenCaughtFlowsThroughLocalVariable() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ String message = "Failed: " + e.getMessage();
+ throw new IllegalStateException(message);
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void doNotChangeWhenCaughtExceptionAliasedThenThrown() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ Throwable cause = e;
+ throw new RuntimeException("Failed", cause);
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void doNotChangeOnPlainRethrow() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() throws IOException {
+ try {
+ risky();
+ } catch (IOException e) {
+ throw e;
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void doNotFlagThrowInsideNestedTryBody() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ try {
+ throw new RuntimeException("inner", e);
+ } catch (RuntimeException re) {
+ throw new RuntimeException(re);
+ }
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void nestedCatchIsEvaluatedAgainstItsOwnException() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ try {
+ risky();
+ } catch (IOException inner) {
+ throw new RuntimeException("dropped");
+ }
+ }
+ }
+ }
+ """,
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ try {
+ risky();
+ } catch (IOException inner) {
+ throw /*~~>*/new RuntimeException("dropped");
+ }
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void multiCatchWithoutReferenceIsFlagged() {
+ rewriteRun(
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException | RuntimeException e) {
+ throw new IllegalStateException("boom");
+ }
+ }
+ }
+ """,
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException | RuntimeException e) {
+ throw /*~~>*/new IllegalStateException("boom");
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+
+ @Test
+ void recordsDataTableRow() {
+ rewriteRun(
+ spec -> spec.dataTable(ExceptionsWithoutCause.Row.class, rows -> {
+ assertThat(rows).hasSize(1);
+ ExceptionsWithoutCause.Row row = rows.getFirst();
+ assertThat(row.getSourcePath()).isEqualTo("A.java");
+ assertThat(row.getCaughtType()).isEqualTo("java.io.IOException");
+ assertThat(row.getThrownType()).isEqualTo("java.lang.RuntimeException");
+ }),
+ //language=java
+ java(
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed");
+ }
+ }
+ }
+ """,
+ """
+ import java.io.IOException;
+ class A {
+ void risky() throws IOException {}
+ void foo() {
+ try {
+ risky();
+ } catch (IOException e) {
+ throw /*~~>*/new RuntimeException("Failed");
+ }
+ }
+ }
+ """
+ )
+ );
+ }
+}