Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SIP-52 - Binary APIs #58
SIP-52 - Binary APIs #58
Changes from 1 commit
3b7acde
9806825
de9c9df
0861c51
19165b1
2857ccd
eb57925
1d22f52
23c29c7
2af1384
c5b585c
8ea645a
dd862f8
46bd277
f1dbf18
64f79d0
ca4f97d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an actual problem?
Based on the Semantic Versioning spec and a discussion with clarifications, my understanding is that the semver way would require a MAJOR version bump to remove a deprecated method.
Binary incompatibility of different major versions seems like a reasonable expectation.
There are obviously other version schemes other than semver, but I imagine the handling of deprecated APIs is similar (e.g. I think PVP is even more extreme, where both deprecating and removing the API would require a major bump)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is an actual problem because the Scala ecosystem, and therefore use of SemVer, is rooted in binary and TASTy compatibility. You want to be able to make source breaking changes while retaining binary and TASTy compatibility, and that should stay a MINOR version bump. Perhaps you are mitigating the source breakage with extension methods or new overloads instead.
(If breaking source compat required a MAJOR version bump in Scala, every new version of a library with a new public class or method would be a MAJOR version, and we don't want that for obvious reasons.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
private[pkg]
? The behavior ofprivate[C]
is to generate a name-mangled definitionC$$myOldAPI
to avoid clashes in case a subclass defines its ownmyOldAPI
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure in which cases
private[C]
generates a mangled name. I tried the following and non of the names get mangled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do get the mangling for
private[this] val
in traits.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it looks like the mangling only happens in Scala 2, not Scala 3. So that's still an issue for cross-compiling codebases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this ties-in well with the discussion in lightbend-labs/mima#724. To summarize:
private[package]
APIs, because the assumption is that these are internal to the library. This improves the signal-to-noise when making such internal changes.package[private]
is often used for removing a public API without breaking bincompat. But, due to (1), MiMa will no longer check these now-package-private APIs for binary-compatibility, which is dangerous. The workaround is to check MiMa against every previous artifact, but this has its own costs/risks.So, I hope that integration with MiMa will be considered as part of this proposal :)
Big 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this proposal was partially designed to address that MiMa problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the link lightbend-labs/mima#724 to the SIP document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is because those get mangled, then it should also be disallowed on
private[C]
definitions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mangled names are not an issue. The problem with these is that they compile to private methods in the bytecode. The names of those methods are not mangled, and therefore a class and its parent could have two unrelated private definitions with the same name. If we make them public in the bytecode, they would clash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're talking about bytecode it would be good to actually specify what we're generating at the bytecode level exactly. I assume that
public
in the code example below maps toACC_PUBLIC
in https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html, but we might also want to useACC_SYNTHETIC
so that these definitions stay hidden from javac and java IDEs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if adding
ACC_SYNTHETIC
is always a good idea.@binaryAPI def publicAPI: Int = ...
should not be marked asACC_SYNTHETIC
because it is truly public.@binaryAPI protected def publicAPI: Int = ...
overridable in JAVA if it hasACC_SYNTHETIC
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if you want the method to be overridable then it can't be ACC_SYNTHETIC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about access to inner private classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question for primary and secondary private constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently do not allow them in inline methods. Not sure if we could use
@binaryAPI
to make the inner class public in the bytecode. I will investigate this possibility.Same for the constructors. We would require
@binaryAPI
and not@binaryAPIAccessor
in this case as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
19165b1 adresses the question on constructors. scala/scala3#16992 Has been updated to reflect this addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this sentence (also I think there is a typo: "is
a
" should be "a
"?), could an example be provided?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the message suggest using either of them? It seems we should always suggest using the latter, because it would break the ABI of existing code called from inline defs if we add the former.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both can break the binary API.
@binaryAPI
migration will always break binary compatibility.private
/private[this]
need@binaryAPIAccessor
. These can break binary compatibility if the class is final.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code like this
would show an error message like this
More examples can be found in https://github.com/lampepfl/dotty/pull/16992/files#diff-d12e949663e52759a532bf54a827260b0513390bfcdebddf5ff1c1bd04919aaa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...
IIUC, currently accessors are generated next to the inline method, whereas
@binaryAPIAccessor
wil generate an accessor next to the annotated private method, so that migration cannot be made in a binary compatbile manner - right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the case if the inline method is in a different class from the accessed member.
On the other hand, if the inline method is in the class the accessor will be placed in the class that defines the accessed members, similar to
@binaryAPIAccessor
. In some cases, these accessors are the same and we avoid binary incompatibilities in the migration from auto-generated to@binaryAPIAccessor
(example 1).In general, the migration can't be made binary-compatible. We chose to reuse the accessor name
<fullClassName>$$inline$<definitionName>
to be binary compatible with some auto-generated accessors. The accessors that are binary compatible are the ones in a non-final class that are generated by an inline method in that class (example 1).Example 1 (compatible):
migrates to
Example 2 (incompatible):
migrates to
Example 3 (incompatible):
migrates to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it's maybe not worth introducing both
@binaryAPI
and@binaryAPIAccessor
? Is it always possible to start using@binaryAPI
and maintain binary compatibility by manually adding the accessors that were previously generated by the compiler?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with that approach is that users would need to change all their
private/private[this]
members that are accessed from inline methods toprotected/private[Cls]/pribate[pack]
. This can cause source incompatibilities and unintentionally leak private members beyond the scope of inlining.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, I'm sorry. Where am I wrong?
private
method is accessed in aninline def
, the compiler generates an accessor to the private method@binaryAPI
(but keep it private in source), which makes it public in bytecode, and the accessor is no longer generatedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@binaryAPI
cannot be placed on private/private[this] APIs. This is because that change could be a source or binary incompatible change. For example:If we make
x
public in the bytecode, we would accidentally makeB.x
overrideA.x
which would change the semantics of a private definition. Or we would have to flag them as overrides of private members, which implies that we would have to rename anything that would clash with a@binaryAPI private
. In this case we would not be able to use the namex
inB
anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, thanks. Was name-mangling considered?
"Only add
@binaryAPIAccessor
" could also be an option. The concernis maybe minor? Accessors are very common in Scala, why would these ones be problematic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do that we would not cover 2 of the motivations: Removing deprecated APIs and No way to inline reference to private constructors. Those use cases could be supported later with the addition of
@binaryAPI
.If we add
@binaryAPI
later there is also a migration concern. In a first step users would add@binaryAPIAccessor
to all the definitions, but then when@binaryAPI
they might also want to add that one to access the member directly. This would force them to have both@binaryAPIAccessor
and@binaryAPI
. If we add both at the same time, then they would be able to only use@binaryAPI
.I assume with
@binaryAPIAccessor
of private members. The issue is that we would need to mangle the names of private members but not public/protected members. This implies that if the user changes a private member into a public one the accessor would not be generated anymore. This is the exact same issue we have right now but triggered in a different way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details. I don't want to be annoying and keep discussing details here, the discussion should move elsewhere?
I think introducing two annotations is bad. Users will have to pick one or the other, they have to know how they both work and how using them affects binary compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see a scenario where a user would need to think about which annotation to choose. In the scenario of Removing deprecated APIs they can only use
@binaryAPI
. In all other scenarios with inlining, the users would be told which annotation they need to use.