From 3972b01c5195f98765cb43d55c05822400a77486 Mon Sep 17 00:00:00 2001 From: Sam Carlberg Date: Fri, 3 Oct 2025 20:42:47 -0400 Subject: [PATCH] Add javac plugin for detecting common error cases at compile time (#8196) Adds a `@NoDiscard` annotation that can be placed on methods to guarantee their return values are used and on types to guarantee that any method returning that type uses the return value. Methods that call `@NoDiscard`-annotated functions can add a `@SuppressWarnings("NoDiscard")` or `@SuppressWarnings("all")` annotation (or annotation on the class declaring that method) to silence the compiler error warnings. --- CMakeLists.txt | 5 + docs/build.gradle | 2 + javacPlugin/BUILD.bazel | 11 + javacPlugin/build.gradle | 17 + .../javacplugin/ReturnValueUsedListener.java | 224 ++++++ .../wpilib/javacplugin/WPILibJavacPlugin.java | 31 + .../services/com.sun.source.util.Plugin | 1 + .../javacplugin/CompileTestOptions.java | 13 + .../ReturnValueUsedListenerTest.java | 638 ++++++++++++++++++ settings.gradle | 2 + wpiannotations/BUILD.bazel | 8 + wpiannotations/CMakeLists.txt | 37 + wpiannotations/build.gradle | 11 + .../java/org/wpilib/annotation/NoDiscard.java | 27 + wpiannotations/wpiannotations-config.cmake | 2 + wpilibNewCommands/BUILD.bazel | 5 + wpilibNewCommands/CMakeLists.txt | 1 + wpilibNewCommands/build.gradle | 2 + .../wpi/first/wpilibj2/command/Command.java | 2 + 19 files changed, 1039 insertions(+) create mode 100644 javacPlugin/BUILD.bazel create mode 100644 javacPlugin/build.gradle create mode 100644 javacPlugin/src/main/java/org/wpilib/javacplugin/ReturnValueUsedListener.java create mode 100644 javacPlugin/src/main/java/org/wpilib/javacplugin/WPILibJavacPlugin.java create mode 100644 javacPlugin/src/main/resources/META-INF/services/com.sun.source.util.Plugin create mode 100644 javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestOptions.java create mode 100644 javacPlugin/src/test/java/org/wpilib/javacplugin/ReturnValueUsedListenerTest.java create mode 100644 wpiannotations/BUILD.bazel create mode 100644 wpiannotations/CMakeLists.txt create mode 100644 wpiannotations/build.gradle create mode 100644 wpiannotations/src/main/java/org/wpilib/annotation/NoDiscard.java create mode 100644 wpiannotations/wpiannotations-config.cmake diff --git a/CMakeLists.txt b/CMakeLists.txt index 794885abe8..e7592ac89c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -289,6 +289,7 @@ endif() set(FILENAME_DEP_REPLACE "get_filename_component(SELF_DIR \"$\{CMAKE_CURRENT_LIST_FILE\}\" PATH)") set(SELF_DIR "$\{SELF_DIR\}") set(WPIUNITS_DEP_REPLACE_IMPL "find_dependency(wpiunits)") +set(WPIANNOTATIONS_DEP_REPLACE_IMPL "find_dependency(wpiannotations)") set(WPIUTIL_DEP_REPLACE "find_dependency(wpiutil)") add_subdirectory(wpiutil) @@ -308,6 +309,10 @@ if(WITH_WPIMATH) add_subdirectory(wpimath) endif() +if(WITH_JAVA) + add_subdirectory(wpiannotations) +endif() + if(WITH_WPIUNITS AND NOT WITH_WPIMATH) # In case of building wpiunits standalone set(WPIUNITS_DEP_REPLACE ${WPIUNITS_DEP_REPLACE_IMPL}) diff --git a/docs/build.gradle b/docs/build.gradle index a500a38961..423dc96cb2 100644 --- a/docs/build.gradle +++ b/docs/build.gradle @@ -9,6 +9,7 @@ evaluationDependsOn(':cscore') evaluationDependsOn(':epilogue-runtime') evaluationDependsOn(':hal') evaluationDependsOn(':ntcore') +evaluationDependsOn(':wpiannotations') evaluationDependsOn(':wpilibNewCommands') evaluationDependsOn(':wpilibc') evaluationDependsOn(':wpilibj') @@ -174,6 +175,7 @@ task generateJavaDocs(type: Javadoc) { source project(':epilogue-runtime').sourceSets.main.java source project(':hal').sourceSets.main.java source project(':ntcore').sourceSets.main.java + source project(':wpiannotations').sourceSets.main.java source project(':wpilibNewCommands').sourceSets.main.java source project(':wpilibj').sourceSets.main.java source project(':wpimath').sourceSets.main.java diff --git a/javacPlugin/BUILD.bazel b/javacPlugin/BUILD.bazel new file mode 100644 index 0000000000..2c3b786338 --- /dev/null +++ b/javacPlugin/BUILD.bazel @@ -0,0 +1,11 @@ +load("@rules_java//java:defs.bzl", "java_plugin") + +java_plugin( + name = "plugin", + srcs = glob(["src/main/java/**/*.java"]), + resources = glob(["src/main/resources/**"]), + visibility = ["//visibility:public"], + deps = [ + "//wpiannotations", + ], +) diff --git a/javacPlugin/build.gradle b/javacPlugin/build.gradle new file mode 100644 index 0000000000..89adb15320 --- /dev/null +++ b/javacPlugin/build.gradle @@ -0,0 +1,17 @@ +ext { + useJava = true + useCpp = false + baseId = 'wpilibj-javac-plugin' + groupId = 'org.wpilib' + + nativeName = '' + devMain = '' +} + +apply from: "${rootDir}/shared/java/javacommon.gradle" + +dependencies { + implementation project(':wpiannotations') + testImplementation 'com.google.testing.compile:compile-testing:+' + testImplementation project(':wpilibNewCommands') +} diff --git a/javacPlugin/src/main/java/org/wpilib/javacplugin/ReturnValueUsedListener.java b/javacPlugin/src/main/java/org/wpilib/javacplugin/ReturnValueUsedListener.java new file mode 100644 index 0000000000..4481e933fa --- /dev/null +++ b/javacPlugin/src/main/java/org/wpilib/javacplugin/ReturnValueUsedListener.java @@ -0,0 +1,224 @@ +// 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.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +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 com.sun.source.util.Trees; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.TypeElement; +import javax.lang.model.type.DeclaredType; +import javax.lang.model.type.TypeKind; +import javax.lang.model.type.TypeMirror; +import javax.tools.Diagnostic; +import org.wpilib.annotation.NoDiscard; + +/** Checks for usages of methods that require their return values to be used. */ +public class ReturnValueUsedListener implements TaskListener { + private final JavacTask m_task; + private final Set m_visitedCUs = new HashSet<>(); + + public ReturnValueUsedListener(JavacTask task) { + m_task = task; + } + + @Override + public void finished(TaskEvent e) { + // We override `finished` instead of `started` because we want to run after the + // ANALYZE attribution phase has completed and assigned types to elements in the AST + // Track the visited CUs to avoid re-processing the same CU multiple times when we call + // `Trees.getElement()` on a tree path. + if (e.getKind() == TaskEvent.Kind.ANALYZE && m_visitedCUs.add(e.getCompilationUnit())) { + e.getCompilationUnit().accept(new Scanner(e.getCompilationUnit()), null); + } + } + + 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 Void visitMethodInvocation(MethodInvocationTree node, Void unused) { + checkIgnoredExpression(node); + return super.visitMethodInvocation(node, unused); + } + + @Override + public Void visitNewClass(NewClassTree node, Void unused) { + checkIgnoredExpression(node); + return super.visitNewClass(node, unused); + } + + /** + * Common logic for both method invocations and constructor calls when they appear as + * stand-alone expression statements (i.e., their result is ignored). + */ + private void checkIgnoredExpression(Tree node) { + var path = m_trees.getPath(m_root, node); + + // Walk the tree upwards to see if the node is directly or indirectly annotated with + // @SuppressWarnings("NoDiscard") or @SuppressWarnings("all"). If so, then we ignore any + // @NoDiscard messages for this node + for (var currentPath = path; currentPath != null; currentPath = currentPath.getParentPath()) { + var element = m_trees.getElement(currentPath); + if (element == null) { + continue; + } + + if (element.getAnnotation(SuppressWarnings.class) != null) { + String[] suppressions = element.getAnnotation(SuppressWarnings.class).value(); + for (String suppression : suppressions) { + if ("NoDiscard".equals(suppression) || "all".equals(suppression)) { + return; + } + } + } + } + + var parentPath = (path == null) ? null : path.getParentPath(); + if (parentPath == null || parentPath.getLeaf().getKind() != Tree.Kind.EXPRESSION_STATEMENT) { + // If the parent node is an expression statement, then the value is ignored. + // Otherwise, the value is used and we can ignore this site. + return; + } + + // Resolve the static type of the expression + TypeMirror type = getType(node); + if (type == null || type.getKind() == TypeKind.VOID) { + // Skip void (e.g., void-returning methods) + return; + } + + // Check @NoDiscard on the invoked executable (method or constructor) + var invoked = getInvokedExecutable(node); + if (invoked != null) { + List messages = getNoDiscardMessages(invoked); + for (String msg : messages) { + m_trees.printMessage(Diagnostic.Kind.ERROR, msg, node, m_root); + } + } + } + + private TypeMirror getType(Tree node) { + var path = m_trees.getPath(m_root, node); + if (path == null) { + return null; + } + // Requires running after ANALYZE attribution has completed for this CU. + return m_trees.getTypeMirror(path); + } + + private ExecutableElement getInvokedExecutable(Tree node) { + var path = m_trees.getPath(m_root, node); + if (path == null) { + return null; + } + var el = m_trees.getElement(path); + return (el instanceof ExecutableElement ee) ? ee : null; + } + + /** + * Collects all @NoDiscard messages applicable to the given executable: - The method/constructor + * itself (if annotated) - The return type (if declared) including its superclasses and all + * implemented interfaces Returns formatted diagnostics messages ready to print. + */ + private List getNoDiscardMessages(ExecutableElement method) { + List messages = new ArrayList<>(); + + // 1) Method-level @NoDiscard + var methodNoDiscard = method.getAnnotation(NoDiscard.class); + if (methodNoDiscard != null) { + String msg = methodNoDiscard.value(); + if (msg.isEmpty()) { + messages.add("Result of @NoDiscard method is ignored"); + } else { + messages.add(msg); + } + } + + // 2) Type-level @NoDiscard (classes + interfaces recursively) + TypeElement targetType = null; + if (method.getKind() == ElementKind.CONSTRUCTOR) { + // For constructors, the "return type" is the enclosing type + var enclosing = method.getEnclosingElement(); + if (enclosing instanceof TypeElement te) { + targetType = te; + } + } else { + var returnType = method.getReturnType(); + if (returnType instanceof DeclaredType dt && dt.asElement() instanceof TypeElement te) { + targetType = te; + } + } + if (targetType != null) { + Set seen = new HashSet<>(); + collectNoDiscardMessagesFromTypeHierarchy(targetType, seen, messages); + } + + return messages; + } + + /** + * Searches for @NoDiscard on the provided type element, its superclasses, and all implemented + * interfaces (recursively). Appends formatted messages to the provided list for every match. + * + * @param type The type element to search + * @param seen A set of type elements that have already been searched + * @param out The list to append messages to + */ + private void collectNoDiscardMessagesFromTypeHierarchy( + TypeElement type, Set seen, List out) { + if (type == null || !seen.add(type)) { + return; + } + + // Check this type directly + var typeNoDiscard = type.getAnnotation(NoDiscard.class); + if (typeNoDiscard != null) { + String message = typeNoDiscard.value(); + if (message.isEmpty()) { + out.add( + "Result of method returning @NoDiscard type %s is ignored" + .formatted(type.getQualifiedName())); + } else { + out.add(message); + } + } + + // Check superclass chain + var superMirror = type.getSuperclass(); + if (superMirror != null && superMirror.getKind() != TypeKind.NONE) { + var superEl = m_task.getTypes().asElement(superMirror); + if (superEl instanceof TypeElement ste) { + collectNoDiscardMessagesFromTypeHierarchy(ste, seen, out); + } + } + + // Check all implemented interfaces (recursively) + for (var iface : type.getInterfaces()) { + var ifaceEl = m_task.getTypes().asElement(iface); + if (ifaceEl instanceof TypeElement ite) { + collectNoDiscardMessagesFromTypeHierarchy(ite, seen, out); + } + } + } + } +} diff --git a/javacPlugin/src/main/java/org/wpilib/javacplugin/WPILibJavacPlugin.java b/javacPlugin/src/main/java/org/wpilib/javacplugin/WPILibJavacPlugin.java new file mode 100644 index 0000000000..87bc8f43f4 --- /dev/null +++ b/javacPlugin/src/main/java/org/wpilib/javacplugin/WPILibJavacPlugin.java @@ -0,0 +1,31 @@ +// 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.JavacTask; +import com.sun.source.util.Plugin; + +/** + * A javac compiler plugin that adds compiler warnings for incorrect usage of WPILib types. Also + * supports WPILib's custom annotations like @NoDiscard. + */ +public class WPILibJavacPlugin implements Plugin { + @Override + public String getName() { + return "WPILib"; + } + + @Override + public void init(JavacTask task, String... args) { + task.addTaskListener(new ReturnValueUsedListener(task)); + } + + @Override + public boolean autoStart() { + // autoStart means we don't need to manually pass -Xplugin:WPILib to the javac compiler args + // for the plugin to run + return true; + } +} diff --git a/javacPlugin/src/main/resources/META-INF/services/com.sun.source.util.Plugin b/javacPlugin/src/main/resources/META-INF/services/com.sun.source.util.Plugin new file mode 100644 index 0000000000..e1b3dbab39 --- /dev/null +++ b/javacPlugin/src/main/resources/META-INF/services/com.sun.source.util.Plugin @@ -0,0 +1 @@ +org.wpilib.javacplugin.WPILibJavacPlugin diff --git a/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestOptions.java b/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestOptions.java new file mode 100644 index 0000000000..a5356b4c81 --- /dev/null +++ b/javacPlugin/src/test/java/org/wpilib/javacplugin/CompileTestOptions.java @@ -0,0 +1,13 @@ +// 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/ReturnValueUsedListenerTest.java b/javacPlugin/src/test/java/org/wpilib/javacplugin/ReturnValueUsedListenerTest.java new file mode 100644 index 0000000000..65a21ce2f0 --- /dev/null +++ b/javacPlugin/src/test/java/org/wpilib/javacplugin/ReturnValueUsedListenerTest.java @@ -0,0 +1,638 @@ +// 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.CompileTestOptions.kJavaVersionOptions; + +import com.google.testing.compile.Compilation; +import com.google.testing.compile.JavaFileObjects; +import org.junit.jupiter.api.Test; + +class ReturnValueUsedListenerTest { + @Test + void nodiscardReturnValueIsUsed() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + class Example { + @NoDiscard + int getI() { return 0; } + + void usage() { + int i = getI(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void nodiscardReturnValueUnused() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + class Example { + @NoDiscard + int getI() { return 0; } + + void usage() { + getI(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Result of @NoDiscard method is ignored", error.getMessage(null)); + } + + @Test + void nodiscardOnClass() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + @NoDiscard + class Example { + Example getExample() { return new Example(); } + + void usage() { + getExample(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Result of method returning @NoDiscard type frc.robot.Example is ignored", + error.getMessage(null)); + } + + @Test + void nodiscardOnClassCustomMessage() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + @NoDiscard("Custom message") + class Example { + Example getExample() { return new Example(); } + + void usage() { + getExample(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Custom message", error.getMessage(null)); + } + + @Test + void nodiscardOnClassAndMethod() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + @NoDiscard + class Example { + @NoDiscard + Example getExample() { return new Example(); } + + void usage() { + getExample(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(2, compilation.errors().size()); + var error1 = compilation.errors().get(0); + var error2 = compilation.errors().get(1); + assertEquals("Result of @NoDiscard method is ignored", error1.getMessage(null)); + assertEquals( + "Result of method returning @NoDiscard type frc.robot.Example is ignored", + error2.getMessage(null)); + } + + @Test + void nodiscardOnInheritedClass() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + @NoDiscard("Objects of type `Base` must be used") + abstract class Base { } + + class Example extends Base { + Example getExample() { return new Example(); } + + void usage() { + getExample(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Objects of type `Base` must be used", error.getMessage(null)); + } + + @Test + void nodiscardOnSingleInterface() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + @NoDiscard("Objects implementing `I` must be used") + interface I { } + + class Example implements I { + Example getExample() { return new Example(); } + + void usage() { + getExample(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Objects implementing `I` must be used", error.getMessage(null)); + } + + @Test + void nodiscardOnMultipleInterfaces() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + @NoDiscard("Objects implementing `I` must be used") + interface I { } + + @NoDiscard("Objects implementing `I2` must be used") + interface I2 { } + + class Example implements I, I2 { + Example getExample() { return new Example(); } + + void usage() { + getExample(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(2, compilation.errors().size()); + var error1 = compilation.errors().get(0); + var error2 = compilation.errors().get(1); + assertEquals("Objects implementing `I` must be used", error1.getMessage(null)); + assertEquals("Objects implementing `I2` must be used", error2.getMessage(null)); + } + + @Test + void nodiscardCustomMessage() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + class Example { + @NoDiscard("Custom message") + int getI() { return 0; } + + void usage() { + getI(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Custom message", error.getMessage(null)); + } + + @Test + void nodiscardMessageEmptyString() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + class Example { + @NoDiscard("") + int getI() { return 0; } + + void usage() { + getI(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals("Result of @NoDiscard method is ignored", error.getMessage(null)); + } + + @Test + void nodiscardOnVoidMethod() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + class Example { + @NoDiscard + void voidMethod() { } + + void usage() { + voidMethod(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void suppressWarningsOnNoDiscardMethod() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + class Example { + @NoDiscard + Object get() { return null; } + + @SuppressWarnings("NoDiscard") + void usage() { + get(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void suppressWarningsAllOnNoDiscardMethod() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + class Example { + @NoDiscard + Object get() { return null; } + + @SuppressWarnings("all") + void usage() { + get(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void suppressWarningsOnNoDiscardClass() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + @SuppressWarnings("NoDiscard") + class Example { + @NoDiscard + Object get() { return null; } + + void usage() { + get(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void suppressWarningsAllOnNoDiscardClass() { + String source = + """ + package frc.robot; + + import org.wpilib.annotation.NoDiscard; + + @SuppressWarnings("all") + class Example { + @NoDiscard + Object get() { return null; } + + void usage() { + get(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void commandsv2CommandFactoryResultIsAssigned() { + String source = + """ + package frc.robot; + + import edu.wpi.first.wpilibj2.command.Command; + import edu.wpi.first.wpilibj2.command.Commands; + import org.wpilib.annotation.NoDiscard; + + class Example { + Command getCommand() { + return Commands.print(""); + } + + void usage() { + Command theCommand = getCommand(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void commandsv2CommandFactoryResultIsPassed() { + String source = + """ + package frc.robot; + + import edu.wpi.first.wpilibj2.command.Command; + import edu.wpi.first.wpilibj2.command.Commands; + import org.wpilib.annotation.NoDiscard; + + class Example { + Command getCommand() { + return Commands.print(""); + } + + void usage() { + System.out.println(getCommand()); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void commandsv2CommandFactoryResultIsChainedAndUsed() { + String source = + """ + package frc.robot; + + import edu.wpi.first.wpilibj2.command.Command; + import edu.wpi.first.wpilibj2.command.Commands; + import org.wpilib.annotation.NoDiscard; + + class Example { + Command getCommand() { + return Commands.print(""); + } + + void usage() { + Command theCommand = getCommand().withName("The name"); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).succeededWithoutWarnings(); + } + + @Test + void commandsv2CommandFactoryResultNotUsed() { + String source = + """ + package frc.robot; + + import edu.wpi.first.wpilibj2.command.Command; + import edu.wpi.first.wpilibj2.command.Commands; + import org.wpilib.annotation.NoDiscard; + + class Example { + Command getCommand() { + return Commands.print(""); + } + + void usage() { + getCommand(); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Commands must be used! Did you mean to bind it to a trigger?", error.getMessage(null)); + } + + @Test + void commandsv2CommandFactoryResultIsChainedAndNotUsed() { + String source = + """ + package frc.robot; + + import edu.wpi.first.wpilibj2.command.Command; + import edu.wpi.first.wpilibj2.command.Commands; + import org.wpilib.annotation.NoDiscard; + + class Example { + Command getCommand() { + return Commands.print(""); + } + + void usage() { + getCommand().withName("The name"); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Commands must be used! Did you mean to bind it to a trigger?", error.getMessage(null)); + } + + @Test + void commandsv2NewCommandInstanceNotUsed() { + String source = + """ + package frc.robot; + + import edu.wpi.first.wpilibj2.command.Command; + import edu.wpi.first.wpilibj2.command.Commands; + import edu.wpi.first.wpilibj2.command.WaitCommand; + import org.wpilib.annotation.NoDiscard; + + class Example { + void usage() { + new WaitCommand(1); + } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .compile(JavaFileObjects.forSourceString("frc.robot.Example", source)); + + assertThat(compilation).failed(); + assertEquals(1, compilation.errors().size()); + var error = compilation.errors().get(0); + assertEquals( + "Commands must be used! Did you mean to bind it to a trigger?", error.getMessage(null)); + } +} diff --git a/settings.gradle b/settings.gradle index 2064a767cd..3c34412c98 100644 --- a/settings.gradle +++ b/settings.gradle @@ -33,6 +33,8 @@ include 'wpilibcIntegrationTests' include 'wpilibjExamples' include 'wpilibjIntegrationTests' include 'wpilibj' +include 'javacPlugin' +include 'wpiannotations' include 'wpiunits' include 'crossConnIntegrationTests' include 'fieldImages' diff --git a/wpiannotations/BUILD.bazel b/wpiannotations/BUILD.bazel new file mode 100644 index 0000000000..ee4b2401c0 --- /dev/null +++ b/wpiannotations/BUILD.bazel @@ -0,0 +1,8 @@ +load("@rules_java//java:defs.bzl", "java_library") + +java_library( + name = "wpiannotations", + srcs = glob(["src/main/java/**/*.java"]), + visibility = ["//visibility:public"], + deps = [], +) diff --git a/wpiannotations/CMakeLists.txt b/wpiannotations/CMakeLists.txt new file mode 100644 index 0000000000..11eabdbc29 --- /dev/null +++ b/wpiannotations/CMakeLists.txt @@ -0,0 +1,37 @@ +project(wpiannotations) + +# Java bindings +if(WITH_JAVA) + include(UseJava) + + file(GLOB_RECURSE JAVA_SOURCES src/main/java/*.java) + + add_jar( + wpiannotations_jar + ${JAVA_SOURCES} + OUTPUT_NAME wpiannotations + OUTPUT_DIR ${WPILIB_BINARY_DIR}/${java_lib_dest} + ) + set_property(TARGET wpiannotations_jar PROPERTY FOLDER "java") + + install_jar(wpiannotations_jar DESTINATION ${java_lib_dest}) + install_jar_exports( + TARGETS wpiannotations_jar + FILE wpiannotations.cmake + DESTINATION share/wpiannotations + ) + install(FILES wpiannotations-config.cmake DESTINATION share/wpiannotations) +endif() + +if(WITH_JAVA_SOURCE) + include(UseJava) + include(CreateSourceJar) + add_source_jar( + wpiannotations_src_jar + BASE_DIRECTORIES ${CMAKE_CURRENT_SOURCE_DIR}/src/main/java + OUTPUT_NAME wpiannotations-sources + ) + set_property(TARGET wpiannotations_src_jar PROPERTY FOLDER "java") + + install_jar(wpiannotations_src_jar DESTINATION ${java_lib_dest}) +endif() diff --git a/wpiannotations/build.gradle b/wpiannotations/build.gradle new file mode 100644 index 0000000000..2a7a0881d7 --- /dev/null +++ b/wpiannotations/build.gradle @@ -0,0 +1,11 @@ +ext { + useJava = true + useCpp = false + baseId = 'annotations' + groupId = 'org.wpilib' + + nativeName = '' + devMain = '' +} + +apply from: "${rootDir}/shared/java/javacommon.gradle" diff --git a/wpiannotations/src/main/java/org/wpilib/annotation/NoDiscard.java b/wpiannotations/src/main/java/org/wpilib/annotation/NoDiscard.java new file mode 100644 index 0000000000..408d1fb861 --- /dev/null +++ b/wpiannotations/src/main/java/org/wpilib/annotation/NoDiscard.java @@ -0,0 +1,27 @@ +// 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.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Marks a method as returning a value that must be used. The WPILib compiler plugin will check for + * uses of methods with this annotation and report a compiler error if the value is unused. Marking + * a class or interface as {@code @NoDiscard} will act as if any method that returns that type or + * any subclass or implementor of that type has been marked with {@code @NoDiscard}. + */ +@Target({ElementType.METHOD, ElementType.TYPE}) +@Retention(RetentionPolicy.CLASS) // needs to be stored in the class for use by libraries +public @interface NoDiscard { + /** + * An error message to display if the return value is not used. + * + * @return The error message. + */ + String value() default ""; +} diff --git a/wpiannotations/wpiannotations-config.cmake b/wpiannotations/wpiannotations-config.cmake new file mode 100644 index 0000000000..fa28e4a9c2 --- /dev/null +++ b/wpiannotations/wpiannotations-config.cmake @@ -0,0 +1,2 @@ +get_filename_component(SELF_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH) +include(${SELF_DIR}/wpiannotations.cmake) diff --git a/wpilibNewCommands/BUILD.bazel b/wpilibNewCommands/BUILD.bazel index 6add11137a..fc9326baf2 100644 --- a/wpilibNewCommands/BUILD.bazel +++ b/wpilibNewCommands/BUILD.bazel @@ -37,11 +37,14 @@ cc_library( java_library( name = "wpilibNewCommands-java", srcs = glob(["src/main/java/**/*.java"]) + [":generated_java"], + exported_plugins = ["//javacPlugin:plugin"], + plugins = ["//javacPlugin:plugin"], visibility = ["//visibility:public"], deps = [ "//cscore:cscore-java", "//hal:hal-java", "//ntcore:networktables-java", + "//wpiannotations", "//wpilibj", "//wpimath:wpimath-java", "//wpinet:wpinet-java", @@ -79,8 +82,10 @@ java_binary( srcs = ["src/dev/java/edu/wpi/first/wpilibj2/commands/DevMain.java"], main_class = "edu.wpi.first.wpilibj2.commands.DevMain", deps = [ + ":wpilibNewCommands-java", "//hal:hal-java", "//ntcore:networktables-java", + "//wpiannotations", "//wpimath:wpimath-java", "//wpiutil:wpiutil-java", ], diff --git a/wpilibNewCommands/CMakeLists.txt b/wpilibNewCommands/CMakeLists.txt index f7c664802e..f7959bff80 100644 --- a/wpilibNewCommands/CMakeLists.txt +++ b/wpilibNewCommands/CMakeLists.txt @@ -22,6 +22,7 @@ if(WITH_JAVA) wpiunits_jar wpiutil_jar wpilibj_jar + wpiannotations_jar OUTPUT_NAME wpilibNewCommands OUTPUT_DIR ${WPILIB_BINARY_DIR}/${java_lib_dest} ) diff --git a/wpilibNewCommands/build.gradle b/wpilibNewCommands/build.gradle index 884c18b65c..fb3cf2dbb3 100644 --- a/wpilibNewCommands/build.gradle +++ b/wpilibNewCommands/build.gradle @@ -21,7 +21,9 @@ dependencies { implementation project(':hal') implementation project(':wpimath') implementation project(':wpilibj') + implementation project(':wpiannotations') testImplementation 'org.mockito:mockito-core:4.1.0' + annotationProcessor project(':javacPlugin') } sourceSets.main.java.srcDir "${projectDir}/src/generated/main/java" diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java index d46ce313c6..68944440e0 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java @@ -16,6 +16,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.Set; import java.util.function.BooleanSupplier; +import org.wpilib.annotation.NoDiscard; /** * A state machine representing a complete action to be performed by the robot. Commands are run by @@ -27,6 +28,7 @@ import java.util.function.BooleanSupplier; * *

This class is provided by the NewCommands VendorDep */ +@NoDiscard("Commands must be used! Did you mean to bind it to a trigger?") public abstract class Command implements Sendable { /** Requirements set. */ private final Set m_requirements = new HashSet<>();