Skip to content

Use synthetic functions of kotlin jvm inline class #4066

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Use synthetic functions of kotlin jvm inline class #4066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0e53942
Use kotlin jvm inline class synthetic constructors
wplong11 Aug 4, 2023
02f3fd2
Use box-impl function as factory for kotlin jvm inline class
wplong11 Aug 4, 2023
1dbce7b
Merge branch '2.16' into revert-1005
cowtowncoder Aug 26, 2023
c1d08c2
Merge branch '2.16' into revert-1005
cowtowncoder Sep 2, 2023
ff989c4
Add more comments
cowtowncoder Sep 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/main/java/com/fasterxml/jackson/databind/ext/KotlinSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package com.fasterxml.jackson.databind.ext;

import java.lang.reflect.Constructor;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;

public class KotlinSupport {
public static boolean isJvmInlineClassSyntheticConstructor(Constructor<?> ctor) {
Class<?>[] params = ctor.getParameterTypes();
if (params.length == 0) {
return false;
}

Class<?> lastParam = params[params.length - 1];
return ctor.isSynthetic() && lastParam.getName().equals("kotlin.jvm.internal.DefaultConstructorMarker");
}
Comment on lines +8 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

This code also covers constructors that do not actually contain value class as an argument.

The following is a class definition in Kotlin and its constructors taken from the decompiled result.
You can see that the value class is not used, but the argument of the synthetic constructor ends with DefaultConstructorMarker.

data class Data(val v: Int = 0)
// decompiled

   public Data(int v) {
      this.v = v;
   }

   // $FF: synthetic method
   public Data(int var1, int var2, DefaultConstructorMarker var3) {
      if ((var2 & 1) != 0) {
         var1 = 0;
      }

      this(var1);
   }

I can't think of any other way to write it myself, but I think it should at least be supplemented with a comment.


public static boolean isJvmInlineClassSyntheticBoxingFunction(Method method) {
return Modifier.isStatic(method.getModifiers())
&& method.isSynthetic()
&& method.getName().equals("box-impl");
}
Comment on lines +18 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

As commented here, using only box-impl to deserialize value class may cause unexpected bugs.
#4066 (comment)

Therefore, this change should be removed.

}
11 changes: 8 additions & 3 deletions src/main/java/com/fasterxml/jackson/databind/introspect/AnnotatedCreatorCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import com.fasterxml.jackson.databind.AnnotationIntrospector;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.ext.KotlinSupport;
import com.fasterxml.jackson.databind.introspect.AnnotatedClass.Creators;
import com.fasterxml.jackson.databind.type.TypeFactory;
import com.fasterxml.jackson.databind.util.ClassUtil;
Expand Down Expand Up @@ -282,7 +283,9 @@ private static boolean _isIncludableFactoryMethod(Method m)
}
// 09-Nov-2020, ckozak: Avoid considering synthetic methods such as
// lambdas used within methods because they're not relevant.
return !m.isSynthetic();
return !m.isSynthetic()
// 02-Sep-2023: As per [databind#4066] Kotlin needs some synthetic factory methods
|| KotlinSupport.isJvmInlineClassSyntheticBoxingFunction(m);
}

protected AnnotatedConstructor constructDefaultConstructor(ClassUtil.Ctor ctor,
Expand Down Expand Up @@ -406,8 +409,10 @@ private final AnnotationMap collectAnnotations(AnnotatedElement main, AnnotatedE
return c.asAnnotationMap();
}

// for [databind#1005]: do not use or expose synthetic constructors
private static boolean isIncludableConstructor(Constructor<?> c) {
Copy link
Member

Choose a reason for hiding this comment

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

/cc

Copy link
Member

Choose a reason for hiding this comment

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

I don't think current AnnotatedXxxCollectors are optimal extension points, so suggested changes here are probably best we can do on short term. Before bigger rewrite,

Copy link
Member

Choose a reason for hiding this comment

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

so suggested changes here are probably best we can do on short term. Before bigger rewrite,

Agreed 👍🏻

return !c.isSynthetic();
// for [databind#1005]: do not use or expose synthetic constructors
return !c.isSynthetic()
// 02-Sep-2023: As per [databind#4066] Kotlin needs some synthetic constructors
|| KotlinSupport.isJvmInlineClassSyntheticConstructor(c);
}
}