Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update ASM from 9.0 to 9.6.1 to use a recent version as IJ JPS #5390

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

develar
Copy link
Contributor

@develar develar commented Jan 2, 2025

No description provided.

@@ -121,7 +121,7 @@ public MethodVisitor newMethod(
@Override
@NotNull
public AnnotationVisitor newAnnotation(@NotNull String desc, boolean visible) {
return new AnnotationRemapper(builder.newAnnotation(remapper.mapDesc(desc), visible), remapper);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, the build fails because warnings are treated as errors. The old API is deprecated but still exists, preserving binary compatibility.

@udalov udalov self-assigned this Jan 6, 2025
@udalov
Copy link
Member

udalov commented Jan 6, 2025

It turns out that such change leads to many tests failing (see internal TC build).

Apparently, names of parameters from Java classes are now loaded properly by ASM, while in the previous version, they were not. I suspect that this happened because of a recent change in intellij fork of asm, related to IDEA-331588, but I'm still going to verify it. Looks like we use intellij's Cls PSI to parse class files in some cases, and now Cls PSI for Java methods contains parameter names, whereas previously it didn't.

Most likely we can just update test data with the new parameter names, because it does not affect compilation of Kotlin code (parameters of Java methods are considered "unstable" anyway, e.g. they can't be used with the named parameter syntax, etc). However, I'm a bit unsure if this is the intended effect of the change in intellij-deps-asm mentioned above, since it looks like an optimization that is not supposed to change behavior, so I'm still investigating.

@udalov
Copy link
Member

udalov commented Jan 7, 2025

Okay, I found out why it happened, basically we used mismatching versions of intellij and asm until now. I'll update test data and write more info in the commit message.

@develar
Copy link
Contributor Author

develar commented Jan 7, 2025

Thanks a lot. Mismatch of version when both JPS and kotlin compiler is in the same process the main reason why I want to unify used versions.

develar and others added 2 commits January 7, 2025 23:55
This is mostly a revert of the following commits:

1) 63ca8eb
2) eb32273

These commits were made to update intellij dependency (KTI-1416).
However, it seems that during that update, the asm dependency was not
updated. This led to a situation where the Kotlin compiler used intellij
with a fix for IDEA-331588:
JetBrains/intellij-community@638600f
But with an older ASM version that did not yet have
`ClassReader.VISIT_LOCAL_VARIABLES` and the corresponding code to handle
it.

Normally this would result in a NoSuchFieldError at runtime. However the
referenced field here is public static final, so it was inlined at the
call site, and everything worked as before, except the fact that
`ClsFileImpl` now passed `SKIP_CODE` (along with a value of
`VISIT_LOCAL_VARIABLES` = 256, which had no effect), which resulted in
parameter names of Java methods not being loaded.

Now that ASM is updated to a new version, `VISIT_LOCAL_VARIABLES`
started working as expected, and parameter names are loaded again.

Note that in most tests parameter names have been updated, but in some
diagnostic tests AA versions of tests have been muted, because behavior
differs between the compiler and AA modes (AA always uses compiled PSI).

 #KT-74276 Fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants