Skip to content

Detect nullable marker parameters in constructor arguments. #2773

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

Closed
wants to merge 2 commits into from

Conversation

christophstrobl
Copy link
Member

This draft seeks to introduce a nullableMarker flag to the Parameter abstraction. If set, it is safe skip and null the parameter value right away.

The PreferredConstructorDiscoverer for Kotlin will use this flag when detecting a DefaultConstructorMarker argument within the java constructor which is there (as the name indicates) to solely mark the constructor to use but does not map to any parameter from the source Kotlin type, resulting in an unresolvable parameter name/target property for the parameter.
This change will allow the ClassGeneratingEntityInstantiator to operate types using Kotlin inline classes.

I think it would also make sense to discuss if the usage of the newly introduced method makes sense in the ParameterValueProvider implementations like PersistentEntityParameterValueProvider to shortcut the value lookup by simply returning null

Closes: #1947

This commit introduces a nullableMarker flag to the Parameter abstraction. If set, it is safe skip and null the parameter value right away.
The PreferredConstructorDiscoverer for Kotlin will use this flag when detecting a DefaultConstrcutorMarker argument within the java constructor which is there (as the name indicates) to solely mark the constructor to use but does not map to any parameter from the source Kotlin type, resulting in an unresolvable parameter name/target property for the parameter.
This change will allow the ClassGeneratingEntityInstantiator to operate types using Kotlin inline classes.
@christophstrobl christophstrobl marked this pull request as ready for review March 1, 2023 12:29
@mp911de
Copy link
Member

mp911de commented Mar 1, 2023

We have a few other flavors of non-domain arguments for constructors such as the outer class instance (for non-static inner classes) or nullable mask markers.

In all other places, we do not create a Parameter in the first place and I think we should follow the same pattern here as well.

@christophstrobl
Copy link
Member Author

So we'd end up with a signature that requires us to hand more arguments than we have parameters for, which is odd in first place.

@mp911de
Copy link
Member

mp911de commented Mar 1, 2023

The alternative to introducing all sorts of markers isn't a better variant. Kotlin nullability integer masks are spread over a few constructor integer parameters. For every 32 nullable parameters there's one additional integer bit mask.

@christophstrobl
Copy link
Member Author

have an example for that? I doubt it's targeting the same scenario.

@christophstrobl
Copy link
Member Author

since we do not have a stable API contract (see: 1947) other than the public constructor, and cannot safely invoke the method with one parameter in the model missing skipping parameters seems to be a bad choice.
Maybe there's a difference in the use case you mention above that would allow to ignore entire constructors, but not the parameter if we detect it.

@mp911de mp911de closed this May 17, 2023
@mp911de mp911de deleted the issue/1947 branch May 17, 2023 08:57
@mp911de
Copy link
Member

mp911de commented May 17, 2023

We're switching to a more elaborate approach for Kotlin inline classes.

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.

Add support for Kotlin Value Classes
2 participants