Kotlin / binary-compatibility-validator

Public API management tool
Apache License 2.0
761 stars 55 forks source link

Consider `@PublishedApi internal inline` functions to not be public API #165

Closed kyay10 closed 6 months ago

kyay10 commented 6 months ago

Currently, if a library developer wishes to extract an inline part of a public inline fun's code, but doesn't wish to expose that new method, they're forced to mark it with @PublishedApi internal inline. While this isn't ideal, there's valid reasons for it I believe (JVM backend is the one that does inlining, not IR). However, that inlining happens at compile time, and hence there's no legitimate way for a compiled binary that uses a library to access that @PublishedApi inline fun from the library. Hence, I propose that such methods aren't outputted by the tool because they dissuade library developers from extracting those methods even though they won't actually ever be used from any compiled Kotlin binary

qwwdfsad commented 6 months ago

BCV validates not the API, but the ABI.

What it actually means is that if something is not an ABI (but it may be the API! E.g. reified inline fun), it is safe to remove it without breaking users' code during the linkage. @PublishedApi internal inline constitutes public ABI (https://github.com/Kotlin/binary-compatibility-validator?tab=readme-ov-file#what-makes-an-incompatible-change-to-the-public-binary-api)

kyay10 commented 6 months ago

@qwwdfsad but is it ABI? I know it shows up in the binary, but Kotlin code will never call it because it's inlined upon compilation, and Java code doesn't even see it. In other words, the jar that includes such a @PublishedApi internal inline method will never ever get any calls to that method (but it might be used during compilation to inline such a method, which is fine because removing such a method also means altering the public-facing methods that used it, and thus there would still be no dependency on it). I'm assuming there's just something wrong with my mental model, but I can't see a situation where compiled Kotlin code would try to access a @PublishedApi internal inline fun from a jar.

My understanding is that when an inline method is called, Kotlin always inlines it no matter what, and that public inline methods are present in the binary only for Java callers. Hence, if a method is @PublishedApi internal inline, no Java callers might call it, and all Kotlin callers will inline it upon compilation, and thus its existence in the binary doesn't affect anything.

qwwdfsad commented 6 months ago

Oh, I see what you've meant here, thanks for the elaborate answer.

PublishedApi internal inline, no Java callers might call it

Unfortunately, it's still available for Java callers, @PublishedApi is not mangled. Apart from that, there is also a caveat with multiplatform (e.g. we are going to introduce BCV for K/N and there inlining works differently, and we have to decide whether we want to expose it in dumps)

kyay10 commented 6 months ago

Isn't internal marked synthetic automatically? AFAIK internal methods aren't accessible to Java callers, but correct me if I'm wrong.

For K/N, would there ever be runtime inlining? I assume not, right? Thus @PublishedApi internal inline methods still don't need to be in the binary. I appreciate that there might be a difference with multiplatform, but for the time being this tool is only generating JVM dumps, right? Thus, would it not be preferable to correct the behaviour at least for the JVM dumps?

qwwdfsad commented 6 months ago

Isn't internal marked synthetic automatically?

They are not. AFAIR ACC_SYNTHETIC is used only for some compiler-generated methods (and for @JvmSynthetic ones as an exception from this rule :)).

I assume not, right?

It's a bit harder than that because K/N compilation is two-phased -- there is linkage (when the code is compiled into klib against other klibs) and actual compilation (when the final kexe/iOS bundle are built) and, depending on the phase, there are different binary compatibility guarantees. I would like to avoid exploring this one further :)

Thus, would it not be preferable to correct the behaviour at least for the JVM dumps?

Agree. Unfortunately, implementation details leak into this tool from time to time, like the one being discussed. E.g. in the closed Kotlin world, @PublishedApi internal inline is indeed not part of ABI. With Java in the picture, it is part of the ABI.

kyay10 commented 6 months ago

Just to clarify, is it the case that only @PublishedApi internal methods are accessible to Java, or is that the case for all internal methods? My understanding is that the tool doesn't report non-published internal methods as part of the ABI.

Hence, my point is that if non-published internal methods are not viewed as part of the ABI, even though they're accessible by Java callers, then by the same logic @PublishedApi internal inline methods should not be considered part of the ABI either.

Finally, though, could a tiny exception be made where @JvmSynthetic @PublishedApi internal inline methods are not considered part of the ABI by this tool? Correct me if I'm wrong, but that combination of annotations should guarantee no callers (Java nor Kotlin) that rely on the method being in the binary. I think such a change could be the best of both worlds here.

qwwdfsad commented 6 months ago

Just to clarify, is it the case that only @PublishedApi internal methods are accessible to Java, or is that the case for all internal methods?

Some of them are. Most of the time, the Kotlin compiler mangles them (i.e. compiles the function to name$module_name) so they are inaccessible from Java. Sometimes (for implementation or legacy reasons), it does not.

It's indeed possible to make specific exceptions and detect specific scenarios, as well as leverage some specific knowledge, but we would like to avoid that to keep the mental model straightforward and understandable.

@PublishedApi states in its contract that the marked declaration effectively becomes public and should be treated as such with respect to binary compatibility and it would be nice to keep it that way. Trying to outsmart the mental model/contract here makes room for both errors [1] and inconsistencies:

1) E.g. for specific @PublishedApi internal inline foo function there might be Java interop, but also there might be the case that for some specific combination of method references (e.g. ::foo) compiler will decide not to inline it because it's part of the ABI. And if BCV is now aware of that, then it's a false negative error.

2) Unfortunately, there are dozens of such scenarios where something seems to be ABI but is not for some specific reason. Some arbitrary examples are: - symbol in the file name effectively disables Java interop and allows a new line of reasoning w.r.t. inlining and accessibility of the method; covariant overrides in interfaces/classes with type parameter are usually not ABI but if they narrow the type -- they are; with @JvmName it is possible to create a signature inaccessible with Java, and with a combination of @JvmStatic it's probably possible to produce a classfile with public static something which never goes resolved into. There is also an incremental compilation to make things more interesting. Supporting all of the cases is unlikely possible or reasonable and supporting some of them brings inconsistency "this combination is supported and this one is not".