https://kotlinlang.org logo
Join Slack
Powered by
Found what seems like a pretty significant bug for...
# ksp
z
Found what seems like a pretty significant bug for processors handling serialization of any sort - jvm modifiers from externally-compiled classes are not visible to subclasses in the current compilation https://github.com/google/ksp/issues/710
FYI @yigit since I imagine this affects Room too
y
likely. we've had a couple of similar bugs. at some cases, room goes and parses the class file (for order of elements) https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:room/roo[…]ing/ksp/KspClassFileUtility.kt;l=39?q=KspClassFileUtility&sq=
z
oof
I don't think we want to do that in moshi vs just waiting for a fix
j
I tried to compile 
BaseClass
with kotlinc and found no transient information with javap, compared with a java class with transient field. Need to check compiler if it is supported. compiled from Java.
Copy code
Classfile /Users/jiaxiang/code/tmp/A.class
  Last modified Nov 3, 2021; size 211 bytes
  MD5 checksum b79bd6ca07077f984016d38c860a7b23
  Compiled from "A.java"
public class A
  minor version: 0
  major version: 55
  flags: (0x0021) ACC_PUBLIC, ACC_SUPER
  this_class: #3                          // A
  super_class: #4                         // java/lang/Object
  interfaces: 0, fields: 1, methods: 1, attributes: 1
Constant pool:
   #1 = Methodref          #4.#13         // java/lang/Object."<init>":()V
   #2 = Fieldref           #3.#14         // A.k:I
   #3 = Class              #15            // A
   #4 = Class              #16            // java/lang/Object
   #5 = Utf8               k
   #6 = Utf8               I
   #7 = Utf8               <init>
   #8 = Utf8               ()V
   #9 = Utf8               Code
  #10 = Utf8               LineNumberTable
  #11 = Utf8               SourceFile
  #12 = Utf8               A.java
  #13 = NameAndType        #7:#8          // "<init>":()V
  #14 = NameAndType        #5:#6          // k:I
  #15 = Utf8               A
  #16 = Utf8               java/lang/Object
{
  transient int k;
    descriptor: I
    flags: (0x0080) ACC_TRANSIENT

  public A();
    descriptor: ()V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=2, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method java/lang/Object."<init>":()V
         4: aload_0
         5: iconst_1
         6: putfield      #2                  // Field k:I
         9: return
      LineNumberTable:
        line 1: 0
        line 2: 4
}
SourceFile: "A.java"
compiled from Kotlin
Copy code
Classfile /Users/jiaxiang/code/tmp/BaseClass.class
  Last modified Nov 3, 2021; size 717 bytes
  MD5 checksum 056d382f7986c813aed1d76805a9c8be
  Compiled from "tmp.kt"
public abstract class BaseClass
  minor version: 0
  major version: 52
  flags: (0x0421) ACC_PUBLIC, ACC_SUPER, ACC_ABSTRACT
  this_class: #2                          // BaseClass
  super_class: #4                         // java/lang/Object
  interfaces: 0, fields: 1, methods: 3, attributes: 2
Constant pool:
   #1 = Utf8               BaseClass
   #2 = Class              #1             // BaseClass
   #3 = Utf8               java/lang/Object
   #4 = Class              #3             // java/lang/Object
   #5 = Utf8               <init>
   #6 = Utf8               ()V
   #7 = NameAndType        #5:#6          // "<init>":()V
   #8 = Methodref          #4.#7          // java/lang/Object."<init>":()V
   #9 = Utf8               lastSet
  #10 = Utf8               I
  #11 = NameAndType        #9:#10         // lastSet:I
  #12 = Fieldref           #2.#11         // BaseClass.lastSet:I
  #13 = Utf8               this
  #14 = Utf8               LBaseClass;
  #15 = Utf8               getLastSet
  #16 = Utf8               ()I
  #17 = Utf8               setLastSet
  #18 = Utf8               (I)V
  #19 = Utf8               <set-?>
  #20 = Utf8               Lkotlin/Metadata;
  #21 = Utf8               mv
  #22 = Integer            1
  #23 = Integer            6
  #24 = Integer            0
  #25 = Utf8               k
  #26 = Utf8               xi
  #27 = Integer            48
  #28 = Utf8               d1
  #29 = Utf8               \u0000\u0014\n\u0002\u0018\u0002\n\u0002\u0010\u0000\n\u0002\b\u0002\n\u0002\u0010\b\n\u0002\b\u0004\b&\u0018\u00002\u00020\u0001B\u0005¢\u0006\u0002\u0010\u0002R\u001a\u0010\u0003\u001a\u00020\u0004X\u0096\u000e¢\u0006\u000e\n\u0000\u001a\u0004\b\u0005\u0010\u0006\"\u0004\b\u0007\u0010\b
  #30 = Utf8               d2
  #31 = Utf8
  #32 = Utf8               tmp.kt
  #33 = Utf8               Code
  #34 = Utf8               LineNumberTable
  #35 = Utf8               LocalVariableTable
  #36 = Utf8               SourceFile
  #37 = Utf8               RuntimeVisibleAnnotations
{
  public BaseClass();
    descriptor: ()V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=2, locals=1, args_size=1
         0: aload_0
         1: invokespecial #8                  // Method java/lang/Object."<init>":()V
         4: aload_0
         5: iconst_1
         6: putfield      #12                 // Field lastSet:I
         9: return
      LineNumberTable:
        line 1: 0
        line 2: 4
        line 1: 9
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      10     0  this   LBaseClass;

  public int getLastSet();
    descriptor: ()I
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: getfield      #12                 // Field lastSet:I
         4: ireturn
      LineNumberTable:
        line 2: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   LBaseClass;

  public void setLastSet(int);
    descriptor: (I)V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=2, locals=2, args_size=2
         0: aload_0
         1: iload_1
         2: putfield      #12                 // Field lastSet:I
         5: return
      LineNumberTable:
        line 2: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       6     0  this   LBaseClass;
            0       6     1 <set-?>   I
}
SourceFile: "tmp.kt"
RuntimeVisibleAnnotations:
  0: #20(#21=[I#22,I#23,I#24],#25=I#22,#26=I#27,#28=[s#29],#30=[s#14,s#31,s#6,s#9,s#31,s#15,s#16,s#17,s#18])
    kotlin.Metadata(
      mv=[1,6,0]
      k=1
      xi=48
      d1=["\u0000\u0014\n\u0002\u0018\u0002\n\u0002\u0010\u0000\n\u0002\b\u0002\n\u0002\u0010\b\n\u0002\b\u0004\b&\u0018\u00002\u00020\u0001B\u0005¢\u0006\u0002\u0010\u0002R\u001a\u0010\u0003\u001a\u00020\u0004X\u0096\u000e¢\u0006\u000e\n\u0000\u001a\u0004\b\u0005\u0010\u0006\"\u0004\b\u0007\u0010\b"]
      d2=["LBaseClass;","","()V","lastSet","","getLastSet","()I","setLastSet","(I)V"]
    )
z
it must be, we're able to see this in kapt/java apt
t
re: bytecode dump: the backing field is private. did you pass 
-p
to 
javap
? @Jiaxiang
Unfortunately Kotlin compiler didn't leave any clue about this in metadata, and therefore the only solution is parsing the class file. Because 
transient
and 
volatile
is JVM specific and don't really appear in Kotlin code, and also because of performance consideration, adding them to modifiers seems infeasible*1. Instead, the functionality can be provided by 
Resolver.isJvmTransient(prop: KSPropertyDeclaration)
and 
Resolver.isJvmVolatile(prop: KSPropertyDeclaration)
. In that way you also won't need to check annotations for Kotlin sources. Please let me know what you think. *1 for Kotlin libraries. Java classes are already parsed and the information should be handily available.
z
would it maybe be simpler to do something like 
Resolver.jvmModifiers(prop: KSPropertyDeclaration): Set<Modifier>
?
I also wonder if functions need something like this for synchronized/strictfp
t
How about 
Resolver.jvmAccessFlags
? I'm worried that 
Resolver.jvmModifiers
would look too similar to the existing KSModifierListOwner.modifiers and be confusing. Making it explicit that it is jvm implementation specific probably has another benefit. IMHO, modifiers are more of a source / language level thing than library/implementation. Sometimes it's unclear what to do for compiled classes. For example (besides transient and volatile), Kotlin classes are public by default and we debated long long ago whether to synthesize it.
z
hmm, they're not really access-specific though. We call them jvm modifiers in kotlinpoet fwiw
by the way - is it possible to access the underlying descriptor like you'd do in that function? Wondering if we can work around this for now in Moshi
t
It is possible by a lot of casting to implementations in 
symbol-processing
and 
kotlin-compiler-embeddable
to read the class file as a stream, and parsing the stream with another library. I won't recommend it though if you can wait for a couple of weeks until the new API is implemented and released.
z
gotcha. Unfortunately not sure if we can wait 😕. We could probably do a quick patch release after it's out though if it's not an experimental API
t
btw, one of the drawback of depending on the implementation is that processor could be broken with future releases of ksp and compiler. If you really need to do it, please access the implementation as few as possible.
z
yup we're not going to cast to impl versions in moshi at least, too much of a risk
t
re: API: I would like to avoid "modifier" in the name of the new API. It is confusing in many cases. I'm going to implement 
isJvmVolatile
, 
isJvmTransient
, etc for now but please feel free to advise.
z
your call! Just feels like one function is better than two
I'd also expect this to come via the existing JAVA_TRANSIENT and JAVA_VOLATILE modifiers
which is why I've been sticking to that nomenclature
t
Yeah, I agree with the simplicity. We can change the API anytime before next release (or add better ones after the release); The implementations are not too different. For compiled Java code, they will have those modifiers in place. Only compiled Kotlin code need the new API.
👍 1
8 Views
Open in Slack
PreviousNext
Powered by