Skip to content

Preserve Flag.Varargs in bytecode/reflection type mappers#7988

Open
knutwannheden wants to merge 1 commit into
mainfrom
investigate-dropped-flag.varargs-in-type-attribution
Open

Preserve Flag.Varargs in bytecode/reflection type mappers#7988
knutwannheden wants to merge 1 commit into
mainfrom
investigate-dropped-flag.varargs-in-type-attribution

Conversation

@knutwannheden

Copy link
Copy Markdown
Contributor

Motivation

Flag.Varargs was silently dropped from JavaType.Method for varargs methods attributed through some (non-javac) paths, because of a bit collision:

  • The JVM/bytecode ACC_VARARGS access flag — also the bit returned by java.lang.reflect.Member#getModifiers() and ASM — occupies bit 0x0080.
  • Flag assigns that same bit (1L << 7) to Transient. Flag.Varargs is instead 1L << 34, matching javac's com.sun.tools.javac.code.Flags.VARARGS.

Any type mapper that fed raw JVM/reflection access flags into JavaType.Method's flags bitmap therefore produced a spurious Transient and lost Varargs. GroovyTypeMapping (MethodNode.getModifiers()) and JavaReflectionTypeMapping (Member.getModifiers()) both did this. KotlinTypeMapping already compensated for the same collision (KotlinTypeMapping.kt, search for the 1L shl 7 / 1L shl 34 remap), confirming the convention.

This is a broad-impact attribution gap: anything that branches on Flag.Varargs or reasons about varargs (overload resolution, method matching, templating, etc.) can misbehave on these method types. It was the upstream cause of an ArrayIndexOutOfBoundsException in UseDiamondOperator.getMethodParamType (rewrite-static-analysis) seen in a flagship recipe run — that recipe maps invocation arguments by index and is only reachable when a call has more arguments than the method's reported parameter types, which in valid Java only happens for a varargs method whose Flag.Varargs was missing. (A defensive guard for the recipe itself is handled separately in rewrite-static-analysis.)

Scope / where the flag is and isn't dropped

I exercised each attribution path:

Path Flag.Varargs preserved before this PR?
OpenRewrite Java parser (javac) ✅ (javac uses 1L << 34 natively)
TypeTable jar round-trip (writeJar → javac) ✅ (added a regression test)
KotlinTypeMapping ✅ (already remapped)
GroovyTypeMapping ❌ fixed here
JavaReflectionTypeMapping ❌ fixed here

Note: the in-memory TypeTableSink consumer that builds JavaType directly from bytecode (used for fast classpath type loading) lives outside this repo and receives the same raw ASM access int; it needs the equivalent remap and is tracked separately.

Summary

  • Add Flag.mapBytecodeAccessFlagsToBitMap(long) — remaps ACC_VARARGS (0x0080) to Flag.Varargs for method/constructor access flags; leaves everything else (and field flags, where 0x0080 legitimately means transient) untouched.
  • Route GroovyTypeMapping#methodType and JavaReflectionTypeMapping's method and constructor mapping through the helper.

Test plan

  • FlagTest — unit-tests the helper (remaps varargs, leaves non-varargs flags identical).
  • JavaReflectionTypeMappingTest#varargsMethodHasVarargsFlag — maps Arrays.asList(Object...) reflectively; asserts Varargs present and Transient absent.
  • GroovyVarargsTypeMappingTest#varargsMethodHasVarargsFlag — parses Groovy calling Arrays.asList; asserts the resolved method type has Varargs and not Transient.
  • TypeTableTest#varargsFlagPreservedThroughTypeTableRoundtrip — positive regression guard proving the TypeTable jar path already preserves the flag.
  • ./gradlew :rewrite-java:test :rewrite-groovy:test — full suites pass.

The bytecode `ACC_VARARGS` access flag occupies bit `0x0080`, which is the
same bit `Flag.Transient` uses. `Flag.Varargs`, however, is modeled as
`1L << 34` (matching javac's `Flags.VARARGS`). Type mappers that fed raw
JVM/reflection access flags into `JavaType.Method` therefore produced a
spurious `Transient` flag and dropped `Varargs`.

`GroovyTypeMapping` (`MethodNode.getModifiers()`) and
`JavaReflectionTypeMapping` (`Member.getModifiers()`) both did this;
`KotlinTypeMapping` already compensated. Add a shared
`Flag.mapBytecodeAccessFlagsToBitMap` helper that remaps `ACC_VARARGS` to
`Varargs` and route both mappers through it.

Methods missing `Flag.Varargs` cause anything that reasons about varargs to
misbehave; e.g. UseDiamondOperator indexed past the end of
`getParameterTypes()` (ArrayIndexOutOfBoundsException) on such method types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant