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

Binary compatibility and inline accessors #16983

Closed
nicolasstucki opened this issue Feb 21, 2023 · 7 comments · Fixed by #18402
Closed

Binary compatibility and inline accessors #16983

nicolasstucki opened this issue Feb 21, 2023 · 7 comments · Fixed by #18402
Assignees
Labels
itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException)

Comments

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Feb 21, 2023

Compiler version

3.0, 3.1, 3.2, 3.3

Inline accessors

Whenever we refer to a definition that is not public in an inline definition, we generate an inline accessor to access that definition from where the code is inlined.

final class C:
  private def a: Int = 2
  inline def b: Int = a + C.d

object C:
  private[C] val d: Int = 4

is transformed into something like

final class C:
  private def a: Int = 2
  def inline$a = a
  def inline$d = C.d
  inline def b: Int = inline$a + inline$d
object C:
  private[C] val d: Int = 4

Note that the accessors are added to the class using the private definition. This can lead to duplication of accessors.

Issue 1: Changing any definition from private to public is a binary incompatible change

object C:
- private def a: Int = 2
+ def a: Int = 2
  inline def b: Int = a

This change would remove the need to generate C$$inline$a because a is publicly accessible. But any compiled code that inlined b before this change refers to C$$inline$a. This code can be located in different libraries, and we might be unable to recompile it with the new inline definition.

Issue 2: Changing the implementation of an inline definition can be a binary incompatible change

object C:
  private def a: Int = 2
-  inline def b: Int = a
+  inline def b: Int = 3

This change would remove the need to generate C$$inline$a because we never inline a. But any compiled code that inlined b before this change refers to C$$inline$a. This code can be located in different libraries, and we might be unable to recompile it with the new inline definition.

Issue 3: Removing final from a class is a binary incompatible change

- final class C:
+ class C:
  private def a: Int = 2
  inline def b: Int = a

If the class is final a will get the accessor inline$a but if it is not final it gets the accessor C$$inline$a.

@nicolasstucki nicolasstucki added the itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException) label Feb 21, 2023
@Kordyjan
Copy link
Contributor

I think it is interesting how Kotlin deals with that. They could have the same problem, but they solved it by requiring every symbol used from the inline function to be either public or have a @PublishedApi annotation.

@PublishedApi makes annotated symbol look in the output bytecode the same way as if it was public. Of course, the proper visibility is still checked during the compiletime. Therefore

object C {
-   @PublishedApi internal fun sth() = TODO()
+   fun sth() = TODO()
}

is a safe and backward-compatible change.

The benefit of doing it only through annotation and not automatically is that all symbols used from the inline functions are documented, so the maintainers know they have the same compatibility guarantees as public symbols.

To solve our issue, I suggest taking inspiration from the Kotlin approach:
We would leave the accessors' generation as it is. We would also introduce a new annotation in the standard library (let's call it @api). In 3.3, each time we generate an inline accessor, we should check if the accessed symbol is annotated with the @api annotation; if not, we issue a warning. We should change the warning into a proper error in some future versions.

The @api annotation will force the generation of the accessor and leave a visible trace that the annotated symbol is part of the API and is subject to the same compatibility guarantees as any public symbol.

This still leaves

object C:
-  @api private def a: Int = 2
+  def a: Int = 2
}

as an unsafe change breaking binary compatibility, but it is at least well documented.

@nicolasstucki
Copy link
Contributor Author

A possible solution would be to add a @inlineAccessible annotation to make a public inline accessor for that definition. Inline methods will use these inline accessors if they exist.

final class C:
  @inlineAccessible private def a: Int = 2
  def inline$a = a // generated by @inlineAccessible
  inline def b: Int = a // use inline$a because a has @inlineAccessible

We would also need to phase out the old inline accessor generation after this feature is added. We could start warning users when a dangerous inline accessor is generated.

@nicolasstucki
Copy link
Contributor Author

I forgot to click on Comment and in meantime @Kordyjan #16983 (comment) proposed the same idea. I already coded a prototype.

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 21, 2023
@nicolasstucki nicolasstucki self-assigned this Feb 21, 2023
@nicolasstucki
Copy link
Contributor Author

A possible problem with the annotation approach is how to make top-level objects accessible

class C:
  inline def f: Int = C.n
@inlineAccessible private object C:
  def n: Int  = 3

Not sure if a static method in class C$ would work.

@sjrd
Copy link
Member

sjrd commented Feb 21, 2023

We could take it a step further and decouple it from the inline use case. I would suggest to call it @binaryAPI (and perhaps a variant @binaryAPI(binaryAPI.Protected)). If such a method is accessed from an inline we call it as is instead of generating an inline accessor.

But it then has other uses. In particular, it can be used for binary compatibility scenarios. If I want to make a public method private in a new version, I can mark it as @binaryAPI to preserve binary compatibility. Currently, we use the private[package] trick for that, but it leads to issues because MiMa does not respect transitivity for package-private stuff.

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 22, 2023
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 22, 2023
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 22, 2023
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 22, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
@nicolasstucki
Copy link
Contributor Author

@sjrd how would @binaryAPI work on private[this] definitions? For example

class A:
  @binaryAPI private val x = 1
  inline def f = x
class B extends A:
  @binaryAPI private val x = 2
  inline def g = x

We cannot make those definitions public or package private in the binary as they would clash. Maybe we don't allow those.

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 22, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
@sjrd
Copy link
Member

sjrd commented Feb 22, 2023

That's a good point. I think we would have to refuse those, indeed. Not sure whether that makes sense 🤔

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 22, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 22, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 22, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 23, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 23, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 23, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Feb 23, 2023
Part of the fix for scala#15413
Part of the fix for scala#16983
nicolasstucki added a commit that referenced this issue Dec 19, 2023
…ing flag (#18402)

Introduces
[SIP-52](scala/improvement-proposals#58) as an
experimental feature.

Fixes #16983

### `@publicInBinary`

A binary API is a definition that is annotated with `@publicInBinary` or
overrides a definition annotated with `@publicInBinary`. This annotation
can be placed on `def`, `val`, `lazy val`, `var`, `object`, and `given`
definitions. A binary API will be publicly available in the bytecode. It
cannot be used on `private`/`private[this]` definitions.

#### Interaction with inline
This is useful in combination with inline definitions. If an inline
definition refers to a private/protected definition marked as
`@publicInBinary` it does not need to use an accessor. We still generate
the accessors for binary compatibility but do not use them.

### `-WunstableInlineAccessors`
If the linting option `-WunstableInlineAccessors` is enabled, then a
warning will be emitted if an inline accessor is generated. The warning
will guide the user to the use of `@publicInBinary`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
itype:soundness Soundness bug (it lets us compile code that crashes at runtime with a ClassCastException)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants