Add compiler plugin for SemanticDB #91
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a Scala 3 compiler plugin that enhances SemanticDB output with bundle field source information, enabling IDE features like go-to-definition and find-references for dynamically-accessed bundle fields via the Dynamic trait. The implementation requires upgrading to Scala 3.6.4-RC1-NIGHTLY to access the ResearchPlugin API.
Key changes:
- New compiler plugin with two phases: BundleFieldCollectorPhase (collects bundle field definitions) and SemanticDBEnhancerPhase (injects symbols and rewrites occurrences in SemanticDB)
- Thread-safe BundleFieldRegistry for cross-phase communication using TrieMap
- Upgrade to Scala 3.6.4-RC1-NIGHTLY with custom repository configuration for NIGHTLY dependencies
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| build.mill | Adds NIGHTLY repository support, upgrades Scala version to 3.6.4-RC1-NIGHTLY, and defines CustomJvmWorkerModule |
| zaozi-compiler-plugin/package.mill | Defines compiler plugin module with scala3-compiler dependency, test configuration with plugin enabled, and SemanticDB settings |
| zaozi-compiler-plugin/resources/plugin.properties | Specifies plugin entry point class for compiler plugin loading |
| zaozi-compiler-plugin/src/me/jiuyang/zaozi/plugin/ZaoziSemanticDBPlugin.scala | Main plugin class extending ResearchPlugin, inserts custom phases into compiler pipeline |
| zaozi-compiler-plugin/src/me/jiuyang/zaozi/plugin/BundleFieldCollectorPhase.scala | Compiler phase that traverses AST to collect bundle field definitions and usages |
| zaozi-compiler-plugin/src/me/jiuyang/zaozi/plugin/SemanticDBEnhancerPhase.scala | Compiler phase that modifies SemanticDB files to replace Dynamic occurrences with bundle field references |
| zaozi-compiler-plugin/src/me/jiuyang/zaozi/plugin/BundleFieldRegistry.scala | Thread-safe storage for bundle field definitions and usages using TrieMap |
| zaozi-compiler-plugin/tests/src/PluginSpec.scala | Test suite verifying plugin functionality with bundle field access patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Custom JvmWorkerModule for Mill bootstrap with NIGHTLY Scala | ||
| // See: https://docs.scala-lang.org/overviews/core/nightlies.html | ||
| object CustomJvmWorkerModule extends JvmWorkerModule with CoursierModule { | ||
| override def repositories = super.repositories() ++ Seq( | ||
| "https://repo.scala-lang.org/artifactory/maven-nightlies" | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
The CustomJvmWorkerModule is defined but never used in the code. If it's intended to be used by Mill's build infrastructure, this should be documented. Otherwise, if it's not needed, it should be removed to avoid confusion.
| // Custom JvmWorkerModule for Mill bootstrap with NIGHTLY Scala | |
| // See: https://docs.scala-lang.org/overviews/core/nightlies.html | |
| object CustomJvmWorkerModule extends JvmWorkerModule with CoursierModule { | |
| override def repositories = super.repositories() ++ Seq( | |
| "https://repo.scala-lang.org/artifactory/maven-nightlies" | |
| ) | |
| } |
| // Just verify it compiles - the actual tracking happens at compile time | ||
| assert(BundleFieldAccessTest != null) |
There was a problem hiding this comment.
The test only checks that the object is not null, which is a trivial assertion since object definitions in Scala cannot be null. This test doesn't actually verify that the plugin correctly tracks field accesses. Consider asserting on the actual collected data from BundleFieldRegistry or verifying the generated SemanticDB output.
| fieldToBundleMap.get(fieldName) match | ||
| case Some(definitions) if definitions.nonEmpty => | ||
| // Use the first matching definition (could be improved with type context) | ||
| val defn = definitions.head | ||
| val symbol = createSymbolString(defn.bundleSymbol, fieldName) |
There was a problem hiding this comment.
When multiple bundle field definitions exist with the same name, only the first one is used (definitions.head). This could lead to incorrect go-to-definition behavior if there are multiple bundles with fields of the same name. Consider using additional context (like the receiver type) to select the correct definition, or document this limitation.
| wire.flag := io.bundle.flag | ||
| io.out := wire.input | ||
|
|
||
| assert(WireWithBundleTest != null) |
There was a problem hiding this comment.
Similar to the previous tests, this only verifies that the object is not null. The test doesn't verify that the Wire fields are being tracked by the plugin. Consider adding assertions that check the BundleFieldRegistry for the expected field definitions and usages.
| } match | ||
| case Success(_) => () | ||
| case Failure(e) => | ||
| // Log error but don't fail compilation | ||
| () |
There was a problem hiding this comment.
The error handling silently suppresses exceptions without logging. While the comment indicates this is intentional to avoid failing compilation, it would be better to at least log warnings to help with debugging. Consider adding a debug or warning message when SemanticDB modification fails.
| /** Clear the registry (useful between compilation runs). */ | ||
| def clear(): Unit = | ||
| definitions.clear() | ||
| usages.clear() |
There was a problem hiding this comment.
The BundleFieldRegistry uses module-level mutable state that is never cleared between compilation runs. This could lead to stale data accumulating across multiple compilations in long-running processes (like build servers or IDEs). While a clear() method exists, it's never called. Consider clearing the registry at the start of each compilation run.
| test("BundleFieldRegistry API works") { | ||
| val defs = BundleFieldRegistry.getAllDefinitions | ||
| assert(defs != null) | ||
|
|
||
| val symbols = BundleFieldRegistry.getAllBundleSymbols | ||
| assert(symbols != null) |
There was a problem hiding this comment.
The test only verifies that the API methods return non-null values, but doesn't verify the actual content or correctness of the returned data. Since the plugin is supposed to collect bundle field definitions, the test should verify that the expected definitions (from TestBundle and OuterBundle) are actually present in the returned collections.
| io.nested.data.dontCare() | ||
| io.bundle.flag.dontCare() | ||
|
|
||
| assert(NestedBundleAccessTest != null) |
There was a problem hiding this comment.
Similar to the previous test, this only verifies that the object is not null. The test doesn't actually verify that nested field accesses are being tracked correctly by the plugin. Consider adding assertions that verify the BundleFieldRegistry contains the expected nested field usages.
…cement Add a Scala 3 compiler plugin that enhances SemanticDB output with bundle field source information, enabling IDE features like go-to-definition and find-references for dynamically-accessed bundle fields via Dynamic trait. The plugin introduces: - BundleFieldCollectorPhase: collects Aligned/Flipped field definitions - SemanticDBEnhancerPhase: injects symbols and rewrites occurrences - BundleFieldRegistry: thread-safe storage for cross-phase communication Also upgrades to Scala 3.6.4-RC1-NIGHTLY for ResearchPlugin API access. Signed-off-by: Jiuyang Liu <liu@jiuyang.me>
Add a Scala 3 compiler plugin that enhances SemanticDB output with bundle field source information, enabling IDE features like go-to-definition and find-references for dynamically-accessed bundle fields via Dynamic trait.
The plugin introduces:
Also upgrades to Scala 3.6.4-RC1-NIGHTLY for ResearchPlugin API access.