Use synthetic functions of kotlin jvm inline class #4066

wplong11 · 2023-08-03T15:40:13Z

Background

To support kotlin value class,
Synthetic Kotlin constructors should be available

revert: #1005

pjfanning · 2023-08-03T15:46:06Z

That code has been in jackson for years. Many users have used it knowingly or unknowingly. We can't just revert it because 1 user doesn't like it. No use cases have been provided. No meaningful description has been provided.

wplong11 · 2023-08-03T15:59:19Z

@pjfanning Below is the use case. Before discussing the detailed design, I created a PoC.
FasterXML/jackson-module-kotlin#693

wplong11 · 2023-08-03T16:04:12Z

@pjfanning

Since it's a Kotlin-related problem, I wonder if it's correct to support it in a Kotlin module...

Many users have used it knowingly or unknowingly

But what you said worries me too. By the way, can you reproduce the original issue? Deleting it now doesn't seem to be a problem. Can you help me reproduce the problem?

pjfanning · 2023-08-03T16:07:42Z

It would be better to get consensus among the jackson-module-kotlin maintainers (eg @k163377) before looking for a jackson-databind change.

It seems like this 1005 change is affecting FasterXML/jackson-module-kotlin#689 too.

k163377 · 2023-08-03T17:18:13Z

First, you can get a callable constructor without making this change.
Synthetic constructors have a pair of private, non-synthetic constructors that reside in a scope that can be parsed by Jackson.
The code is quite old, but the following may be helpful.
FasterXML/jackson-module-kotlin@2.16...k163377:jackson-module-kotlin:github-413/get_constructor_backup
FasterXML/jackson-module-kotlin#199 (comment)

This approach has the problem that annotations given to the constructor do not work, but Kogera has found a temporary solution.
https://github.com/ProjectMapK/jackson-module-kogera/blob/0ffefda68d5caa17b149d488e4e0054909e84319/src/main/kotlin/io/github/projectmapk/jackson/module/kogera/KotlinFeature.kt#L76

As long as there is a workaround, it does not seem so reasonable to change the treatment of composite constructors.

However, I too would like to target composite constructors for processing.

For example, what about adding a process to AnnotationIntrospector that would extract the synthetic constructors that I want to process from the class?
I am not familiar with the databind implementation, but I suspect such a feature would minimize the impact.

If this is also required in modules or libraries other than Kotlin, I think it would be worth considering implementation.

kkurczewski · 2023-08-03T17:41:01Z

Hi

As my PR was mentioned I will allow myself highlight problem which I have as well, assume I have class like this:

data class Something(val x: String, val y: String)

And I want to run code like this (it mimics Jackson ctor lookup):

Something::class.java.constructors.filter { !it.isSynthetic }.onEach { println("#1: $it") }.first()

And this will work. However, when you add an inline class to POJO it isn't gonna work:

data class SomethingWithInlineClass(val x: String, val y: kotlin.time.Duration)

SomethingWithInlineClass::class.java.constructors.filter { !it.isSynthetic }.onEach { println("2: $it") }.first()

Result is:

Exception in thread "main" java.util.NoSuchElementException: List is empty.
	at kotlin.collections.CollectionsKt___CollectionsKt.first(_Collections.kt:212)
	at com.fasterxml.jackson.module.kotlin.test.MainKt.main(main.kt:10)
	at com.fasterxml.jackson.module.kotlin.test.MainKt.main(main.kt)

In case of Jackson we get a more verbose message about missing creators. Sadly, there is no way to configure that behavior, there is no class equivalent like NopAnnotationIntrospector, StdValueInstantiator etc. that allow to customize that (there is a lot of final classes in chain) hence I don't have a place where I could use trick @k163377 propose.

cowtowncoder · 2023-08-03T17:42:42Z

Ok, I am worried too, although all unit tests do pass. But then again we don't have tests for auto-generated classes frameworks produce etc.
There is also no test showing this works (but I assume Kotlin module would have something).

I do think that improved handling should be added (but not via AnnotationIntrospector): ideally as part of full rewrite of property introspection. I still plan to tackle that before 2.16.0.
This probably wouldn't be added as part of rewrite BUT it:

could/should be tackled immediately after rewritten code works and
possibly should not be made before rewrite (as it might complicate)

So I won't be merging this before going on vacation, but it may be left open.

cowtowncoder · 2023-08-03T17:48:41Z

One more thing: allowing per-class inclusion of synthetic methods would be something I'd consider, but the question is how to access this without needing to sub-class is or have global configuration.
If code in question could access AnnotationIntrospector, that might be an option (although not necessarily great one). I may be forgetting some obvious choice, but pluggable "constructor classifier" could do.

The key, at any rate, would be to limit scope of changed visibility to Kotlin (-generated) classes; allow Kotlin module indicate changed rules.
If so, then other modules (often Scala module benefits from additions like this; but also some frameworks?) would benefit from this too, and risk would be limited.

pjfanning · 2023-08-03T17:59:52Z

@cowtowncoder I doubt whether this affects Scala module.

wplong11 · 2023-08-03T18:01:30Z

How about this approach? #4067
Ideally, all the logic for Kotlin support would be improved to be in the Kotlin module, but I wanted to start small first.

wplong11 · 2023-08-03T18:19:41Z

I will also check #4066 (comment)

wplong11 · 2023-08-03T18:27:34Z

@k163377

#4066 (comment) For example, what about adding a process to AnnotationIntrospector that would extract the synthetic constructors that I want to process from the class?
I am not familiar with the databind implementation, but I suspect such a feature would minimize the impact.

You mean you want to be able to set the constructor extraction strategy using Module interface (SimpleModule / KotlinModule)?

#4066 (comment) all the logic for Kotlin support would be improved to be in the Kotlin module

If so, this is also possible and is a satisfactory direction.

k163377 · 2023-08-03T18:54:09Z

@kkurczewski

Something::class.java.constructors.filter { !it.isSynthetic }.onEach { println("#1: $it") }.first()

I don't think I'm reading your argument well, but does not using getDeclaredConstructors have anything to do with the issue?
https://docs.oracle.com/javase/jp/8/docs/api/java/lang/Class.html#getDeclaredConstructors--

@wplong11

How about this approach? #4067

Could you please check the Kogera implementation and test cases first?
Since Kogera provides deserialization of value class, I would be happy if you could make a suggestion based on that.

At the very least, the approach you suggested is not sufficient, since the instantiation of the value class requires calling box-impl after calling constructor-impl.

You mean you want to be able to set the constructor extraction strategy using Module interface (SimpleModule / KotlinModule)?

Yes, it should be customizable from Module.

kkurczewski · 2023-08-03T19:26:38Z

@k163377

Something::class.java.constructors.filter { !it.isSynthetic }.onEach { println("#1: $it") }.first()

I don't think I'm reading your argument well, but does not using getDeclaredConstructors have anything to do with the issue? https://docs.oracle.com/javase/jp/8/docs/api/java/lang/Class.html#getDeclaredConstructors--

Yes, my mistake - Jackson uses declared constructors. I had too many attempts and my memory starts to plays tricks on me. But I'm pretty sure my current impl still has problem with missing creator, but will take a second look on my PR and will try to use this method.

wplong11 · 2023-08-04T14:46:00Z

since the instantiation of the value class requires calling box-impl after calling constructor-impl.

@k163377 Sorry, I didn't understand.
The Value Class seems to be initialized correctly even if I called box-impl without calling constructor-impl.(FasterXML/jackson-module-kotlin#694 - I added more test cases)
Is there any black magic I don't know about?

k163377 · 2023-08-04T14:56:01Z

The value class may have a check process in its constructor.
If only box-impl is called, the check is actually skipped.
https://github.com/ProjectMapK/jackson-module-kogera/blob/81c6497eaa5609ede547567bc8ebdc961399e77d/src/test/kotlin/io/github/projectmapk/jackson/module/kogera/_integration/deser/value_class/NoSpecifiedTest.kt#L43-L48

cowtowncoder · 2023-08-18T23:32:51Z

Ok, this looks doable -- but I'll need to think about it a bit; I hope to be able to give better feedback (or approve) soon but have a big backlog to go through after vacation.

JooHyukKim · 2023-09-02T00:11:12Z

For example, what about adding a process to AnnotationIntrospector that would extract the synthetic constructors that I want to process from the class?
I am not familiar with the databind implementation, but I suspect such a feature would minimize the impact.

+1 on what @k163377 suggests, adding "extensible point on for modules that can override property discovery behavior". But few things to consider...

Method : isIncludableConstructor method is private static --is current change, so we need higher level method for extension. Check comment
Class : AnnotationIntrospector name itself already suggests it's not for property-discovery

JooHyukKim · 2023-09-02T00:11:35Z

src/main/java/com/fasterxml/jackson/databind/introspect/AnnotatedCreatorCollector.java

@@ -406,6 +411,6 @@ private final AnnotationMap collectAnnotations(AnnotatedElement main, AnnotatedE

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


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,

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

Agreed 👍🏻

cowtowncoder · 2023-09-02T17:34:18Z

Ok, I'd be happy to merge this, if we can change this to non-draft PR?

k163377 · 2023-09-02T17:48:04Z

Personally, I am against merging this PR.

It contains at least some undesirable content from a Kotlin perspective.
It also looks like this could affect many projects that use Kotlin.
I don't think we should make changes with such a large impact when workarounds exist.

k163377 · 2023-09-02T17:50:35Z

src/main/java/com/fasterxml/jackson/databind/ext/KotlinSupport.java

+    public static boolean isJvmInlineClassSyntheticBoxingFunction(Method method) {
+        return Modifier.isStatic(method.getModifiers())
+                && method.isSynthetic()
+                && method.getName().equals("box-impl");
+    }


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

Therefore, this change should be removed.

k163377 · 2023-09-02T17:59:09Z

src/main/java/com/fasterxml/jackson/databind/ext/KotlinSupport.java

+    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");
+    }


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.

k163377 · 2023-09-02T18:16:00Z

I thought about it again, but it seems that simply including the synthetic constructor in the processing target does not solve the problem, since the deserialization of the value class requires two functions to be called in the correct order.
It seems to me that we need a function to make externally defined functions and processes available as Creator.

cowtowncoder · 2023-09-03T22:22:32Z

@k163377 I trust your judgment here -- I was under (wrong) impression that this alone would help solve the issue by not pruning certain synthetic constructors/factory methods. But if this alone won't help, yeah, no point in doing that.

That said, I am perfectly willing to add a little bit of code (like here) where necessary to support extensions such as Kotlin or Scala module, and have appropriate scope not to change behavior of types on "other" platforms.
But it sounds, unfortunately, that the code here isn't quite discriminating enough?

Ideally we would have something workable here -- but I need help to known when/if this is the case.

cowtowncoder · 2023-11-29T04:24:15Z

Looks like this PR isn't going anywhere and is out-of-date. Will close.

JooHyukKim · 2023-11-30T00:46:07Z

Looks like this PR isn't going anywhere and is out-of-date. Will close.

WRT out-dated, stale issues, there is automated labeler as Github workflow for stale-ness we might be interested in?
I recently been thinking we need one cause you manage loads of PRs and issues 🥲

cowtowncoder · 2023-11-30T01:35:28Z

@JooHyukKim thank you for suggestion!

My immediate reaction is that I am -1 for auto-close-issues Bots. Projects that use them seem to have very aggressive garbage collection, leading to (IMHO) hostile experience for submitters. So while I have sympathy for maintainers, personally I much prefer actual human beings closing issues over automated processes.

But this is only for Bots that actually close issues -- I would not be against auto-labeller?
As long as that has long enough period before adding label(s).

JooHyukKim · 2023-11-30T01:38:57Z

Np, @cowtowncoder 👍🏼 I agree, closing would be too agressive. And yes it will be just labeler. Okay, will get into it.

cowtowncoder · 2023-11-30T01:42:38Z

Sounds good -- and we could/should probably only enable it (if we do) for some of highest-traffic repos.

wplong11 marked this pull request as draft August 3, 2023 15:51

JooHyukKim mentioned this pull request Aug 3, 2023

Use box-impl function as factory for kotlin value class #4067

Closed

wplong11 force-pushed the revert-1005 branch 2 times, most recently from 887e79c to a39e938 Compare August 4, 2023 13:24

wplong11 changed the title ~~Support Synthetic Kotlin constructors~~ Use kotlin jvm inline class synthetic functions Aug 4, 2023

wplong11 changed the title ~~Use kotlin jvm inline class synthetic functions~~ Use synthetic functions of kotlin jvm inline class Aug 4, 2023

wplong11 added 2 commits August 4, 2023 22:29

Use kotlin jvm inline class synthetic constructors

0e53942

Use box-impl function as factory for kotlin jvm inline class

02f3fd2

wplong11 force-pushed the revert-1005 branch from a39e938 to 02f3fd2 Compare August 4, 2023 13:29

Merge branch '2.16' into revert-1005

1dbce7b

JooHyukKim reviewed Sep 2, 2023

View reviewed changes

cowtowncoder added a commit that referenced this pull request Sep 2, 2023

Minor refactoring to help with #4066

322a640

cowtowncoder added 2 commits September 2, 2023 10:30

Merge branch '2.16' into revert-1005

c1d08c2

Add more comments

ff989c4

k163377 reviewed Sep 2, 2023

View reviewed changes

cowtowncoder closed this Nov 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use synthetic functions of kotlin jvm inline class #4066

Use synthetic functions of kotlin jvm inline class #4066

wplong11 commented Aug 3, 2023

pjfanning commented Aug 3, 2023 •

edited

Loading

wplong11 commented Aug 3, 2023

wplong11 commented Aug 3, 2023 •

edited

Loading

pjfanning commented Aug 3, 2023

k163377 commented Aug 3, 2023

kkurczewski commented Aug 3, 2023

cowtowncoder commented Aug 3, 2023

cowtowncoder commented Aug 3, 2023

pjfanning commented Aug 3, 2023

wplong11 commented Aug 3, 2023 •

edited

Loading

wplong11 commented Aug 3, 2023

wplong11 commented Aug 3, 2023 •

edited

Loading

k163377 commented Aug 3, 2023

kkurczewski commented Aug 3, 2023

wplong11 commented Aug 4, 2023

k163377 commented Aug 4, 2023

cowtowncoder commented Aug 18, 2023

JooHyukKim commented Sep 2, 2023 •

edited

Loading

JooHyukKim Sep 2, 2023

cowtowncoder Sep 2, 2023

JooHyukKim Sep 3, 2023

cowtowncoder commented Sep 2, 2023

k163377 commented Sep 2, 2023

k163377 Sep 2, 2023

k163377 Sep 2, 2023

k163377 commented Sep 2, 2023

cowtowncoder commented Sep 3, 2023 •

edited

Loading

cowtowncoder commented Nov 29, 2023

JooHyukKim commented Nov 30, 2023 •

edited

Loading

cowtowncoder commented Nov 30, 2023

JooHyukKim commented Nov 30, 2023

cowtowncoder commented Nov 30, 2023

		@@ -406,6 +411,6 @@ private final AnnotationMap collectAnnotations(AnnotatedElement main, AnnotatedE

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

Use synthetic functions of kotlin jvm inline class #4066

Use synthetic functions of kotlin jvm inline class #4066

Conversation

wplong11 commented Aug 3, 2023

Background

pjfanning commented Aug 3, 2023 • edited Loading

wplong11 commented Aug 3, 2023

wplong11 commented Aug 3, 2023 • edited Loading

pjfanning commented Aug 3, 2023

k163377 commented Aug 3, 2023

kkurczewski commented Aug 3, 2023

cowtowncoder commented Aug 3, 2023

cowtowncoder commented Aug 3, 2023

pjfanning commented Aug 3, 2023

wplong11 commented Aug 3, 2023 • edited Loading

wplong11 commented Aug 3, 2023

wplong11 commented Aug 3, 2023 • edited Loading

k163377 commented Aug 3, 2023

kkurczewski commented Aug 3, 2023

wplong11 commented Aug 4, 2023

k163377 commented Aug 4, 2023

cowtowncoder commented Aug 18, 2023

JooHyukKim commented Sep 2, 2023 • edited Loading

JooHyukKim Sep 2, 2023

Choose a reason for hiding this comment

cowtowncoder Sep 2, 2023

Choose a reason for hiding this comment

JooHyukKim Sep 3, 2023

Choose a reason for hiding this comment

cowtowncoder commented Sep 2, 2023

k163377 commented Sep 2, 2023

k163377 Sep 2, 2023

Choose a reason for hiding this comment

k163377 Sep 2, 2023

Choose a reason for hiding this comment

k163377 commented Sep 2, 2023

cowtowncoder commented Sep 3, 2023 • edited Loading

cowtowncoder commented Nov 29, 2023

JooHyukKim commented Nov 30, 2023 • edited Loading

cowtowncoder commented Nov 30, 2023

JooHyukKim commented Nov 30, 2023

cowtowncoder commented Nov 30, 2023

pjfanning commented Aug 3, 2023 •

edited

Loading

wplong11 commented Aug 3, 2023 •

edited

Loading

wplong11 commented Aug 3, 2023 •

edited

Loading

wplong11 commented Aug 3, 2023 •

edited

Loading

JooHyukKim commented Sep 2, 2023 •

edited

Loading

cowtowncoder commented Sep 3, 2023 •

edited

Loading

JooHyukKim commented Nov 30, 2023 •

edited

Loading