Repository navigation
Lombok & VSCode - #3082
Lombok & VSCode#3082
Conversation
| "zjsonPatchCompileOnly"(libs.zjsonpatch) | ||
| // lombokStubs – needs ECJ types to match the exact method descriptors that lombok | ||
| // transplants into ECJ's ASTConverter, ASTNode, etc. | ||
| "lombokStubsCompileOnly"("org.eclipse.jdt:org.eclipse.jdt.core:3.46.0") |
There was a problem hiding this comment.
Moving forward, how would you like us to maintain this 3.46.0 version? Should we bump it to latest whenever a new version is released? Should we pin it here until/unless you open a PR that bumps it?
There was a problem hiding this comment.
Hi @nedtwigg Thanks for taking a look at this.
So I've looked at the history of the Eclipse code that is imported. It dates from >= 15 years ago (for ASTNode) (and similarly for IMember) , while ASTVisitor was created ~ early 2004 and has had additions (and some tiny removals for String Templates) since then for JDK versions - but as its used a parameter only this shouldn't affect it. So therefore I think this should be pinned and only reconsidered if there is perhaps a CVE or a fix needed.
Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.
Please make sure that your PR allows edits from maintainers. Sometimes it's faster for us to just fix something than it is to describe how to fix it.
After creating the PR, please add a commit that adds a bullet-point under the
[Unreleased]section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:If your change only affects a build plugin, and not the lib, then you only need to update the
plugin-foo/CHANGES.mdfor that plugin.If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update
CHANGES.mdfor both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.This makes it easier for the maintainers to quickly release your changes :)
@nedtwigg Unfortunately after trying on other repositories I've found further issues with Lombok & VSCode (when working with spotless) after #3038.
I am not sure whether you will accept this as it adds a dependency - but only for
compileOnlyscope, not runtime.Summary of changes:
While 3038 started the foundational changes, there were still two categories of failures remaining:
1.
NoSuchMethodErroronPatchFixesHider$PatchFixesmethods with concrete ECJ typesLombok transplants call sites into ECJ bytecode with exact descriptors referencing concrete ECJ types — e.g.
invokestatic PatchFixesHider$PatchFixes.isGenerated(Lorg/eclipse/jdt/internal/compiler/ast/ASTNode;)Z. PR #3038's stub declaredisGenerated(Object), which the JVM rejects at link time withNoSuchMethodErrorbecause the descriptor doesn't match. Similarly, several otherPatchFixesmethods (getSourceEndFixed,getRealNodeSource,isBlockedVisitorAndGenerated) referenced ECJ types that were missing, andLombokDepshad no method bodies at all.2.
NoClassDefFoundErrorforeclipse/agent/portal classesECJ's patched methods reference
PatchDiagnostics,PatchValEclipsePortal,PatchDelegatePortal,PatchFixesShadowLoaded,PatchJavadoc, andEclipseLoaderPatcherTransplantsdirectly by method call.synthesiseEmptyClasscan satisfy class loading but not method lookup — when ECJ callsPatchDiagnostics.setSourceRangeCheck(...)the JVM finds the synthesised empty class has no such method and throwsNoSuchMethodError.Changes in this PR
lib/build.gradle.kts(build file migrated to Kotlin DSL since #3038)"lombokStubsCompileOnly"("org.eclipse.jdt:org.eclipse.jdt.core:3.46.0")— needed soPatchFixesHider.javacan compile against the exact ECJ types whose descriptors lombok transplants into ECJ bytecode. This iscompileOnly; it adds zero runtime dependency tospotless-lib.lib/src/lombokStubs/java/lombok/launch/PatchFixesHider.java— rewrittenPatchFixes.isGeneratedsplit into three overloads with concrete ECJ types:(org.eclipse.jdt.core.dom.ASTNode),(org.eclipse.jdt.internal.compiler.ast.ASTNode),(org.eclipse.jdt.core.IMember)— matching the exactinvokestaticdescriptors lombok transplantsPatchFixes.isBlockedVisitorAndGeneratedupdated to use concreteASTNode/ASTVisitortypesPatchFixes.getSourceEndFixedandgetRealNodeSourceupdated to use concrete ECJASTNodetypesPatchFixesmethods:fixRetrieveIdentifierEndPosition,fixRetrieveEllipsisStartPosition,fixRetrieveStartBlockPositionLombokDepsbody filled in:addLombokNotesToEclipseAboutDialog,runPostCompiler(×3 overloads)Transformmethods declaredthrows IOExceptionto match transplanted descriptors6 new stub source files for
eclipse/agent/portal classes — these need real method bodies (notsynthesiseEmptyClass) because ECJ's patched bytecode calls their methods directly:setSourceRangeCheck(Object,int,int) → falsecopyInitializationOf…(no-ops)handleDelegateForType → false,addGeneratedDelegateMethods → []addLombokNotes… → original,runPostCompiler(×3)getHTMLContentFromSource → originaloverrideLoadDecide → false,overrideLoadResult → nullFeatureClassLoaderLombokStubsTest.java— expanded from 11 to 17 testspatchFixesHider_patchFixesCanBeLoaded→ replaced bypatchFixesHider_patchFixes_isGeneratedOverloadsExist, which parses the compiled.classfile's constant pool directly (hand-written JVMS §4.4 byte parser, no external libraries) to verify the three concrete ECJ method descriptors are present — without triggering reflective type resolution of ECJ classes absent from the test classpatheclipse/agent/stubs, each verifying the method exists, is static, and returns the correct sentinel valueunknownLombokClass_*tests updated to useEclipsePatcher(which genuinely has no stub) rather thanPatchDiagnostics(which now has one)