Skip to content
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

Add and fix test pattern for KFunction involving value class (to fix KT-31141). #4743

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Add and fix test pattern for KFunction involving value class (to fix KT-31141). #4743

merged 1 commit into from
Mar 10, 2022

Conversation

k163377
Copy link
Contributor

@k163377 k163377 commented Feb 18, 2022

Test patterns were added and modified to fix KT-31141.

This PR change will cover the following 3 x 2 patterns.

  • boxed type
    • non-null object type
    • nullable object type
    • primitive type
  • nullability of parameter or extension receiver
    • non-null
    • nullable

This PR does not include the mitigation measures that I introduced earlier.
https://kotlinlang.slack.com/archives/C0BUHC9HD/p1644666394983169

Questions

About nested value class

This change does not include testing for nested value class, as it seemed too much work.

As far as I can figure out, it would take up to two levels of nesting to fully cover the validation for nested value class.
On the other hand, if implement all these tests, I would have a combinatorial explosion as shown in the sample code below.

// primitive
inline class Z0(val value: Int)

inline class Z1_nonNull(val value: Z0)
inline class Z1_nullable(val value: Z0?)

inline class Z2_nonNull_nonNull(val value: Z1_nonNull)
inline class Z2_nullable_nonNull(val value: Z1_nonNull?)
inline class Z2_nonNull_nullable(val value: Z1_nullable)
inline class Z2_nullable_nullable(val value: Z1_nullable?)

// non-null object
inline class S0_nonNull(val value: String)

inline class S1_nonNull_nonNull(val value: S0_nonNull)

/* ... */

// nullable object
inline class S0_nullable(val value: String?)

/* ... */

In this PR, we have tested up to Z0/S0_nonNull/S0_nullable.
This means that implementing all the combinations shown above will increase the amount of code by about 7 times, and even up to 1-level nesting will increase it by about 3 times.

How should I handle the tests for nested value class?

Which version the mitigation will be merged into

It may be too early to tell, but if I create a PR about mitigation measures, which version will that merge into?

This mitigation is important for the value class support of libraries that depend on kotlin-reflect.
For this reason, I would be happy to merge the proposed mitigation into kotlin-reflect 1.5.x.

@udalov udalov self-assigned this Feb 18, 2022
@udalov
Copy link
Member

udalov commented Mar 8, 2022

Thanks for your work. The tests look good, I'll merge them after the build succeeds.

How should I handle the tests for nested value class?

Yeah, I would prefer not to add all combinations for now. I would rather focus on fixing the known problems already covered by newly added tests in this PR, and then, after they're all fixed, see if there are any cases with nested value classes which are failing.

which version will that merge into?
[...]
For this reason, I would be happy to merge the proposed mitigation into kotlin-reflect 1.5.x.

We don't usually cherry-pick fixes to older versions. Current version under development is 1.6.20, however it's very close to being released already, so most likely the fix will be merged into the next version (either 1.6.30 or 1.7.0 -- I'm not totally sure right now what it'll be).

@udalov udalov merged commit c4735f9 into JetBrains:master Mar 10, 2022
@udalov
Copy link
Member

udalov commented Mar 10, 2022

Thanks! Merged with minor changes (use TARGET_BACKEND: JVM for all tests, remove unneded Suppress)

@k163377 k163377 deleted the KT-31141/add_and_fix_tests branch March 11, 2022 02:28
armandRobled

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants