From 8992cf7081b0f847f03455134f56f2babbfe5f45 Mon Sep 17 00:00:00 2001 From: Sam Carlberg Date: Sat, 1 Nov 2025 20:27:08 -0400 Subject: [PATCH] [javac plugin] Add compile-time checks for unsafe or incorrect coroutine usage (#8289) CoroutineYieldInLoopDetector This checks for while loops where coroutines are in scope but without calling a blocking method on at least one of those coroutines: ``` drivetrain.run(theCoroutine -> { while (drivetrain.getDistance() < 10) { // ERROR: "Missing call to `theCoroutine.yield()` inside loop" drivetrain.setSpeed(1); } }); ``` Note that, because we assume most looping constructs in commands will use while loops, we don't check for-loops, for-each loops, or do-while loops. This check can be disabled with `@SuppressWarnings("CoroutineYieldInLoop")` CodeAfterCoroutineParkDetector Essentially acts like the Java compiler's check for code after a while (true) loop, but for coroutine.park(): ``` drivetrain.run(theCoroutine -> { drivetrain.setSpeed(1.0); theCoroutine.park(); drivetrain.setSpeed(0.0); // ERROR: "Unreachable statement: `theCoroutine.park()` will never exit" }); ``` This check can be disabled with `@SuppressWarnings("CodeAfterCoroutinePark")` IncorrectCoroutineUseDetector Checks for usage of captured (outer) coroutine parameters and assignments to fields. ``` drivetrain.run(outer -> { outer.await(arm.run(inner -> { outer.yield(); // ERROR: "Coroutine `outer` may not be in scope. Consider using `inner`" })) }); ``` This check can be disabled with `@SuppressWarnings("CoroutineMayNotBeInScope")` ``` private Coroutine coroutineField; drivetrain.run(co -> coroutineField = co); // ERROR: "Captured coroutines may not be stored in fields" ``` This check can be disabled with `@SuppressWarnings("CoroutineCapture")` --- .../org/wpilib/commands3/CoroutineTest.java | 1 + .../CodeAfterCoroutineParkDetector.java | 86 ++++ .../javacplugin/CoroutineBasedDetector.java | 56 +++ .../CoroutineYieldInLoopDetector.java | 303 ++++++++++++ .../IncorrectCoroutineUseDetector.java | 242 ++++++++++ .../org/wpilib/javacplugin/Suppressions.java | 57 +++ .../wpilib/javacplugin/WPILibJavacPlugin.java | 3 + .../CodeAfterCoroutineParkDetectorTest.java | 240 +++++++++ .../javacplugin/CompileTestOptions.java | 13 - .../wpilib/javacplugin/CompileTestUtils.java | 48 ++ .../CoroutineInLoopListenerTest.java | 456 ++++++++++++++++++ .../IncorrectCoroutineUseDetectorTest.java | 303 ++++++++++++ .../ReturnValueUsedListenerTest.java | 2 +- 13 files changed, 1796 insertions(+), 14 deletions(-) create mode 100644 javacPlugin/src/main/java/org/wpilib/javacplugin/CodeAfterCoroutineParkDetector.java create mode 100644 javacPlugin/src/main/java/org/wpilib/javacplugin/CoroutineBasedDetector.java create mode 100644 javacPlugin/src/main/java/org/wpilib/javacplugin/CoroutineYieldInLoopDetector.java create mode 100644 javacPlugin/src/main/java/org/wpilib/javacplugin/IncorrectCoroutineUseDetector.java create mode 100644 javacPlugin/src/main/java/org/wpilib/javacplugin/Suppressions.java create mode 100644 javacPlugin/src/test/java/org/wpilib/javacplugin/CodeAfterCoroutineParkDetectorTest.java delete mode 100644 javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestOptions.java create mode 100644 javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestUtils.java create mode 100644 javacPlugin/src/test/java/org/wpilib/javacplugin/CoroutineInLoopListenerTest.java create mode 100644 javacPlugin/src/test/java/org/wpilib/javacplugin/IncorrectCoroutineUseDetectorTest.java diff --git a/commandsv3/src/test/java/org/wpilib/commands3/CoroutineTest.java b/commandsv3/src/test/java/org/wpilib/commands3/CoroutineTest.java index b6779f8a9a..7b143751cf 100644 --- a/commandsv3/src/test/java/org/wpilib/commands3/CoroutineTest.java +++ b/commandsv3/src/test/java/org/wpilib/commands3/CoroutineTest.java @@ -113,6 +113,7 @@ class CoroutineTest extends CommandTestBase { } @Test + @SuppressWarnings("CoroutineMayNotBeInScope") void usingParentCoroutineInChildThrows() { var parent = Command.noRequirements() diff --git a/javacPlugin/src/main/java/org/wpilib/javacplugin/CodeAfterCoroutineParkDetector.java b/javacPlugin/src/main/java/org/wpilib/javacplugin/CodeAfterCoroutineParkDetector.java new file mode 100644 index 0000000000..c6a98f2fc0 --- /dev/null +++ b/javacPlugin/src/main/java/org/wpilib/javacplugin/CodeAfterCoroutineParkDetector.java @@ -0,0 +1,86 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.EmptyStatementTree; +import com.sun.source.tree.ExpressionStatementTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.util.JavacTask; +import com.sun.source.util.TreeScanner; +import com.sun.source.util.Trees; +import javax.lang.model.element.VariableElement; +import javax.tools.Diagnostic; + +/** + * Detects any statements after a call to {@code coroutine.park()} and labels them as unreachable + * code, similar to a {@code while (true)} statement. + */ +public class CodeAfterCoroutineParkDetector extends CoroutineBasedDetector { + public static final String SUPPRESSION_KEY = "CodeAfterCoroutinePark"; + + public CodeAfterCoroutineParkDetector(JavacTask task) { + super(task); + } + + @Override + protected TreeScanner createScanner(CompilationUnitTree compilationUnit) { + return new Scanner(compilationUnit); + } + + private final class Scanner extends TreeScanner { + private final CompilationUnitTree m_root; + private final Trees m_trees = Trees.instance(m_task); + + Scanner(CompilationUnitTree compilationUnit) { + m_root = compilationUnit; + } + + @Override + public Void visitBlock(BlockTree node, Void param) { + var path = m_trees.getPath(m_root, node); + if (Suppressions.hasSuppression(m_trees, path, SUPPRESSION_KEY)) { + // Error is suppressed for this block, don't bother checking + return super.visitBlock(node, param); + } + + MethodInvocationTree parkInvocation = null; + for (StatementTree statement : node.getStatements()) { + if (statement instanceof EmptyStatementTree) { + // skip empty statements; someone could have just added an extra semicolon by accident + continue; + } + + if (parkInvocation != null) { + m_trees.printMessage( + Diagnostic.Kind.ERROR, + "Unreachable statement: `" + parkInvocation + "` will never exit", + statement, + m_root); + break; + } + + if (statement instanceof ExpressionStatementTree est + && est.getExpression() instanceof MethodInvocationTree mit + && mit.getMethodSelect() instanceof MemberSelectTree ms + && ms.getIdentifier().contentEquals("park") + && ms.getExpression() instanceof IdentifierTree id) { + var idPath = m_trees.getPath(m_root, id); + var identifierElement = m_trees.getElement(idPath); + if (identifierElement instanceof VariableElement ve + && m_task.getTypes().isSameType(m_coroutineType, ve.asType())) { + parkInvocation = mit; + } + } + } + + return super.visitBlock(node, param); + } + } +} diff --git a/javacPlugin/src/main/java/org/wpilib/javacplugin/CoroutineBasedDetector.java b/javacPlugin/src/main/java/org/wpilib/javacplugin/CoroutineBasedDetector.java new file mode 100644 index 0000000000..8506b9a2c0 --- /dev/null +++ b/javacPlugin/src/main/java/org/wpilib/javacplugin/CoroutineBasedDetector.java @@ -0,0 +1,56 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.util.JavacTask; +import com.sun.source.util.TaskEvent; +import com.sun.source.util.TaskListener; +import com.sun.source.util.TreeScanner; +import java.util.HashSet; +import java.util.Set; +import javax.lang.model.type.TypeMirror; + +/** + * Base class for detectors that scan for code that uses coroutines. Subclasses should provide a + * {@link TreeScanner} class that implements the logic for the detector. + */ +public abstract class CoroutineBasedDetector implements TaskListener { + protected final JavacTask m_task; + private final Set m_visitedCUs = new HashSet<>(); + + /** + * The type of the Coroutine class. This is null if the commands v3 library is not on the + * classpath, or if the field is read before the Java compiler finishes the analyze phase. Custom + * scanners are only used if this is non-null; scanner code can safely assume that it is present. + */ + protected TypeMirror m_coroutineType; + + protected CoroutineBasedDetector(JavacTask task) { + m_task = task; + } + + @Override + public void finished(TaskEvent taskEvent) { + var compilationUnit = taskEvent.getCompilationUnit(); + if (taskEvent.getKind() == TaskEvent.Kind.ANALYZE && m_visitedCUs.add(compilationUnit)) { + m_coroutineType = getCoroutineType(); + + if (m_coroutineType == null) { + // Not using the commands library; nothing to scan for + return; + } + + compilationUnit.accept(createScanner(compilationUnit), null); + } + } + + protected abstract TreeScanner createScanner(CompilationUnitTree compilationUnit); + + private TypeMirror getCoroutineType() { + var te = m_task.getElements().getTypeElement("org.wpilib.commands3.Coroutine"); + return te == null ? null : te.asType(); + } +} diff --git a/javacPlugin/src/main/java/org/wpilib/javacplugin/CoroutineYieldInLoopDetector.java b/javacPlugin/src/main/java/org/wpilib/javacplugin/CoroutineYieldInLoopDetector.java new file mode 100644 index 0000000000..e510a8f4db --- /dev/null +++ b/javacPlugin/src/main/java/org/wpilib/javacplugin/CoroutineYieldInLoopDetector.java @@ -0,0 +1,303 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.WhileLoopTree; +import com.sun.source.util.JavacTask; +import com.sun.source.util.TreeScanner; +import com.sun.source.util.Trees; +import java.util.ArrayList; +import java.util.List; +import javax.lang.model.element.Element; +import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.VariableElement; +import javax.tools.Diagnostic; + +/** + * Checks for {@code while} loops inside methods or lambda functions that accept coroutine + * arguments. If a loop does not call {@code yield()} on one of the most local coroutine objects, a + * compiler error will be emitted for that loop element. This check cannot be silenced. + */ +// Note: cannot be silenced because annotations cannot be placed on loops. +// This is not legal Java: +// @SuppressWarnings("UnsafeCoroutineUsage") +// while (true) { ... } +// Placing it at a higher level (lambda or method declaration) would silence ALL unsafe usage in +// that expression; it's impossible to do on a case-by-case basis. +public class CoroutineYieldInLoopDetector extends CoroutineBasedDetector { + public static final String SUPPRESSION_KEY = "CoroutineYieldInLoop"; + + public CoroutineYieldInLoopDetector(JavacTask task) { + super(task); + } + + @Override + protected TreeScanner createScanner(CompilationUnitTree compilationUnit) { + return new Scanner(compilationUnit); + } + + /** + * Tracks the state of a while loop while traversing the AST. These are initially created when + * encountering a method or lambda function declaration, after checking that the method or lambda + * accepts at least one Coroutine argument. All Coroutine arguments on the method or lambda + * function will be added to m_availableCoroutines. If a nested lambda function is found that + * accepts coroutines, then a new state will be created for that lambda and its coroutine + * arguments will be the ones that need to be yielded on. + * + *

If a `while` loop is not encountered while further traversing the tree, then the initial + * object will not be modified and will be discarded, unused. But if a `while` loop _is_ + * encountered, then m_loop will be assigned to that loop element and further traversal will + * occur. Any calls to `yield()` on one of the coroutine arguments declared by the enclosing + * method or lambda function will be detected and added to m_yieldCalls. Any `while` loops + * encountered while m_loop is set are child loops, and will be parsed standalone and given new + * LoopState objects, which will then be added to m_children. Error reporting is only done by the + * root state object once its entire AST has been traversed, to ensure that inner loops do not + * report errors first and appearing out of order in the compiler output. + * + *

Note: this is a mutable type so that a single object may be updated as the tree traversal + * reaches points of interest (lambda definition, loop declarations, and so on) and have its state + * checked at a higher level when inner AST traversal is finished. + */ + private static final class LoopState { + /** The loop element being tracked. */ + WhileLoopTree m_loop; + + /** + * All discovered calls to Coroutine.yield(). Only applies to calls to coroutines in + * m_availableCoroutines. + */ + final List m_yieldCalls = new ArrayList<>(); + + /** + * The set of the most local coroutine arguments. eg for nested lambdas, the coroutine arguments + * to the innermost lambda will be present, but none of the coroutine arguments from any of the + * enclosing lambdas. + */ + final List m_availableCoroutines = new ArrayList<>(); + + /** + * All `while` loops nested inside m_loop. Only applies to direct children; loops in + * conditionals, switch blocks, and the like will be present, but not loops in other nested + * loops, nor loops declared inside a lambda inside a loop. + */ + final List m_children = new ArrayList<>(); + } + + private final class Scanner extends TreeScanner { + private final CompilationUnitTree m_root; + private final Trees m_trees; + + Scanner(CompilationUnitTree compilationUnit) { + m_root = compilationUnit; + m_trees = Trees.instance(m_task); + } + + @Override + public LoopState visitMethod(MethodTree node, LoopState loopState) { + var path = m_trees.getPath(m_root, node); + var el = m_trees.getElement(path); + if (!(el instanceof ExecutableElement ex)) { + return super.visitMethod(node, loopState); + } + + // Scan parameters to see if any are coroutines + List coroutineParameters = + ex.getParameters().stream() + .filter( + parameter -> m_task.getTypes().isSameType(parameter.asType(), m_coroutineType)) + .toList(); + + if (coroutineParameters.isEmpty()) { + // This method does not take coroutine arguments + return super.visitMethod(node, loopState); + } + + // Note: We only want to allow yield() calls to the MOST local coroutines possible, so nested + // methods or lambdas don't accidentally call yield on the coroutine of an outer command + // (which would be a RuntimeError, since that coroutine would not be mounted). + // Therefore, we do not copy the available coroutines from the parent state to the new + // child state. + var localState = new LoopState(); + localState.m_availableCoroutines.addAll(coroutineParameters); + if (loopState != null) { + loopState.m_children.add(localState); + } + + return super.visitMethod(node, localState); + } + + @Override + public LoopState visitLambdaExpression(LambdaExpressionTree node, LoopState loopState) { + // Scan parameters to see if any are coroutines + List coroutineParameters = + node.getParameters().stream() + .filter( + parameter -> { + var paramPath = m_trees.getPath(m_root, parameter.getType()); + var paramTypeMirror = m_trees.getTypeMirror(paramPath); + return m_task.getTypes().isSameType(paramTypeMirror, m_coroutineType); + }) + .map( + variableTree -> { + return (VariableElement) + m_trees.getElement(m_trees.getPath(m_root, variableTree)); + }) + .toList(); + + if (coroutineParameters.isEmpty()) { + // This lambda function does not take coroutine arguments + return super.visitLambdaExpression(node, loopState); + } + + // Note: we only want to allow yield() calls to the MOST local coroutines possible, so nested + // methods or lambdas don't accidentally call yield on the coroutine of an outer command + // (which would be a RuntimeError, since that coroutine would not be mounted). + // Therefore, we do not copy the available coroutines from the parent state to the new + // child state. + var localState = new LoopState(); + localState.m_availableCoroutines.addAll(coroutineParameters); + if (loopState != null) { + loopState.m_children.add(localState); + } + + return super.visitLambdaExpression(node, localState); + } + + @Override + public LoopState visitWhileLoop(WhileLoopTree node, LoopState loopState) { + if (loopState == null) { + // Not inside a coroutine-accepting method or lambda function; bail + return super.visitWhileLoop(node, null); + } + + var path = m_trees.getPath(m_root, node); + if (Suppressions.hasSuppression(m_trees, path, SUPPRESSION_KEY)) { + // Error is suppressed in this context, don't bother checking + return super.visitWhileLoop(node, loopState); + } + + if (loopState.m_loop == null) { + loopState.m_loop = node; + var result = super.visitWhileLoop(node, loopState); + printErrors(loopState); + return result; + } else { + // Nested loop; split off a new child with the same available coroutines + var localState = new LoopState(); + localState.m_loop = node; + localState.m_availableCoroutines.addAll(loopState.m_availableCoroutines); + loopState.m_children.add(localState); + + // Don't print errors now - we'll handle that when we finish the parent loop + // Otherwise, errors would be printed by the innermost loops first and appear out of order, + // which is confusing + return super.visitWhileLoop(node, localState); + } + } + + private void printErrors(LoopState state) { + if (state == null) { + return; + } + if (state.m_availableCoroutines.isEmpty() || state.m_loop == null) { + // Technically, this state should never be reachable. + // But to prevent errors in the compiler plugin from preventing user code from building, + // we silently ignore this bad state. + return; + } + + if (state.m_yieldCalls.isEmpty()) { + m_trees.printMessage( + Diagnostic.Kind.ERROR, buildErrorMessageForLoop(state), state.m_loop, m_root); + } + + // Recurse over children + for (var childLoop : state.m_children) { + printErrors(childLoop); + } + } + + private CharSequence buildErrorMessageForLoop(LoopState state) { + StringBuilder messageBuilder = new StringBuilder(64).append("Missing call to "); + var suggestedFunctionCalls = + state.m_availableCoroutines.stream() + .map(e -> "`" + e.getSimpleName().toString() + ".yield()`") + .toList(); + + // Join function calls with commas, and inserting an "or" before the final item + // No commas are used for lists of 2 items or fewer + switch (suggestedFunctionCalls.size()) { + case 1 -> messageBuilder.append(suggestedFunctionCalls.get(0)); + case 2 -> + messageBuilder + .append(suggestedFunctionCalls.get(0)) + .append(" or ") + .append(suggestedFunctionCalls.get(1)); + default -> { + for (int i = 0; i < suggestedFunctionCalls.size(); i++) { + messageBuilder.append(suggestedFunctionCalls.get(i)); + if (i < suggestedFunctionCalls.size() - 2) { + // early items + messageBuilder.append(", "); + } else if (i == suggestedFunctionCalls.size() - 2) { + // penultimate item + messageBuilder.append(", or "); + } else { + // last item - append nothing + } + } + } + } + + messageBuilder.append(" inside loop"); + + return messageBuilder; + } + + @Override + public LoopState visitMethodInvocation(MethodInvocationTree node, LoopState loopState) { + if (loopState == null) { + // not inside a coroutine-accepting method or lambda function; bail + return super.visitMethodInvocation(node, null); + } + + var sel = node.getMethodSelect(); + if (sel instanceof MemberSelectTree ms && isPermittedCoroutineFunction(ms)) { + // calling a method on something (may not be a coroutine object) with the same name as a + // permitted coroutine function name + var recv = ms.getExpression(); + if (recv instanceof IdentifierTree id) { + var idPath = m_trees.getPath(m_root, id); + var el = m_trees.getElement(idPath); + if (el instanceof VariableElement ve && loopState.m_availableCoroutines.contains(ve)) { + // definitely calling yield on a local (method or lambda argument) coroutine object + loopState.m_yieldCalls.add(el); + } + } + } + + return super.visitMethodInvocation(node, loopState); + } + } + + private boolean isPermittedCoroutineFunction(MemberSelectTree tree) { + // Loops MUST either call yield() or a blocking coroutine function like await() or waitFor() + // that call yield() internally. Nonblocking coroutine functions like fork() aren't enough. + var identifier = tree.getIdentifier(); + return identifier.contentEquals("yield") + || identifier.contentEquals("park") + || identifier.contentEquals("wait") + || identifier.contentEquals("waitUntil") + || identifier.contentEquals("await") + || identifier.contentEquals("awaitAll") + || identifier.contentEquals("awaitAny"); + } +} diff --git a/javacPlugin/src/main/java/org/wpilib/javacplugin/IncorrectCoroutineUseDetector.java b/javacPlugin/src/main/java/org/wpilib/javacplugin/IncorrectCoroutineUseDetector.java new file mode 100644 index 0000000000..fb786ad456 --- /dev/null +++ b/javacPlugin/src/main/java/org/wpilib/javacplugin/IncorrectCoroutineUseDetector.java @@ -0,0 +1,242 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.util.JavacTask; +import com.sun.source.util.TreeScanner; +import com.sun.source.util.Trees; +import java.util.ArrayList; +import java.util.List; +import javax.lang.model.element.TypeElement; +import javax.lang.model.element.VariableElement; +import javax.tools.Diagnostic; + +/** + * Detects usage of an incorrect coroutine object. For example, nesting commands with multiple + * coroutines in scope need to use the most local coroutine object and not reference external ones. + * + * 

{@code
+ * mech.run(outerCoroutine -> {
+ *   mech.run(innerCoroutine -> {
+ *     outerCoroutine.yield(); // ERROR
+ *   })
+ * })
+ * }
+ * + *

Additional checks for escaping coroutines are also performed: + * + * 

{@code
+ * private Coroutine coroutineField;
+ *
+ * mech.run(coroutine -> coroutineField = coroutine); // ERROR
+ * }
+ */ +public class IncorrectCoroutineUseDetector extends CoroutineBasedDetector { + public static final String CAPTURE_SUPPRESSION_KEY = "CoroutineCapture"; + public static final String SCOPE_SUPPRESSION_KEY = "CoroutineMayNotBeInScope"; + + public IncorrectCoroutineUseDetector(JavacTask task) { + super(task); + } + + @Override + protected TreeScanner createScanner(CompilationUnitTree compilationUnit) { + return new Scanner(compilationUnit); + } + + private static final class State { + /** Tracks captured coroutine parameters from external lambda expressions. */ + private final List m_nonlocalCoroutineParameters; + + /** + * Tracks coroutine parameters from the most local lambda expression. Captures from outer lambda + * expressions will be in {@link #m_nonlocalCoroutineParameters}. + */ + private final List m_localCoroutineParameters; + + private State( + List nonlocalCoroutineParameters, + List coroutineParameters) { + m_nonlocalCoroutineParameters = List.copyOf(nonlocalCoroutineParameters); + m_localCoroutineParameters = List.copyOf(coroutineParameters); + } + + boolean isLocalCoroutine(VariableElement el) { + return m_localCoroutineParameters.contains(el); + } + + boolean isCapturedCoroutine(VariableElement el) { + return m_nonlocalCoroutineParameters.contains(el) || m_localCoroutineParameters.contains(el); + } + } + + private final class Scanner extends TreeScanner { + private final CompilationUnitTree m_root; + private final Trees m_trees; + + Scanner(CompilationUnitTree compilationUnit) { + m_root = compilationUnit; + m_trees = Trees.instance(m_task); + } + + @Override + public State visitLambdaExpression(LambdaExpressionTree node, State state) { + // Scan parameters to see if any are coroutines + List coroutineParameters = + node.getParameters().stream() + .filter( + parameter -> { + var paramPath = m_trees.getPath(m_root, parameter.getType()); + var paramTypeMirror = m_trees.getTypeMirror(paramPath); + return m_task.getTypes().isSameType(paramTypeMirror, m_coroutineType); + }) + .map( + variableTree -> { + return (VariableElement) + m_trees.getElement(m_trees.getPath(m_root, variableTree)); + }) + .toList(); + + if (coroutineParameters.isEmpty()) { + // This lambda function does not take coroutine arguments + return super.visitLambdaExpression(node, state); + } + + State localState; + if (state == null) { + localState = new State(List.of(), coroutineParameters); + } else { + localState = + new State( + concat(state.m_nonlocalCoroutineParameters, state.m_localCoroutineParameters), + coroutineParameters); + } + + return super.visitLambdaExpression(node, localState); + } + + @Override + public State visitMethodInvocation(MethodInvocationTree node, State state) { + if (state == null || state.m_localCoroutineParameters.isEmpty()) { + // No coroutines in scope, do nothing + return super.visitMethodInvocation(node, state); + } + + var path = m_trees.getPath(m_root, node); + if (Suppressions.hasSuppression(m_trees, path, SCOPE_SUPPRESSION_KEY)) { + // Suppressed this warning, bail + return super.visitMethodInvocation(node, state); + } + + // Check for calling a method on a nonlocal coroutine (eg `outerCoroutine.yield()`) + var sel = node.getMethodSelect(); + if (sel instanceof MemberSelectTree ms && ms.getExpression() instanceof IdentifierTree id) { + var idPath = m_trees.getPath(m_root, id); + var el = m_trees.getElement(idPath); + if (el instanceof VariableElement ve + && ve.asType().equals(m_coroutineType) + && !state.isLocalCoroutine(ve)) { + m_trees.printMessage( + Diagnostic.Kind.ERROR, nonlocalCoroutineUsageMessage(ve, state), node, m_root); + } + } + + // Check for passing a nonlocal coroutine to a method (eg `foo(outerCoroutine)`) + for (var arg : node.getArguments()) { + var argPath = m_trees.getPath(m_root, arg); + var el = m_trees.getElement(argPath); + if (el instanceof VariableElement ve + && ve.asType().equals(m_coroutineType) + && !state.isLocalCoroutine(ve)) { + m_trees.printMessage( + Diagnostic.Kind.ERROR, nonlocalCoroutineUsageMessage(ve, state), node, m_root); + } + } + + return super.visitMethodInvocation(node, state); + } + + private String nonlocalCoroutineUsageMessage(VariableElement ve, State state) { + StringBuilder optionsBuilder = new StringBuilder(); + if (state.m_localCoroutineParameters.size() == 1) { + optionsBuilder + .append('`') + .append(state.m_localCoroutineParameters.get(0).getSimpleName()) + .append('`'); + } else if (state.m_localCoroutineParameters.size() == 2) { + optionsBuilder + .append('`') + .append(state.m_localCoroutineParameters.get(0).getSimpleName()) + .append("` or `") + .append(state.m_localCoroutineParameters.get(1).getSimpleName()) + .append('`'); + } else { + for (int i = 0; i < state.m_localCoroutineParameters.size(); i++) { + var coroutineParameter = state.m_localCoroutineParameters.get(i); + optionsBuilder.append('`').append(coroutineParameter.getSimpleName()).append('`'); + if (i < state.m_localCoroutineParameters.size() - 2) { + optionsBuilder.append(", "); + } else if (i == state.m_localCoroutineParameters.size() - 2) { + optionsBuilder.append(", or "); + } + } + } + + return "Coroutine `%s` may not be in scope. Consider using %s" + .formatted(ve.getSimpleName(), optionsBuilder); + } + + @Override + public State visitAssignment(AssignmentTree node, State state) { + if (state == null || state.m_localCoroutineParameters.isEmpty()) { + return super.visitAssignment(node, state); + } + + // Check if the assignment is to a field, and if the expression is a local or nonlocal + // coroutine. + var variable = node.getVariable(); + var variablePath = m_trees.getPath(m_root, variable); + if (Suppressions.hasSuppression(m_trees, variablePath, CAPTURE_SUPPRESSION_KEY)) { + // User suppressed this error, skip + return super.visitAssignment(node, state); + } + + var variableElement = m_trees.getElement(variablePath); + if (variableElement instanceof VariableElement ve + && ve.getEnclosingElement() instanceof TypeElement + && ve.asType().equals(m_coroutineType)) { + // Assigning to a coroutine-typed field. Check the expression to see if we're assigning + // a captured coroutine. + var expression = node.getExpression(); + var expressionPath = m_trees.getPath(m_root, expression); + var expressionElement = m_trees.getElement(expressionPath); + if (expressionElement instanceof VariableElement ve2 && state.isCapturedCoroutine(ve2)) { + m_trees.printMessage( + Diagnostic.Kind.ERROR, + "Captured coroutines may not be stored in fields", + node, + m_root); + } + } + + return super.visitAssignment(node, state); + } + + // Concatenates two lists and returns the result. + // Why is this not in the standard library? + private static List concat(List a, List b) { + var result = new ArrayList(a.size() + b.size()); + result.addAll(a); + result.addAll(b); + return result; + } + } +} diff --git a/javacPlugin/src/main/java/org/wpilib/javacplugin/Suppressions.java b/javacPlugin/src/main/java/org/wpilib/javacplugin/Suppressions.java new file mode 100644 index 0000000000..7d7007dc56 --- /dev/null +++ b/javacPlugin/src/main/java/org/wpilib/javacplugin/Suppressions.java @@ -0,0 +1,57 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import com.sun.source.util.TreePath; +import com.sun.source.util.Trees; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +/** Utility class for checking for compiler error and warning suppressions. */ +public final class Suppressions { + private Suppressions() { + throw new UnsupportedOperationException("This is a utility class!"); + } + + /** + * Checks if the given tree has a suppression annotation for at least one of the given keys. The + * "all" suppression key is automatically included. + * + * @param trees The tree utils to use to loop up elements. + * @param tree The tree to the node to check for suppressions. + * @param suppressionKeys The suppression keys to check for. + * @return True if the tree has a suppression annotation for at least one of the given keys or + * "all". False otherwise + */ + public static boolean hasSuppression(Trees trees, TreePath tree, String... suppressionKeys) { + TreePath currentPath = tree; + Set suppressionKeysSet = new HashSet<>(List.of(suppressionKeys)); + suppressionKeysSet.add("all"); + + while (currentPath != null) { + var element = trees.getElement(currentPath); + currentPath = currentPath.getParentPath(); + + if (element == null) { + continue; + } + + var suppression = element.getAnnotation(SuppressWarnings.class); + if (suppression == null) { + continue; + } + + for (String key : suppression.value()) { + if (suppressionKeysSet.contains(key)) { + return true; + } + } + } + + // No suppression annotations found on the element or any parent element. + return false; + } +} diff --git a/javacPlugin/src/main/java/org/wpilib/javacplugin/WPILibJavacPlugin.java b/javacPlugin/src/main/java/org/wpilib/javacplugin/WPILibJavacPlugin.java index 87bc8f43f4..d7ca12d2c7 100644 --- a/javacPlugin/src/main/java/org/wpilib/javacplugin/WPILibJavacPlugin.java +++ b/javacPlugin/src/main/java/org/wpilib/javacplugin/WPILibJavacPlugin.java @@ -20,6 +20,9 @@ public class WPILibJavacPlugin implements Plugin { @Override public void init(JavacTask task, String... args) { task.addTaskListener(new ReturnValueUsedListener(task)); + task.addTaskListener(new CoroutineYieldInLoopDetector(task)); + task.addTaskListener(new CodeAfterCoroutineParkDetector(task)); + task.addTaskListener(new IncorrectCoroutineUseDetector(task)); } @Override diff --git a/javacPlugin/src/test/java/org/wpilib/javacplugin/CodeAfterCoroutineParkDetectorTest.java b/javacPlugin/src/test/java/org/wpilib/javacplugin/CodeAfterCoroutineParkDetectorTest.java new file mode 100644 index 0000000000..9fbba05c6a --- /dev/null +++ b/javacPlugin/src/test/java/org/wpilib/javacplugin/CodeAfterCoroutineParkDetectorTest.java @@ -0,0 +1,240 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import static com.google.testing.compile.CompilationSubject.assertThat; +import static com.google.testing.compile.Compiler.javac; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.wpilib.javacplugin.CompileTestUtils.getErrorSource; +import static org.wpilib.javacplugin.CompileTestUtils.kJavaVersionOptions; + +import com.google.testing.compile.Compilation; +import com.google.testing.compile.JavaFileObjects; +import org.junit.jupiter.api.Test; + +class CodeAfterCoroutineParkDetectorTest { + private static final String kCoroutineSource = + """ + package org.wpilib.commands3; + + public interface Coroutine { + void park(); + } + """; + + @Test + void soloPark() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + coroutine.park(); + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void printAfterPark() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + coroutine.park(); + System.out.println("Unreachable 1"); // this line should get an error + System.out.println("Unreachable 2"); // but not this one + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Unreachable statement: `coroutine.park()` will never exit", error.getMessage(null)); + assertEquals("System.out.println(\"Unreachable 1\"); ", getErrorSource(error)); + } + + @Test + void printAfterParkInBlock() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + { + // Only .park() calls at the top level are detected + coroutine.park(); + } + System.out.println("Unreachable 1"); + System.out.println("Unreachable 2"); + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void loopAfterPark() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + coroutine.park(); + for (;;) { + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Unreachable statement: `coroutine.park()` will never exit", error.getMessage(null)); + assertEquals("for (;;) {\n }\n", getErrorSource(error)); + } + + @Test + void blockAfterPark() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + coroutine.park(); + { + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Unreachable statement: `coroutine.park()` will never exit", error.getMessage(null)); + assertEquals("{\n }\n", getErrorSource(error)); + } + + @Test + void emptyStatementAfterPark() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + coroutine.park();; + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void printAfterEmptyStatementAfterPark() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + coroutine.park();; + System.out.println("Unreachable"); + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Unreachable statement: `coroutine.park()` will never exit", error.getMessage(null)); + assertEquals("System.out.println(\"Unreachable\");\n", getErrorSource(error)); + } +} diff --git a/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestOptions.java b/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestOptions.java deleted file mode 100644 index a5356b4c81..0000000000 --- a/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestOptions.java +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright (c) FIRST and other WPILib contributors. -// Open Source Software; you can modify and/or share it under the terms of -// the WPILib BSD license file in the root directory of this project. - -package org.wpilib.javacplugin; - -import java.util.List; - -public class CompileTestOptions { - public static final int kJavaVersion = 17; - public static final List kJavaVersionOptions = - List.of("-source", kJavaVersion, "-target", kJavaVersion); -} diff --git a/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestUtils.java b/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestUtils.java new file mode 100644 index 0000000000..711ec8bebe --- /dev/null +++ b/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestUtils.java @@ -0,0 +1,48 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import java.io.IOException; +import java.util.List; +import javax.tools.Diagnostic; +import javax.tools.JavaFileObject; + +public final class CompileTestUtils { + public static final int kJavaVersion = 17; + public static final List kJavaVersionOptions = + List.of("-source", kJavaVersion, "-target", kJavaVersion); + + private CompileTestUtils() { + // Utility class + } + + /** + * Gets the source code for a given compiler error. + * + * @param diagnostic The diagnostic to get the source for. + * @return The source code for the given diagnostic. + */ + public static String getErrorSource(Diagnostic diagnostic) { + try (var reader = diagnostic.getSource().openReader(true)) { + int sourceLength = (int) (diagnostic.getEndPosition() - diagnostic.getStartPosition() + 1); + char[] buf = new char[sourceLength]; + long skipCnt = reader.skip(diagnostic.getStartPosition()); + if (skipCnt != diagnostic.getStartPosition()) { + // Didn't skip to the expected position; bail + return ""; + } + + int readCnt = reader.read(buf); + if (readCnt != sourceLength) { + // Didn't read the expected length of text; bail + return ""; + } + + return new String(buf); + } catch (IOException e) { + return ""; + } + } +} diff --git a/javacPlugin/src/test/java/org/wpilib/javacplugin/CoroutineInLoopListenerTest.java b/javacPlugin/src/test/java/org/wpilib/javacplugin/CoroutineInLoopListenerTest.java new file mode 100644 index 0000000000..f01df7f910 --- /dev/null +++ b/javacPlugin/src/test/java/org/wpilib/javacplugin/CoroutineInLoopListenerTest.java @@ -0,0 +1,456 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import static com.google.testing.compile.CompilationSubject.assertThat; +import static com.google.testing.compile.Compiler.javac; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.wpilib.javacplugin.CompileTestUtils.kJavaVersionOptions; + +import com.google.testing.compile.Compilation; +import com.google.testing.compile.JavaFileObjects; +import org.junit.jupiter.api.Test; + +class CoroutineInLoopListenerTest { + private static final String kCoroutineSource = + """ + package org.wpilib.commands3; + + public interface Coroutine { + void yield(); + } + """; + + @Test + void noYieldInLoopWithoutCoroutines() { + String source = + """ + package frc.robot; + + class Example { + Runnable lambda = () -> { + while (true) { + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void basicYieldInLoopInLambda() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + while (true) { + coroutine.yield(); + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void basicYieldInLoopInMethod() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + void useCoroutine(Coroutine coroutine) { + while (true) { + coroutine.yield(); + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void noYieldInLoopInLambda() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + while (true) { + // No yield + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Missing call to `coroutine.yield()` inside loop", error.getMessage(null)); + } + + @Test + void yieldInLoopInRunnableInLambda() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + Runnable r = () -> { + while (true) { + coroutine.yield(); + } + }; + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + // TODO: Should we make it an error to yield (or invoke any methods on) a captured coroutine? + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void noYieldInMethodWithManyCoroutineParams() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + void manyCoroutineParams( + Coroutine firstCoroutine, + Coroutine c1, + Coroutine next, + Coroutine thisMightBeTheLastOne) { + while (true) { + // No yield + } + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Missing call to " + + "`firstCoroutine.yield()`, " + + "`c1.yield()`, " + + "`next.yield()`, or " + + "`thisMightBeTheLastOne.yield()` inside loop", + error.getMessage(null)); + } + + @Test + void noYieldsInNestedLambda() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + Consumer lambda2 = innerCoroutine -> { + while (true) { + // No yield + } + }; + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Missing call to `innerCoroutine.yield()` inside loop", error.getMessage(null)); + } + + @Test + void nestedLambdaYieldsToOuter() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = outerCoroutine -> { + Consumer lambda2 = innerCoroutine -> { + while (true) { + outerCoroutine.yield(); + } + }; + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + + // One error for not calling a yielding function on the most local coroutine object, + // and another error for calling a method on a captured coroutine (from a different analyzer) + assertEquals(2, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Missing call to `innerCoroutine.yield()` inside loop", error.getMessage(null)); + } + + @Test + void noYieldsInNestedLoops() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + while (0 == 0) { + while (1 == 1) { + // inner - no yield + } + // outer - also no yield + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(2, compilation.errors().size()); + + var error1 = compilation.errors().get(0); + assertEquals("Missing call to `coroutine.yield()` inside loop", error1.getMessage(null)); + assertEquals(8, error1.getLineNumber()); + + var error2 = compilation.errors().get(1); + assertEquals("Missing call to `coroutine.yield()` inside loop", error2.getMessage(null)); + assertEquals(9, error2.getLineNumber()); + } + + @Test + void noYieldInOuterLoopButYieldInInnerLoop() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + while (true) { + coroutine.yield(); + while (true) { + // No yields + } + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Missing call to `coroutine.yield()` inside loop", error.getMessage(null)); + assertEquals(10, error.getLineNumber()); + } + + @Test + void noYieldInInnerLoopButYieldInOuterLoop() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + while (true) { + while (true) { + coroutine.yield(); + } + // No yields + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Missing call to `coroutine.yield()` inside loop", error.getMessage(null)); + assertEquals(8, error.getLineNumber()); + } + + @Test + void noYieldsInDeeplyNestedLoops() { + String source = + """ + package frc.robot; + + import java.util.function.Consumer; + import org.wpilib.commands3.Coroutine; + + class Example { + Consumer lambda = coroutine -> { + while (0 == 0) { + while (1 == 1) { + while (2 == 2) { + while (3 == 3) { + while (4 == 4) { + while (5 == 5) { + if (true) { + // nested if statement shouldn't mess with error detection or reporting + while (6 == 6) { + } + } + } + } + } + } + } + } + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(7, compilation.errors().size()); + + var error1 = compilation.errors().get(0); + assertEquals("Missing call to `coroutine.yield()` inside loop", error1.getMessage(null)); + assertEquals(8, error1.getLineNumber()); + + var error2 = compilation.errors().get(1); + assertEquals("Missing call to `coroutine.yield()` inside loop", error2.getMessage(null)); + assertEquals(9, error2.getLineNumber()); + + var error3 = compilation.errors().get(2); + assertEquals("Missing call to `coroutine.yield()` inside loop", error3.getMessage(null)); + assertEquals(10, error3.getLineNumber()); + + var error4 = compilation.errors().get(3); + assertEquals("Missing call to `coroutine.yield()` inside loop", error4.getMessage(null)); + assertEquals(11, error4.getLineNumber()); + + var error5 = compilation.errors().get(4); + assertEquals("Missing call to `coroutine.yield()` inside loop", error5.getMessage(null)); + assertEquals(12, error5.getLineNumber()); + + var error6 = compilation.errors().get(5); + assertEquals("Missing call to `coroutine.yield()` inside loop", error6.getMessage(null)); + assertEquals(13, error6.getLineNumber()); + + var error7 = compilation.errors().get(6); + assertEquals("Missing call to `coroutine.yield()` inside loop", error7.getMessage(null)); + assertEquals(16, error7.getLineNumber()); + } +} diff --git a/javacPlugin/src/test/java/org/wpilib/javacplugin/IncorrectCoroutineUseDetectorTest.java b/javacPlugin/src/test/java/org/wpilib/javacplugin/IncorrectCoroutineUseDetectorTest.java new file mode 100644 index 0000000000..56ee704f87 --- /dev/null +++ b/javacPlugin/src/test/java/org/wpilib/javacplugin/IncorrectCoroutineUseDetectorTest.java @@ -0,0 +1,303 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.javacplugin; + +import static com.google.testing.compile.CompilationSubject.assertThat; +import static com.google.testing.compile.Compiler.javac; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.wpilib.javacplugin.CompileTestUtils.getErrorSource; +import static org.wpilib.javacplugin.CompileTestUtils.kJavaVersionOptions; + +import com.google.testing.compile.Compilation; +import com.google.testing.compile.JavaFileObjects; +import org.junit.jupiter.api.Test; + +class IncorrectCoroutineUseDetectorTest { + private static final String kCoroutineSource = + """ + package org.wpilib.commands3; + + public interface Coroutine { + void yield(); + } + """; + + @Test + void methodCalledOnNonlocalCoroutine() { + String source = + """ + package frc.robot; + + import org.wpilib.commands3.Coroutine; + import java.util.function.Consumer; + + class Example { + Consumer lambda = outerCoroutine -> { + Consumer lambda2 = innerCoroutine -> { + outerCoroutine.yield(); + }; + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Coroutine `outerCoroutine` may not be in scope. Consider using `innerCoroutine`", + error.getMessage(null)); + } + + @Test + void nonlocalCoroutinePassedToMethod() { + String source = + """ + package frc.robot; + + import org.wpilib.commands3.Coroutine; + import java.util.function.Consumer; + + class Example { + Consumer lambda = outerCoroutine -> { + Consumer lambda2 = innerCoroutine -> { + method(outerCoroutine); + }; + }; + + void method(Coroutine foo) { } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Coroutine `outerCoroutine` may not be in scope. Consider using `innerCoroutine`", + error.getMessage(null)); + } + + @Test + void twoLocalCoroutines() { + String source = + """ + package frc.robot; + + import org.wpilib.commands3.Coroutine; + import java.util.function.BiConsumer; + import java.util.function.Consumer; + + class Example { + Consumer lambda = outerCoroutine -> { + BiConsumer lambda2 = (a, b) -> { + method(outerCoroutine); + }; + }; + + void method(Coroutine foo) { } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Coroutine `outerCoroutine` may not be in scope. Consider using `a` or `b`", + error.getMessage(null)); + } + + @Test + void threeLocalCoroutines() { + String source = + """ + package frc.robot; + + import org.wpilib.commands3.Coroutine; + import java.util.function.Consumer; + + @FunctionalInterface + interface TriConsumer { + void accept(T t, U u, V v); + } + + class Example { + Consumer lambda = outerCoroutine -> { + TriConsumer lambda2 = (a, b, c) -> { + method(outerCoroutine); + }; + }; + + void method(Coroutine foo) { } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Coroutine `outerCoroutine` may not be in scope. Consider using `a`, `b`, or `c`", + error.getMessage(null)); + } + + @Test + void coroutineSavedToFieldErrors() { + String source = + """ + package frc.robot; + + import org.wpilib.commands3.Coroutine; + import java.util.function.Consumer; + + class Example { + Coroutine coroutineField; + + Consumer lambda = coroutine -> { + coroutineField = coroutine; + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Captured coroutines may not be stored in fields", error.getMessage(null)); + } + + @Test + void outerCoroutineSavedToFieldErrors() { + String source = + """ + package frc.robot; + + import org.wpilib.commands3.Coroutine; + import java.util.function.Consumer; + + class Example { + Coroutine coroutineField; + + Consumer lambda = outerCoroutine -> { + Consumer lambda2 = innerCoroutine -> { + coroutineField = outerCoroutine; + coroutineField = innerCoroutine; + }; + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(2, compilation.errors().size()); + + var error1 = compilation.errors().get(0); + assertEquals("Captured coroutines may not be stored in fields", error1.getMessage(null)); + assertEquals(11, error1.getLineNumber()); + assertEquals("coroutineField = outerCoroutine;", getErrorSource(error1)); + + var error2 = compilation.errors().get(1); + assertEquals("Captured coroutines may not be stored in fields", error2.getMessage(null)); + assertEquals("coroutineField = innerCoroutine;", getErrorSource(error2)); + assertEquals(12, error2.getLineNumber()); + } + + @Test + void coroutineSavedToVariableIsAllowed() { + String source = + """ + package frc.robot; + + import org.wpilib.commands3.Coroutine; + import java.util.function.Consumer; + + class Example { + Consumer lambda = coroutine -> { + Coroutine local = coroutine; + }; + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + // This test is really to make it clear that the compiler plugin can't feasibly check for + // everything, rather than this being a usecase we actually want to support. + @Test + void coroutineSavedToFieldViaMethodCannotBeDetected() { + String source = + """ + package frc.robot; + + import org.wpilib.commands3.Coroutine; + import java.util.function.Consumer; + + class Example { + private Coroutine coroutineField; + + Consumer lambda = coroutine -> { + saveCoroutine(coroutine); + }; + + void saveCoroutine(Coroutine coroutine) { + coroutineField = coroutine; + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile( + JavaFileObjects.forSourceString("org.wpilib.commands3.Coroutine", kCoroutineSource), + JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } +} diff --git a/javacPlugin/src/test/java/org/wpilib/javacplugin/ReturnValueUsedListenerTest.java b/javacPlugin/src/test/java/org/wpilib/javacplugin/ReturnValueUsedListenerTest.java index 65a21ce2f0..b03aa90217 100644 --- a/javacPlugin/src/test/java/org/wpilib/javacplugin/ReturnValueUsedListenerTest.java +++ b/javacPlugin/src/test/java/org/wpilib/javacplugin/ReturnValueUsedListenerTest.java @@ -7,7 +7,7 @@ package org.wpilib.javacplugin; import static com.google.testing.compile.CompilationSubject.assertThat; import static com.google.testing.compile.Compiler.javac; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.wpilib.javacplugin.CompileTestOptions.kJavaVersionOptions; +import static org.wpilib.javacplugin.CompileTestUtils.kJavaVersionOptions; import com.google.testing.compile.Compilation; import com.google.testing.compile.JavaFileObjects;