Replace the Scala threading instrumenter with IntelliJ's TMH Java sources - #153
Merged
Merged
Conversation
…rces 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR replaces the hand-written Scala port of the threading annotation instrumentation
(#SCL-18660) with the original Java sources from IntelliJ IDEA's JPS build, copied verbatim —
the same approach already used for the
@NotNullinstrumentation (#SCL-22085).The Scala port (Feb 2, 2024) had drifted from upstream: three days after it was written,
IntelliJ's JPS switched to generating
ThreadingAssertionscalls(IJPL-255) and chose
softAssertReadAccessfor
@RequiresReadLock, while our port generates the hardassertReadAccess. Copying theupstream implementation removes this drift and the maintenance burden of keeping a parallel
port in sync.
Matching IntelliJ IDEA's own instrumentation,
@RequiresReadLockviolations now log anerror instead of throwing an exception (
ThreadingAssertions.softAssertReadAccess). Thismay change what CI reports for read-lock issues in projects using
instrumentThreadingAnnotations := true. All other annotations still throw as before.Implementation
TMHInstrumenter,TMHAssertionGenerator, andTMHAssertionGenerator2are copiedverbatim from IntelliJ IDEA Community Edition (Apache 2.0, commit
7a35a7d7fe64— thesame commit the
notNullVerificationsources were copied from). The only changes are thepackage name, the ASM package,
Opcodes.API_VERSION→Opcodes.ASM9, and theFailSafeMethodVisitorimport (reusing the copy we already have).TMHAssertionGenerator1(the legacy fallback for SDKs withoutThreadingAssertions) isnot copied — the previous implementation already required
ThreadingAssertions, sonothing regresses.
ThreadingAnnotationInstrumenter.scalashrinks from ~106 lines of ASM visitor logic to athin wrapper around
TMHInstrumenter. Same object name and signature, soManipulateBytecodeand theinstrumentThreadingAnnotationssetting are unchanged.org.jetbrains:annotationsmoves from Test to compile scope, since the copied sources use@ApiStatus.Internal/@NotNull.generateAssertion = falseno longertrigger a pointless no-op rewrite of their class file.
Tests
The fixture harness from
NotNullInstrumenterTestis extracted into a sharedInstrumenterTestHarnesstrait, and a newThreadingAnnotationInstrumenterTest(5 tests)compiles stub
com.intellij.util.concurrencyclasses at test runtime whose assertionmethods record their invocations. It pins the annotation → assertion mapping (including
@RequiresReadLock→softAssertReadAccess), assertion-before-body ordering, thegenerateAssertion = falseopt-out, no-op behavior on unannotated classes, and theone-assertion-per-method behavior for doubly-annotated methods (which matches upstream's
last-annotation-wins semantics).