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

jvmModifiers doesn't return the Builder #638

Open
ShaishavGandhi opened this issue Mar 4, 2019 · 10 comments · May be fixed by #2004
Open

jvmModifiers doesn't return the Builder #638

ShaishavGandhi opened this issue Mar 4, 2019 · 10 comments · May be fixed by #2004
Labels
Milestone

Comments

@ShaishavGandhi
Copy link
Collaborator

The method jvmModifiers in FunSpec breaks the chaining by not returning the Builder. There is a similar method in ParameterSpec which does return the Builder.

This will be a breaking change so it will have to wait till 2.x.

@Egorand Egorand added this to the 2.0 milestone Mar 4, 2019
@Egorand Egorand added the bug label Mar 4, 2019
@JakeWharton
Copy link
Collaborator

It doesn't need to wait.

@JvmName("jvmModifiers")
@Deprecated(level = HIDDEN)
fun jvmModifersOld(..) { .. }

@JvmName("jvmModifiersNew")
fun jvmModifers(..): Builder { .. }

@Egorand
Copy link
Collaborator

Egorand commented Mar 4, 2019

Wouldn't it break implicit returns in Kotlin though? Also, introducing jvmModifiersNew() for Java call-sites to remove it in 2.0 kinda sucks...

@JakeWharton
Copy link
Collaborator

Wouldn't it break implicit returns in Kotlin though?

Yes it is source incompatible but literally (and not hyperbolically) no one is relying on this. It would have to be the last function call on the builder when used as an expression.

Also, introducing jvmModifiersNew() for Java call-sites to remove it in 2.0 kinda sucks...

I mean you can name it whatever you want, but I'm not sure how much I care about the ergonomics of Java callers here. Android suffixed their methods with "Flags" when they made this change.

You can also use bytecode manipulation to break neither binary nor source compatibility: https://github.com/JakeWharton/OverloadReturn

@Egorand
Copy link
Collaborator

Egorand commented Mar 4, 2019

Yeah, agree. Fine with both fixing it now the way you suggest, or leaving it hanging until 2.0. The workaround is straightforward (.apply { jvmModifiers(...) }, so the issue probably doesn't really inconvenience anybody.

@anatawa12
Copy link
Contributor

anatawa12 commented Jul 29, 2019

Sorry, This comment may not be good for here.
I think we/you can make an future-request for kotlin.
if we use @Deprecated("", level = DeprecationLevel.HIDDEN), we can't see the method/property from any new source code and can call from old code.(method signature in JVM includes return type.) So I think Kotlin should allows this source. I think this future is good for fix this without breaking changes.

@Deprecated("", level = DeprecationLevel.HIDDEN)
fun test(){}
fun test(): Any = Any()

@Egorand
Copy link
Collaborator

Egorand commented Jul 29, 2019

I don't think that DeprecationLevel.HIDDEN is broken, it's designed to break source compatibility without breaking binary, which serves a common case - you want to break your library's direct consumers and have them change their code, but you don't want to break consumers who use your library transitively as a dependency of another library they use.

@anatawa12
Copy link
Contributor

you seem to want to make functions which same-name, same-parameter and different-result-type to fix this. But current Kotlin doesn't allow(see https://pl.kotl.in/wITeF-6gG). I think we can make a future request for Kotlin. This is a suggestion about what we can do.

@Egorand
Copy link
Collaborator

Egorand commented Jul 29, 2019

This is not something that Java allows either, and I'm pretty sure it's not allowed by the JVM spec (correct me if I'm wrong).

@JakeWharton
Copy link
Collaborator

It's allowed in bytecode, not source code.

@anatawa12
Copy link
Contributor

In the new java source code, the function with DeprecationLevel.HIDDEN is hidden in java too and can invoke new function.
In current Kotlin, not allowed but I think we can make a request to allow.

@Egorand Egorand linked a pull request Oct 15, 2024 that will close this issue
1 task
@Egorand Egorand modified the milestones: 2.0, 3.0 Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants