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

ksp: more accurately represent function types #1742

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

evant
Copy link
Contributor

@evant evant commented Nov 27, 2023

When converting a KSTypeReference, represent a function type with LambdaTypeName. The better matches the source code and allows for receiver types (ex: String.() -> Unit) to be represented correctly.

When converting a KSTypeReference, represent a function type with
LambdaTypeName. The better matches the source code and allows for
receiver types (ex: String.() -> Unit) to be represented correctly.
Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Nice improvement 👍

One open question (that may be annoying to test) – do you know what happens when you have a function with more than 22 parameters? See https://github.com/Kotlin/KEEP/blob/master/proposals/functional-types-with-big-arity-on-jvm.md

Copy link
Collaborator

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

I don't know anything about KSP but the test improvement seems very nice. Should we add a test for direct usage of Function interfaces to ensure they work? Or are we just going to safely assume that since they're otherwise plain 'ol types.

@ZacSweers can have the final say with regard to the validation thing (which I have no idea wtf that is)

@evant
Copy link
Contributor Author

evant commented Nov 27, 2023

Or are we just going to safely assume that since they're otherwise plain 'ol types.

Yep they are treated just like any other type (just verified locally). No strong opinions here so I wouldn't mind adding to the test if we feel it's useful.

@evant
Copy link
Contributor Author

evant commented Nov 27, 2023

One open question (that may be annoying to test) – do you know what happens when you have a function with more than 22 parameters?

Seems to generate fine if that's what you are asking, I could add something to the test if you want?

…poet/ksp/test/processor/TestProcessor.kt

Co-authored-by: Jake Wharton <github@jakewharton.com>
@ZacSweers
Copy link
Collaborator

Yeah let's add a test just for coverage if you don't mind, otherwise lgtm 👍

@ZacSweers
Copy link
Collaborator

Sorry I wasn't specific – let's cover both direction Function type usages and one 23+ param function

@evant
Copy link
Contributor Author

evant commented Nov 27, 2023

Sorry, I'm a bit confused on what we'd actually be testing with that. From a codegen perspective nothing changes at 23+ params. According to the proposal you linked the generated bytecode is different but I don't think that smoke test is testing that anyway?

@ZacSweers
Copy link
Collaborator

I updated it with what I meant. You're right, I just wanted to be sure :). I was curious if KSP would render that as a KSCallableReference still or if it would represent it as a FunctionN with arity

@evant
Copy link
Contributor Author

evant commented Nov 27, 2023

heh ok

@JakeWharton JakeWharton merged commit 6c0d9b6 into square:main Nov 27, 2023
4 checks passed
@JakeWharton
Copy link
Collaborator

Thanks!

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.

3 participants