From 0dbcaea976de46aa1f82eaf359be1d37dd4621ec Mon Sep 17 00:00:00 2001 From: Vasil Vasilev Date: Fri, 31 Jul 2026 11:25:36 +0200 Subject: [PATCH 1/3] Replace the Scala threading instrumenter with IntelliJ's TMH Java sources The Scala port of the threading annotation instrumentation (SCL-18660, Feb 2, 2024) drifted from upstream: three days after the port, IntelliJ's JPS switched to generating ThreadingAssertions calls (IJPL-255) and chose softAssertReadAccess (logs an error instead of throwing) for @RequiresReadLock, while our port generates the hard assertReadAccess. Copy TMHInstrumenter, TMHAssertionGenerator and TMHAssertionGenerator2 verbatim from IntelliJ IDEA Community Edition (Apache 2.0, commit 7a35a7d7fe64, the same commit the notNullVerification sources were copied from), with only the package, the ASM package, Opcodes.API_VERSION -> Opcodes.ASM9 and the FailSafeMethodVisitor import changed. TMHAssertionGenerator1 (the legacy pre-ThreadingAssertions fallback) is not needed and therefore not copied. ThreadingAnnotationInstrumenter.scala becomes a thin wrapper around TMHInstrumenter, so ManipulateBytecode is unchanged. The org.jetbrains annotations dependency moves to compile scope because the copied sources use @ApiStatus.Internal and @NotNull. Behavior change: @RequiresReadLock violations now log an error instead of throwing, matching IntelliJ IDEA's own instrumentation. Co-Authored-By: Claude Fable 5 --- README.md | 4 + build.sbt | 5 +- .../TMHAssertionGenerator.java | 17 ++ .../TMHAssertionGenerator2.java | 112 +++++++++++++ .../threadingModelHelper/TMHInstrumenter.java | 134 ++++++++++++++++ .../ThreadingAnnotationInstrumenter.scala | 107 ++----------- .../InstrumenterTestHarness.scala | 59 +++++++ .../NotNullInstrumenterTest.scala | 49 +----- .../ThreadingAnnotationInstrumenterTest.scala | 150 ++++++++++++++++++ 9 files changed, 497 insertions(+), 140 deletions(-) create mode 100644 ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHAssertionGenerator.java create mode 100644 ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHAssertionGenerator2.java create mode 100644 ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHInstrumenter.java create mode 100644 ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/InstrumenterTestHarness.scala create mode 100644 ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenterTest.scala diff --git a/README.md b/README.md index b55599a5..d38d8642 100644 --- a/README.md +++ b/README.md @@ -437,6 +437,10 @@ Generate JVM bytecode to assert that a method is called on the correct IDEA thre 4. `com.intellij.util.concurrency.annotations.RequiresReadLockAbsence` 5. `com.intellij.util.concurrency.annotations.RequiresWriteLock` +This is the same instrumentation that IntelliJ IDEA applies to its own codebase. Note that, matching IntelliJ IDEA's +behavior, `@RequiresReadLock` generates a call to `ThreadingAssertions.softAssertReadAccess`, which logs an error +instead of throwing an exception when read access is missing. + See: [IntelliJ IDEA ThreadingAssertions.java](https://github.com/JetBrains/intellij-community/blob/5758eb99b4a1971ebe75cda755693cc930949465/platform/core-api/src/com/intellij/util/concurrency/ThreadingAssertions.java) #### `instrumentNotNullAnnotations :: SettingKey[Boolean]` diff --git a/build.sbt b/build.sbt index 79890b5f..df030d48 100644 --- a/build.sbt +++ b/build.sbt @@ -134,8 +134,9 @@ lazy val ideaSupport = (project in file("ideaSupport")) "io.get-coursier" %% "coursier" % "2.1.24", "commons-io" % "commons-io" % "2.22.0", - // Used to compile the test fixtures of the @NotNull instrumentation tests - "org.jetbrains" % "annotations" % "26.1.0" % Test + // Used by the sources copied from IntelliJ IDEA in the threadingModelHelper package + // and to compile the test fixtures of the @NotNull instrumentation tests + "org.jetbrains" % "annotations" % "26.1.0" ), ) diff --git a/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHAssertionGenerator.java b/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHAssertionGenerator.java new file mode 100644 index 00000000..5350c6d6 --- /dev/null +++ b/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHAssertionGenerator.java @@ -0,0 +1,17 @@ +// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +// Copied from IntelliJ IDEA Community Edition (commit 7a35a7d7fe64), original package org.jetbrains.jps.devkit.threadingModelHelper. +// The only changes are the package name and the ASM package (org.jetbrains.org.objectweb.asm -> org.objectweb.asm). +package org.jetbrains.sbtidea.instrumentation.threadingModelHelper; + +import org.jetbrains.annotations.ApiStatus; +import org.objectweb.asm.AnnotationVisitor; +import org.objectweb.asm.MethodVisitor; + +@ApiStatus.Internal +public interface TMHAssertionGenerator { + boolean isMyAnnotation(String annotationDescriptor); + + AnnotationVisitor getAnnotationChecker(int api, Runnable onShouldGenerateAssertion); + + void generateAssertion(MethodVisitor writer, int methodStartLineNumber); +} diff --git a/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHAssertionGenerator2.java b/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHAssertionGenerator2.java new file mode 100644 index 00000000..fc7dabd6 --- /dev/null +++ b/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHAssertionGenerator2.java @@ -0,0 +1,112 @@ +// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +// Copied from IntelliJ IDEA Community Edition (commit 7a35a7d7fe64), original package org.jetbrains.jps.devkit.threadingModelHelper. +// The only changes are the package name and the ASM package (org.jetbrains.org.objectweb.asm -> org.objectweb.asm). +package org.jetbrains.sbtidea.instrumentation.threadingModelHelper; + +import org.jetbrains.annotations.ApiStatus; +import org.jetbrains.annotations.NotNull; +import org.objectweb.asm.AnnotationVisitor; +import org.objectweb.asm.Label; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; + +import java.util.Set; + +@ApiStatus.Internal +public final class TMHAssertionGenerator2 implements TMHAssertionGenerator { + + private static final String THREAD_ASSERTIONS_CLASS_NAME = "com/intellij/util/concurrency/ThreadingAssertions"; + private static final String GENERATE_ASSERTION_PARAMETER = "generateAssertion"; + + private final String myThreadAssertionsClassName; + private final Type myAnnotationClass; + private final String myAssertionMethodName; + + TMHAssertionGenerator2(String threadAssertionsClassName, Type annotationClass, String assertionMethodName) { + myThreadAssertionsClassName = threadAssertionsClassName; + myAnnotationClass = annotationClass; + myAssertionMethodName = assertionMethodName; + } + + @Override + public boolean isMyAnnotation(String annotationDescriptor) { + return myAnnotationClass.getDescriptor().equals(annotationDescriptor); + } + + @Override + public AnnotationVisitor getAnnotationChecker(int api, Runnable onShouldGenerateAssertion) { + return new AnnotationChecker(api, onShouldGenerateAssertion); + } + + @Override + public void generateAssertion(MethodVisitor writer, int methodStartLineNumber) { + if (methodStartLineNumber != -1) { + Label generatedCodeStart = new Label(); + writer.visitLabel(generatedCodeStart); + writer.visitLineNumber(methodStartLineNumber, generatedCodeStart); + } + writer.visitMethodInsn( + Opcodes.INVOKESTATIC, + myThreadAssertionsClassName, + myAssertionMethodName, + "()V", + false + ); + } + + static class AnnotationChecker extends AnnotationVisitor { + private boolean myShouldGenerateAssertion = true; + private final Runnable myOnShouldGenerateAssertion; + + private AnnotationChecker(int api, Runnable onShouldGenerateAssertion) { + super(api); + myOnShouldGenerateAssertion = onShouldGenerateAssertion; + } + + @Override + public void visit(String annotationParameterName, Object value) { + if (GENERATE_ASSERTION_PARAMETER.equals(annotationParameterName) && Boolean.FALSE.equals(value)) { + myShouldGenerateAssertion = false; + } + } + + @Override + public void visitEnd() { + if (myShouldGenerateAssertion) { + myOnShouldGenerateAssertion.run(); + } + } + } + + // TODO avoid hardcoding annotation names + static @NotNull Set generators() { + return GENERATORS; + } + + private static final Set GENERATORS = generators( + THREAD_ASSERTIONS_CLASS_NAME, + "com/intellij/util/concurrency/annotations" + ); + + public static @NotNull Set generators( + @NotNull String threadAssertionsClassName, + @NotNull String packageString + ) { + return Set.of( + generator(threadAssertionsClassName, packageString + "/RequiresEdt", "assertEventDispatchThread"), + generator(threadAssertionsClassName, packageString + "/RequiresBackgroundThread", "assertBackgroundThread"), + generator(threadAssertionsClassName, packageString + "/RequiresReadLock", "softAssertReadAccess"), + generator(threadAssertionsClassName, packageString + "/RequiresReadLockAbsence", "assertNoReadAccess"), + generator(threadAssertionsClassName, packageString + "/RequiresWriteLock", "assertWriteAccess") + ); + } + + private static @NotNull TMHAssertionGenerator generator( + @NotNull String threadAssertionsClassName, + @NotNull String annotationClassName, + @NotNull String assertionMethodName + ) { + return new TMHAssertionGenerator2(threadAssertionsClassName, Type.getType("L" + annotationClassName + ";"), assertionMethodName); + } +} diff --git a/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHInstrumenter.java b/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHInstrumenter.java new file mode 100644 index 00000000..268c42f6 --- /dev/null +++ b/ideaSupport/src/main/java/org/jetbrains/sbtidea/instrumentation/threadingModelHelper/TMHInstrumenter.java @@ -0,0 +1,134 @@ +// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +// Copied from IntelliJ IDEA Community Edition (commit 7a35a7d7fe64), original package org.jetbrains.jps.devkit.threadingModelHelper. +// The only changes are the package name, the ASM package (org.jetbrains.org.objectweb.asm -> org.objectweb.asm), +// Opcodes.API_VERSION (a JetBrains ASM addition) -> Opcodes.ASM9 and the import of FailSafeMethodVisitor +// (originally com.intellij.compiler.instrumentation.FailSafeMethodVisitor). +package org.jetbrains.sbtidea.instrumentation.threadingModelHelper; + +import org.jetbrains.sbtidea.instrumentation.notNullVerification.FailSafeMethodVisitor; +import org.objectweb.asm.AnnotationVisitor; +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.Label; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; + +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +public final class TMHInstrumenter { + public static boolean instrument(ClassReader classReader, + ClassVisitor classWriter, + Set generators, + boolean generateLineNumbers) { + AnnotatedMethodsCollector collector = new AnnotatedMethodsCollector(generators); + int options = ClassReader.SKIP_FRAMES; + if (!generateLineNumbers) { + options |= ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG; + } + classReader.accept(collector, options); + if (collector.annotatedMethods.isEmpty()) { + return false; + } + Instrumenter instrumenter = new Instrumenter(classWriter, collector.annotatedMethods); + classReader.accept(instrumenter, 0); + return true; + } + + private static final class AnnotatedMethodsCollector extends ClassVisitor { + final Set assertionGenerators; + final Map annotatedMethods = new HashMap<>(); + + AnnotatedMethodsCollector(Set assertionGenerators) { + super(Opcodes.ASM9); + this.assertionGenerators = assertionGenerators; + } + + @Override + public MethodVisitor visitMethod(int access, final String name, final String methodDescriptor, String signature, String[] exceptions) { + return new MethodVisitor(Opcodes.ASM9) { + private final MethodKey methodKey = new MethodKey(name, methodDescriptor); + private boolean annotated = false; + private boolean firstLineNumberVisited = false; + + @Override + public AnnotationVisitor visitAnnotation(String annotationDescriptor, boolean visible) { + for (TMHAssertionGenerator assertionGenerator : assertionGenerators) { + if (assertionGenerator.isMyAnnotation(annotationDescriptor)) { + return assertionGenerator.getAnnotationChecker(Opcodes.ASM9, () -> { + annotatedMethods.put(methodKey, new InstrumentationInfo(assertionGenerator)); + annotated = true; + }); + } + } + return super.visitAnnotation(annotationDescriptor, visible); + } + + @Override + public void visitLineNumber(int line, Label start) { + super.visitLineNumber(line, start); + if (annotated && !firstLineNumberVisited) { + annotatedMethods.get(methodKey).methodStartLineNumber = line; + firstLineNumberVisited = true; + } + } + }; + } + } + + private static final class Instrumenter extends ClassVisitor { + private final Map myAnnotatedMethods; + + Instrumenter(ClassVisitor writer, Map annotatedMethods) { + super(Opcodes.ASM9, writer); + myAnnotatedMethods = annotatedMethods; + } + + @Override + public MethodVisitor visitMethod(int access, String name, String descriptor, String signature, String[] exceptions) { + InstrumentationInfo instrumentationInfo = myAnnotatedMethods.get(new MethodKey(name, descriptor)); + if (instrumentationInfo == null) { + return super.visitMethod(access, name, descriptor, signature, exceptions); + } + return new FailSafeMethodVisitor(Opcodes.ASM9, super.visitMethod(access, name, descriptor, signature, exceptions)) { + @Override + public void visitCode() { + instrumentationInfo.assertionGenerator.generateAssertion(mv, instrumentationInfo.methodStartLineNumber); + super.visitCode(); + } + }; + } + } + + private static final class MethodKey { + final String name; + final String descriptor; + + private MethodKey(String name, String descriptor) { + this.name = name; + this.descriptor = descriptor; + } + + @Override + public int hashCode() { + int result = 1; + result = 31 * result + name.hashCode(); + result = 31 * result + descriptor.hashCode(); + return result; + } + + @Override + public boolean equals(Object obj) { + return obj == this || + obj instanceof MethodKey && ((MethodKey)obj).name.equals(name) && ((MethodKey)obj).descriptor.equals(descriptor); + } + } + + private static final class InstrumentationInfo { + final TMHAssertionGenerator assertionGenerator; + int methodStartLineNumber = -1; + + private InstrumentationInfo(TMHAssertionGenerator generator) {assertionGenerator = generator;} + } +} diff --git a/ideaSupport/src/main/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenter.scala b/ideaSupport/src/main/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenter.scala index 62804f24..6ca0e55f 100644 --- a/ideaSupport/src/main/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenter.scala +++ b/ideaSupport/src/main/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenter.scala @@ -1,106 +1,27 @@ package org.jetbrains.sbtidea.instrumentation -import org.objectweb.asm.* +import org.jetbrains.sbtidea.instrumentation.notNullVerification.FailSafeClassReader +import org.jetbrains.sbtidea.instrumentation.threadingModelHelper.{TMHAssertionGenerator, TMHAssertionGenerator2, TMHInstrumenter} +import org.objectweb.asm.ClassWriter import java.nio.file.{Files, Path} +import java.util private object ThreadingAnnotationInstrumenter { + private val Generators: util.Set[? <: TMHAssertionGenerator] = TMHAssertionGenerator2.generators( + "com/intellij/util/concurrency/ThreadingAssertions", + "com/intellij/util/concurrency/annotations" + ) + def instrument(classFile: Path): Unit = { val bytes = Files.readAllBytes(classFile) - val reader = new ClassReader(bytes) - if (requiresInstrumentation(reader)) { - doInstrumentation(classFile, reader) - } - } - - private def requiresInstrumentation(reader: ClassReader): Boolean = { - val searcher = new AnnotationSearcher() - reader.accept(searcher, ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES) - searcher.requiresInstrumentation - } - - private def doInstrumentation(classFile: Path, reader: ClassReader): Unit = { + val reader = new FailSafeClassReader(bytes) + // The generated assertion is a zero-argument static ()V call at method entry, + // which changes neither stack map frames nor the maximum stack size. val writer = new ClassWriter(reader, 0) - val instrumenter = new Instrumenter(writer) - reader.accept(instrumenter, 0) - Files.write(classFile, writer.toByteArray) - } - - private val GenerateAssertion = "generateAssertion" - - private val ThreadingAssertionsSignature = "com/intellij/util/concurrency/ThreadingAssertions" - - private val AnnotationClasses: Map[String, String] = - Map( - "RequiresBackgroundThread" -> "assertBackgroundThread", - "RequiresEdt" -> "assertEventDispatchThread", - "RequiresReadLock" -> "assertReadAccess", - "RequiresReadLockAbsence" -> "assertNoReadAccess", - "RequiresWriteLock" -> "assertWriteAccess" - ).map { case (annotation, method) => - val fqcn = s"com.intellij.util.concurrency.annotations.$annotation" - val signature = fqcn.replace('.', '/') - val descriptor = s"L$signature;" - (descriptor, method) - } - - private final class AnnotationSearcher extends ClassVisitor(Opcodes.ASM9) { - var requiresInstrumentation: Boolean = false - - override def visitMethod(access: Int, name: String, descriptor: String, signature: String, exceptions: Array[String]): MethodVisitor = - new MethodVisitor(Opcodes.ASM9) { - override def visitAnnotation(descriptor: String, visible: Boolean): AnnotationVisitor = { - if (!requiresInstrumentation) { - requiresInstrumentation = AnnotationClasses.contains(descriptor) - } - null - } - } - } - - private final class Instrumenter(visitor: ClassVisitor) extends ClassVisitor(Opcodes.ASM9, visitor) { - override def visitMethod(access: Int, name: String, descriptor: String, signature: String, exceptions: Array[String]): MethodVisitor = { - val methodVisitor = super.visitMethod(access, name, descriptor, signature, exceptions) - new MethodInstrumenter(methodVisitor) - } - } - - private final class MethodInstrumenter(visitor: MethodVisitor) extends MethodVisitor(Opcodes.ASM9, visitor) { - private var assertionMethod: Option[String] = None - - override def visitAnnotation(descriptor: String, visible: Boolean): AnnotationVisitor = { - val annotationVisitor = super.visitAnnotation(descriptor, visible) - if (AnnotationClasses.contains(descriptor)) { - new AnnotationChecker(annotationVisitor, () => { - assertionMethod = AnnotationClasses.get(descriptor) - }) - } else annotationVisitor - } - - override def visitCode(): Unit = { - assertionMethod.foreach { method => - super.visitMethodInsn(Opcodes.INVOKESTATIC, ThreadingAssertionsSignature, method, "()V", false) - } - super.visitCode() - } - } - - private final class AnnotationChecker(visitor: AnnotationVisitor, onShouldGenerateAssertion: () => Unit) extends AnnotationVisitor(Opcodes.ASM9, visitor) { - private var shouldGenerateAssertion: Boolean = true - - override def visit(name: String, value: Any): Unit = { - super.visit(name, value) - if (name == GenerateAssertion && value == java.lang.Boolean.FALSE) { - shouldGenerateAssertion = false - } - } - - override def visitEnd(): Unit = { - super.visitEnd() - if (shouldGenerateAssertion) { - onShouldGenerateAssertion() - } + if (TMHInstrumenter.instrument(reader, writer, Generators, /*generateLineNumbers =*/ false)) { + Files.write(classFile, writer.toByteArray) } } } diff --git a/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/InstrumenterTestHarness.scala b/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/InstrumenterTestHarness.scala new file mode 100644 index 00000000..265f6f9f --- /dev/null +++ b/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/InstrumenterTestHarness.scala @@ -0,0 +1,59 @@ +package org.jetbrains.sbtidea.instrumentation + +import org.scalatest.Assertions + +import java.lang.reflect.InvocationTargetException +import java.net.URLClassLoader +import java.nio.charset.StandardCharsets +import java.nio.file.{Files, Path, Paths} +import javax.tools.ToolProvider +import scala.collection.JavaConverters.asScalaIteratorConverter + +/** + * Shared harness for the instrumenter tests: compiles Java fixture sources at test runtime, + * lets the test instrument the resulting class files and loads them in a throwaway + * classloader. Loading and invoking the instrumented classes also runs the JVM bytecode + * verifier over the rewritten methods. + */ +trait InstrumenterTestHarness { this: Assertions => + + protected def annotationsJar: Path = + Paths.get(classOf[org.jetbrains.annotations.NotNull].getProtectionDomain.getCodeSource.getLocation.toURI) + + protected def compileFixture(sources: Map[String, String], debugInfo: Boolean = false): Path = { + val dir = Files.createTempDirectory("instrumentation-test") + val javaFiles = sources.map { case (relativePath, content) => + val file = dir.resolve(relativePath) + Files.createDirectories(file.getParent) + Files.write(file, content.getBytes(StandardCharsets.UTF_8)) + file.toString + }.toSeq + val args = Seq("-classpath", annotationsJar.toString, "-d", dir.toString) ++ + (if (debugInfo) Seq("-g") else Seq.empty) ++ + javaFiles + val exitCode = ToolProvider.getSystemJavaCompiler.run(null, null, null, args: _*) + assert(exitCode == 0, s"javac exited with code $exitCode") + dir + } + + protected def classFiles(dir: Path): Seq[Path] = { + val stream = Files.walk(dir) + try stream.iterator().asScala.filter(_.toString.endsWith(".class")).toList + finally stream.close() + } + + protected def classLoader(dir: Path): ClassLoader = + new URLClassLoader(Array(dir.toUri.toURL), getClass.getClassLoader) + + protected def loadClass(dir: Path, name: String): Class[_] = + classLoader(dir).loadClass(name) + + protected def invokeStatic(clazz: Class[_], methodName: String, args: AnyRef*): AnyRef = { + val method = clazz.getMethods.find(_.getName == methodName) + .getOrElse(fail(s"Method $methodName not found in ${clazz.getName}")) + method.invoke(null, args: _*) + } + + protected def interceptCause(clazz: Class[_], methodName: String, args: AnyRef*): Throwable = + intercept[InvocationTargetException](invokeStatic(clazz, methodName, args: _*)).getCause +} diff --git a/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/NotNullInstrumenterTest.scala b/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/NotNullInstrumenterTest.scala index 6e4cc3a8..2d65bea4 100644 --- a/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/NotNullInstrumenterTest.scala +++ b/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/NotNullInstrumenterTest.scala @@ -3,20 +3,15 @@ package org.jetbrains.sbtidea.instrumentation import org.scalatest.funsuite.AnyFunSuite import org.scalatest.matchers.should.Matchers -import java.lang.reflect.InvocationTargetException import java.net.URLClassLoader -import java.nio.charset.StandardCharsets -import java.nio.file.{Files, Path, Paths} -import javax.tools.ToolProvider -import scala.collection.JavaConverters.asScalaIteratorConverter +import java.nio.file.{Files, Path} /** * Modeled on IntelliJ IDEA's `com.intellij.java.compiler.notNullVerification.NotNullVerifyingInstrumenterTest`: * compile small annotated Java fixtures at test runtime, instrument the class files, load them - * in a throwaway classloader and assert on the thrown exceptions. Loading and invoking the - * instrumented classes also runs the JVM bytecode verifier over the rewritten methods. + * in a throwaway classloader and assert on the thrown exceptions. */ -class NotNullInstrumenterTest extends AnyFunSuite with Matchers { +class NotNullInstrumenterTest extends AnyFunSuite with Matchers with InstrumenterTestHarness { private val NotNullFqn = "org.jetbrains.annotations.NotNull" @@ -173,31 +168,10 @@ class NotNullInstrumenterTest extends AnyFunSuite with Matchers { invokeStatic(loadClass(dir, "KotlinLike"), "test", null) // no assertion is generated } - private def annotationsJar: Path = - Paths.get(classOf[org.jetbrains.annotations.NotNull].getProtectionDomain.getCodeSource.getLocation.toURI) - - private def compileFixture(sources: Map[String, String], debugInfo: Boolean = false): Path = { - val dir = Files.createTempDirectory("notnull-instrumentation-test") - val javaFiles = sources.map { case (relativePath, content) => - val file = dir.resolve(relativePath) - Files.createDirectories(file.getParent) - Files.write(file, content.getBytes(StandardCharsets.UTF_8)) - file.toString - }.toSeq - val args = Seq("-classpath", annotationsJar.toString, "-d", dir.toString) ++ - (if (debugInfo) Seq("-g") else Seq.empty) ++ - javaFiles - val exitCode = ToolProvider.getSystemJavaCompiler.run(null, null, null, args: _*) - assert(exitCode == 0, s"javac exited with code $exitCode") - dir - } - private def instrumentAll(dir: Path, annotations: Seq[String]): Unit = { val loader = new URLClassLoader(Array(dir.toUri.toURL, annotationsJar.toUri.toURL)) try { - val stream = Files.walk(dir) - val classFiles = try stream.iterator().asScala.filter(_.toString.endsWith(".class")).toList finally stream.close() - classFiles.foreach(NotNullInstrumenter.instrument(_, annotations, loader)) + classFiles(dir).foreach(NotNullInstrumenter.instrument(_, annotations, loader)) } finally { loader.close() } @@ -212,19 +186,4 @@ class NotNullInstrumenterTest extends AnyFunSuite with Matchers { instrumentAll(dir, annotations) dir } - - private def classLoader(dir: Path): ClassLoader = - new URLClassLoader(Array(dir.toUri.toURL), getClass.getClassLoader) - - private def loadClass(dir: Path, name: String): Class[_] = - classLoader(dir).loadClass(name) - - private def invokeStatic(clazz: Class[_], methodName: String, args: AnyRef*): AnyRef = { - val method = clazz.getMethods.find(_.getName == methodName) - .getOrElse(fail(s"Method $methodName not found in ${clazz.getName}")) - method.invoke(null, args: _*) - } - - private def interceptCause(clazz: Class[_], methodName: String, args: AnyRef*): Throwable = - intercept[InvocationTargetException](invokeStatic(clazz, methodName, args: _*)).getCause } diff --git a/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenterTest.scala b/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenterTest.scala new file mode 100644 index 00000000..cd8534fa --- /dev/null +++ b/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenterTest.scala @@ -0,0 +1,150 @@ +package org.jetbrains.sbtidea.instrumentation + +import org.scalatest.funsuite.AnyFunSuite +import org.scalatest.matchers.should.Matchers + +import java.nio.file.{Files, Path} +import scala.collection.JavaConverters.asScalaBufferConverter + +/** + * Tests the threading annotation instrumentation against stub versions of the IntelliJ Platform + * annotation and assertion classes, compiled at test runtime. The stub `ThreadingAssertions` + * methods record their invocations, so the tests can assert exactly which assertion was + * generated for which annotation. + */ +class ThreadingAnnotationInstrumenterTest extends AnyFunSuite with Matchers with InstrumenterTestHarness { + + test("each threading annotation generates a call to its ThreadingAssertions method") { + val dir = compileAndInstrument( + "Annotated.java" -> + """import com.intellij.util.concurrency.annotations.*; + |public class Annotated { + | @RequiresEdt + | public static void edt() {} + | @RequiresBackgroundThread + | public static void background() {} + | @RequiresReadLock + | public static void readLock() {} + | @RequiresReadLockAbsence + | public static void readLockAbsence() {} + | @RequiresWriteLock + | public static void writeLock() {} + |}""".stripMargin + ) + val loader = classLoader(dir) + val clazz = loader.loadClass("Annotated") + Seq("edt", "background", "readLock", "readLockAbsence", "writeLock") + .foreach(clazz.getMethod(_).invoke(null)) + recordedCalls(loader) shouldBe Seq( + "assertEventDispatchThread", + "assertBackgroundThread", + "softAssertReadAccess", + "assertNoReadAccess", + "assertWriteAccess" + ) + } + + test("the assertion is generated before the method body") { + val dir = compileAndInstrument( + "BodyOrder.java" -> + """import com.intellij.util.concurrency.ThreadingAssertions; + |import com.intellij.util.concurrency.annotations.RequiresEdt; + |public class BodyOrder { + | @RequiresEdt + | public static void test() { ThreadingAssertions.calls.add("body"); } + |}""".stripMargin + ) + val loader = classLoader(dir) + loader.loadClass("BodyOrder").getMethod("test").invoke(null) + recordedCalls(loader) shouldBe Seq("assertEventDispatchThread", "body") + } + + test("generateAssertion = false disables the instrumentation") { + val dir = compileThreadingFixture( + "OptedOut.java" -> + """import com.intellij.util.concurrency.annotations.RequiresEdt; + |public class OptedOut { + | @RequiresEdt(generateAssertion = false) + | public static void test() {} + |}""".stripMargin + ) + val classFile = dir.resolve("OptedOut.class") + val before = Files.readAllBytes(classFile) + ThreadingAnnotationInstrumenter.instrument(classFile) + val after = Files.readAllBytes(classFile) + assert(java.util.Arrays.equals(before, after), "class file must not be modified") + val loader = classLoader(dir) + loader.loadClass("OptedOut").getMethod("test").invoke(null) + recordedCalls(loader) shouldBe empty + } + + test("classes without threading annotations are not modified") { + val dir = compileThreadingFixture( + "Unannotated.java" -> + """public class Unannotated { + | public static void test() {} + |}""".stripMargin + ) + val classFile = dir.resolve("Unannotated.class") + val before = Files.readAllBytes(classFile) + ThreadingAnnotationInstrumenter.instrument(classFile) + val after = Files.readAllBytes(classFile) + assert(java.util.Arrays.equals(before, after), "class file must not be modified") + } + + test("a method with two threading annotations gets only one assertion (matching IntelliJ's instrumenter)") { + val dir = compileAndInstrument( + "DoubleAnnotated.java" -> + """import com.intellij.util.concurrency.annotations.*; + |public class DoubleAnnotated { + | @RequiresEdt + | @RequiresWriteLock + | public static void test() {} + |}""".stripMargin + ) + val loader = classLoader(dir) + loader.loadClass("DoubleAnnotated").getMethod("test").invoke(null) + recordedCalls(loader) should have size 1 + } + + private val ThreadingAssertionsSource: (String, String) = + "com/intellij/util/concurrency/ThreadingAssertions.java" -> + """package com.intellij.util.concurrency; + |import java.util.ArrayList; + |import java.util.List; + |public class ThreadingAssertions { + | public static final List calls = new ArrayList<>(); + | public static void assertEventDispatchThread() { calls.add("assertEventDispatchThread"); } + | public static void assertBackgroundThread() { calls.add("assertBackgroundThread"); } + | public static void softAssertReadAccess() { calls.add("softAssertReadAccess"); } + | public static void assertNoReadAccess() { calls.add("assertNoReadAccess"); } + | public static void assertWriteAccess() { calls.add("assertWriteAccess"); } + |}""".stripMargin + + private val AnnotationSources: Seq[(String, String)] = + Seq("RequiresEdt", "RequiresBackgroundThread", "RequiresReadLock", "RequiresReadLockAbsence", "RequiresWriteLock") + .map { name => + s"com/intellij/util/concurrency/annotations/$name.java" -> + s"""package com.intellij.util.concurrency.annotations; + |import java.lang.annotation.*; + |@Retention(RetentionPolicy.CLASS) + |@Target({ElementType.METHOD, ElementType.CONSTRUCTOR}) + |public @interface $name { + | boolean generateAssertion() default true; + |}""".stripMargin + } + + private def compileThreadingFixture(fixtureSources: (String, String)*): Path = + compileFixture((AnnotationSources ++ Seq(ThreadingAssertionsSource) ++ fixtureSources).toMap) + + private def compileAndInstrument(fixtureSources: (String, String)*): Path = { + val dir = compileThreadingFixture(fixtureSources: _*) + classFiles(dir).foreach(ThreadingAnnotationInstrumenter.instrument) + dir + } + + /** Reads the invocations recorded by the stub `ThreadingAssertions` in the given fixture classloader. */ + private def recordedCalls(loader: ClassLoader): Seq[String] = + loader.loadClass("com.intellij.util.concurrency.ThreadingAssertions") + .getField("calls").get(null).asInstanceOf[java.util.List[String]].asScala.toList +} From 443f2656727929482996d755ef9c8507286544ce Mon Sep 17 00:00:00 2001 From: Vasil Vasilev Date: Fri, 31 Jul 2026 12:12:37 +0200 Subject: [PATCH 2/3] Generate line numbers for injected threading assertions Byte-level comparison of the Scala Plugin built by sbt against the same plugin built by IntelliJ IDEA's JPS showed that JPS adds a LineNumberTable entry covering each injected ThreadingAssertions call, while this plugin did not (the only remaining difference in threading-instrumented classes). The standalone JPS builder (TMHInstrumentingBuilder) defaults "tmh.generate.line.numbers" to false, which generateLineNumbers = false mirrored. However, IDE-driven builds behave differently: the DevKit plugin (TMHBuildProcessParametersProvider) injects -Dtmh.generate.line.numbers=true into every JPS build process it spawns, because the corresponding registry key defaults to true (intellij.devkit.core.xml). In practice, plugin projects built by IntelliJ IDEA are always instrumented with line numbers. Passing generateLineNumbers = true therefore matches IDEA's real output: the injected assertion is attributed to the first line number of the method, producing better stack traces when the assertion throws. Co-Authored-By: Claude Fable 5 --- .../instrumentation/ThreadingAnnotationInstrumenter.scala | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ideaSupport/src/main/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenter.scala b/ideaSupport/src/main/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenter.scala index 6ca0e55f..43c35a8b 100644 --- a/ideaSupport/src/main/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenter.scala +++ b/ideaSupport/src/main/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenter.scala @@ -20,7 +20,12 @@ private object ThreadingAnnotationInstrumenter { // The generated assertion is a zero-argument static ()V call at method entry, // which changes neither stack map frames nor the maximum stack size. val writer = new ClassWriter(reader, 0) - if (TMHInstrumenter.instrument(reader, writer, Generators, /*generateLineNumbers =*/ false)) { + // Line number generation matches IDE-driven JPS builds. While the standalone JPS builder defaults + // "tmh.generate.line.numbers" to false, the DevKit plugin injects -Dtmh.generate.line.numbers=true into every + // build process it spawns (the registry key defaults to true in intellij.devkit.core.xml). The generated + // assertion is annotated with the line number of the start of the method, producing better stack traces when + // the assertion throws. + if (TMHInstrumenter.instrument(reader, writer, Generators, /*generateLineNumbers =*/ true)) { Files.write(classFile, writer.toByteArray) } } From fb26cbb42fafa9798944c3778376ff1b9d9fe520 Mon Sep 17 00:00:00 2001 From: Vasil Vasilev Date: Fri, 31 Jul 2026 14:35:33 +0200 Subject: [PATCH 3/3] Test that generated assertions carry the method's first line number Pins generateLineNumbers = true in ThreadingAnnotationInstrumenter, which could otherwise look like a safe "simplification" back to false to a reader who only checks the standalone JPS builder default. The test asserts, on the ASM event stream of the instrumented method, that the injected ThreadingAssertions call is preceded by a line number entry carrying the first line number of the original method, matching JPS builds spawned by IntelliJ IDEA. Verified to fail if generateLineNumbers is flipped back to false. Co-Authored-By: Claude Fable 5 --- .../ThreadingAnnotationInstrumenterTest.scala | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenterTest.scala b/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenterTest.scala index cd8534fa..4338d325 100644 --- a/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenterTest.scala +++ b/ideaSupport/src/test/scala/org/jetbrains/sbtidea/instrumentation/ThreadingAnnotationInstrumenterTest.scala @@ -1,10 +1,12 @@ package org.jetbrains.sbtidea.instrumentation +import org.objectweb.asm.{ClassReader, ClassVisitor, Label, MethodVisitor, Opcodes} import org.scalatest.funsuite.AnyFunSuite import org.scalatest.matchers.should.Matchers import java.nio.file.{Files, Path} import scala.collection.JavaConverters.asScalaBufferConverter +import scala.collection.mutable /** * Tests the threading annotation instrumentation against stub versions of the IntelliJ Platform @@ -92,6 +94,41 @@ class ThreadingAnnotationInstrumenterTest extends AnyFunSuite with Matchers with assert(java.util.Arrays.equals(before, after), "class file must not be modified") } + test("the generated assertion is covered by a line number entry for the first line of the method") { + val dir = compileThreadingFixture( + "LineNumbers.java" -> + """import com.intellij.util.concurrency.ThreadingAssertions; + |import com.intellij.util.concurrency.annotations.RequiresEdt; + |public class LineNumbers { + | @RequiresEdt + | public static void test() { + | ThreadingAssertions.calls.add("body"); + | } + |}""".stripMargin + ) + val classFile = dir.resolve("LineNumbers.class") + + val originalEvents = methodCodeEvents(Files.readAllBytes(classFile), "test") + val firstLineOfMethod = originalEvents.collectFirst { case Line(number) => number } + .getOrElse(fail("the fixture must be compiled with line numbers")) + + ThreadingAnnotationInstrumenter.instrument(classFile) + + // The injected call must be covered by a line number entry pointing at the first line of the method, + // like in JPS builds spawned by IntelliJ IDEA (which pass -Dtmh.generate.line.numbers=true). + // Without that entry, the assertion call would precede the first line number of the method. + val instrumentedEvents = methodCodeEvents(Files.readAllBytes(classFile), "test") + instrumentedEvents.take(2) shouldBe Seq( + Line(firstLineOfMethod), + Invoke("com/intellij/util/concurrency/ThreadingAssertions", "assertEventDispatchThread") + ) + + // The rewritten class must still pass the JVM bytecode verifier and run. + val loader = classLoader(dir) + loader.loadClass("LineNumbers").getMethod("test").invoke(null) + recordedCalls(loader) shouldBe Seq("assertEventDispatchThread", "body") + } + test("a method with two threading annotations gets only one assertion (matching IntelliJ's instrumenter)") { val dir = compileAndInstrument( "DoubleAnnotated.java" -> @@ -147,4 +184,24 @@ class ThreadingAnnotationInstrumenterTest extends AnyFunSuite with Matchers with private def recordedCalls(loader: ClassLoader): Seq[String] = loader.loadClass("com.intellij.util.concurrency.ThreadingAssertions") .getField("calls").get(null).asInstanceOf[java.util.List[String]].asScala.toList + + private sealed trait CodeEvent + private case class Line(number: Int) extends CodeEvent + private case class Invoke(owner: String, name: String) extends CodeEvent + + /** Extracts line number entries and method invocations of a method's code, in bytecode order. */ + private def methodCodeEvents(classBytes: Array[Byte], methodName: String): Seq[CodeEvent] = { + val events = mutable.Buffer.empty[CodeEvent] + new ClassReader(classBytes).accept(new ClassVisitor(Opcodes.ASM9) { + override def visitMethod(access: Int, name: String, descriptor: String, signature: String, exceptions: Array[String]): MethodVisitor = + if (name != methodName) null + else new MethodVisitor(Opcodes.ASM9) { + override def visitLineNumber(line: Int, start: Label): Unit = + events += Line(line) + override def visitMethodInsn(opcode: Int, owner: String, name: String, descriptor: String, isInterface: Boolean): Unit = + events += Invoke(owner, name) + } + }, 0) + events.toList + } }