Update to a single annotation with linter warning

nicolasstucki · nicolasstucki · commit dd862f85e1e1 · 2023-08-16T11:15:48.000+02:00
diff --git a/content/binary-api.md b/content/binary-api.md
@@ -13,10 +13,12 @@ title: SIP-52 - Binary APIs
 | Date          | Version            |
 |---------------|--------------------|
 | Feb 27 2022   | Initial Draft      |
+| Aug 16 2022   | Single Annotation  |
 
 ## Summary
 
-This proposal introduces the `@binaryAPI` and  `@binaryAPIAccessor` annotations on term definitions. The purpose of binary APIs is to have publicly accessible definitions in generated bytecode for definitions that are private or protected.
+The purpose of binary APIs is to have publicly accessible definitions in generated bytecode for definitions that are package private or protected.
+This proposal introduces the `@binaryAPI` annotation on term definitions and the `-WunstableInlineAccessors` linting flag.
 
 
 ## Motivation
@@ -66,7 +68,7 @@ object C:
 
 ### High-level overview
 
-This proposal introduces 2 the `@binaryAPI` and `@binaryAPIAccessor` annotations, and adds a migration path to inline methods.
+This proposal introduces the `@binaryAPI` annotation, and adds a migration path to inline methods in libraries (requiring binary compatibility).
 
 #### `@binaryAPI` annotation
 
@@ -100,49 +102,18 @@ public class C {
 In the bytecode, `@binaryAPI` definitions will have the [ACC_PUBLIC](https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.1-200-E.1) flag.
 <!-- We can also set the [ACC_SYNTHETIC](https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.1-200-E.1) to hide these definitions from javac and java IDEs. -->
 
-#### `@binaryAPIAccessor` annotation
-
-A binary API with accessor is a definition that is annotated with `@binaryAPIAccessor`.
-This annotation can be placed on `def`, `val`, `lazy val`, `var`, `object`, and `given` definitions.
-The annotated definition will get a public accessor.
-
-This can be used to access `private`/`private[this]` definitions within inline definitions.
-
-Example:
-~~~ scala
-class C {
-  @binaryAPIAccessor private def privateAPI: Int = ...
-  @binaryAPIAccessor def publicAPI: Int = ...
-}
-~~~
-will generate the following bytecode signatures
-~~~ java
-public class C {
-  public C();
-  private int privateAPI();
-  public int publicAPI();
-  public final int C$$inline$privateAPI();
-  public final int C$$inline$publicAPI();
-}
-~~~
-
-Note that the change from `private[this]` to package private, protected or public is a binary compatible change.
-Removing this annotation is a binary incompatible change.
-
-In the bytecode, `@binaryAPIAccessor` generated accessors will have the [ACC_PUBLIC | ACC_SYNTHETIC](https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.1-200-E.1) flag.
-
 #### Binary API and inlining
 
 A non-public reference in an inline method is handled as follows:
  - if the reference is a `@binaryAPI` the reference is used;
- - if the reference is a `@binaryAPIAccessor` the accessor is used;
- - otherwise, an accessor is automatically generated and used. We will emit a warning with an actionable diagnostic containing the code needed to migrate to `@binaryAPI` or `@binaryAPIAccessor`.
+ - otherwise, an accessor is automatically generated and used.
 
-Example 3:
+Example:
 ~~~ scala
+import scala.annotation.binaryAPI
 class C {
-  @binaryAPIAccessor private def a: Int = ...
-  private def b: Int = ...
+  @binaryAPI private[C] def a: Int = ...
+  private[C] def b: Int = ...
   @binaryAPI protected def c: Int = ...
   protected def d: Int = ...
   inline def foo: Int = a + b + c + d
@@ -151,26 +122,100 @@ class C {
 before inlining the compiler will generate the accessors for inlined definitions
 ~~~ scala
 class C {
-  @binaryAPIAccessor private def a: Int = ...
-  private def b: Int = ...
+  @binaryAPI private[C] def a: Int = ...
+  private[C] def b: Int = ...
   @binaryAPI protected def c: Int = ...
   protected def d: Int = ...
-  final def C$$inline$a: Int = ... // generated by `@binaryAPIAccessor`
-  final def C$$inline$b: Int = ... // warn: `b` should be annotated with `@binaryAPIAccessor` + migration code
-  final def C$$inline$d: Int = ... // warn: `d` should be annotated with `@binaryAPI` + migration code
-  inline def foo: Int = C$$inline$a + C$$inline$b + c + C$$inline$d
+  final def C$$inline$b: Int = ...
+  final def C$$inline$d: Int = ...
+  inline def foo: Int = a + C$$inline$b + c + C$$inline$d
 }
 ~~~
 
+##### `-WunstableInlineAccessors`
+
+In addition we introduce the `-WunstableInlineAccessors` flag to allow libraries to detect when the compiler generates unstable accessors.
+The previous code would show a linter warning that looks like this:
+
+~~~
+-- [E...] Compatibility Warning: C.scala -----------------------------
+  |  inline def foo: Int = a + b + c + d
+  |                            ^
+  |            Unstable inline accessor C$$inline$b was generated in class C.
+  |
+  | longer explanation available when compiling with `-explain`
+-- [E...] Compatibility Warning: C.scala -----------------------------
+  |  inline def foo: Int = a + b + c + d
+  |                                    ^
+  |            Unstable inline accessor C$$inline$d was generated in class C.
+  |
+  | longer explanation available when compiling with `-explain`
+~~~
+
+When an accessor is detected we can tell the user how to fix the issue. For example we could use the `-explain` flag to add the following details to the message.
+
+<details>
+<summary>With `-WunstableInlineAccessors -explain`</summary>
+
+~~~
+-- [E...] Compatibility Warning: C.scala -----------------------------
+  |  inline def foo: Int = a + b + c + d
+  |                            ^
+  |            Unstable inline accessor C$$inline$b was generated in class C.
+  |-----------------------------------------------------------------------------
+  | Explanation (enabled by `-explain`)
+  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+  | Access to non-public method b causes the automatic generation of an accessor.
+  | This accessor is not stable, its name may change or it may disappear
+  | if not needed in a future version.
+  |
+  | To make sure that the inlined code is binary compatible you must make sure that
+  | method b is public in the binary API.
+  |  * Option 1: Annotate method b with @binaryAPI
+  |  * Option 2: Make method b public
+  |
+  | This change may break binary compatibility if a previous version of this
+  | library was compiled with generated accessors. Binary compatibility should
+  | be checked using MiMa. If binary compatibility is broken, you should add the
+  | old accessor explicitly in the source code. The following code should be
+  | added to class C:
+  |   @binaryAPI private[C] def C$$inline$b: Int = this.b
+   -----------------------------------------------------------------------------
+-- [E...] Compatibility Warning: C.scala -----------------------------
+  |  inline def foo: Int = a + b + c + d
+  |                                    ^
+  |            Unstable inline accessor C$$inline$d was generated in class C.
+  |-----------------------------------------------------------------------------
+  | Explanation (enabled by `-explain`)
+  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+  | Access to non-public method d causes the automatic generation of an accessor.
+  | This accessor is not stable, its name may change or it may disappear
+  | if not needed in a future version.
+  |
+  | To make sure that the inlined code is binary compatible you must make sure that
+  | method d is public in the binary API.
+  |  * Option 1: Annotate method d with @binaryAPI
+  |  * Option 2: Make method d public
+  |
+  | This change may break binary compatibility if a previous version of this
+  | library was compiled with generated accessors. Binary compatibility should
+  | be checked using MiMa. If binary compatibility is broken, you should add the
+  | old accessor explicitly in the source code. The following code should be
+  | added to class C:
+  |   @binaryAPI private[C] def C$$inline$d: Int = this.d
+   -----------------------------------------------------------------------------
+~~~
+
+</details>
+
 ### Specification
 
-We must add `binaryAPI` and `binaryAPIAccessor` to the standard library.
+We must add `binaryAPI` to the standard library.
 
 ```scala
 package scala.annotation
 
 final class binaryAPI extends scala.annotation.StaticAnnotation
-final class binaryAPIAccessor extends scala.annotation.StaticAnnotation
 ```
 
 #### `@binaryAPI` annotation
@@ -179,63 +224,35 @@ final class binaryAPIAccessor extends scala.annotation.StaticAnnotation
 * TASTy will contain references to non-public definitions that are out of scope but `@binaryAPI`. TASTy already allows those references.
 * The annotated definitions will be public in the generated bytecode. Definitions should be made public as early as possible in the compiler phases, as this can remove the need to create other accessors. It should be done after we check the accessibility of references.
 
-
-#### `@binaryAPIAccessor` annotation
-
-This annotation is only valid on `def`, `val`, `lazy val`, `var`, `object`, and `given`.
-
-A public accessor will be generated for the annotated definition. This accessor will be named `<fullClassName>$$inline$<definitionName>`. If the public accessor is in an object, the accessor will be named `inline$<definitionName>`. These names where chosen to minimize the complexity of migrating from automatically generates accessors in inline methods to `@binaryAPIAccessor` (see [Gist](https://gist.github.com/nicolasstucki/003f7293941836b08a0d53dbcb913e3c)).
-
 #### Inline
 
 * Inlining will not require the generation of an inline accessor for binary APIs.
-* Inlining will not require the generation of a new inline accessor, it will use the binary API accessors.
-* The user will be warned if a new inline accessor is automatically generated.
-  The message will suggest `@binaryAPI` or `@binaryAPIAccessor` and how to fix potential incompatibilities.
-  In a future version, these will become an error.
+* The user will be warned if a new inline accessor is automatically generated under `-WunstableInlineAccessors`.
+  The message will suggest `@binaryAPI` and how to fix potential incompatibilities.
 
 ### Compatibility
 
-The introduction of the `@binaryAPI` and `@binaryAPIAccessor` do not introduce any binary incompatibility.
-
-Using references to `@binaryAPI` and `@binaryAPIAccessor` in inline code can cause binary incompatibilities. These incompatibilities are equivalent to the ones that can occur due to the unsoundness we want to fix. When migrating to binary APIs, the compiler will show the implementation of accessors that the users need to add to keep binary compatibility with pre-binaryAPI code.
+The introduction of the `@binaryAPI` do not introduce any binary incompatibility.
 
-A definition can be both `@binaryAPI` and `@binaryAPIAccessor`. This would be used to indicate that the definition used to be private, but now we want to publish it as public. The definition would become public, and the accessor would be generated for binary compatibility.
+Using references to `@binaryAPI` in inline code can cause binary incompatibilities. These incompatibilities are equivalent to the ones that can occur due to the unsoundness we want to fix. When migrating to binary APIs, the compiler will show the implementation of accessors that the users need to add to keep binary compatibility with pre-binaryAPI code.
 
 ### Other concerns
 
 * Tools that analyze inlined TASTy code might need to know about `@binaryAPI`. For example [MiMa](https://github.com/lightbend/mima/) and [TASTy MiMa](https://github.com/scalacenter/tasty-mima).
 
-### Open questions
-
-#### Question 1
-Should `@binaryAPIAccessor` accessors be named `<fullClassName>$$<definitionName>`? This encoding would match the names of `trait` accessor generated for private definition. We could use a single accessor instead of two. This would introduce an extra binary incompatibility with pre-binaryAPI code.
-
-#### Question 2
-```scala
-class A:
-  @binaryAPIAccessor protected def protectedDef: Int = ...
-class B extends A:
-  override protected def protectedDef: Int = ...
-  inline def inlinedDef: Int =
-    // Should this use the accessor of generated for `A.protectedDef`? Or should we warn that `protectedDef` should be a `@binaryAPI`
-    protectedDef
-```
-
 ## Alternatives
 
-Having alternatives is not a strict requirement for a proposal, but having at least one with carefully exposed pros and cons gives much more weight to the proposal as a whole.
+### Add a `@binaryAPIAccessor`
+This annotation would generate an stable accessor. This annotation could be used on `private` definition. It would also mitigate [migration costs](https://gist.github.com/nicolasstucki/003f7293941836b08a0d53dbcb913e3c) for library authors that have published unstable accessors.
 
-### Only add `@binaryAPI`
-This would simplify the system and the user interaction with this feature. The drawback is that we could not access `private[this]` definitions in inline code. Users would need to use `private[C]` instead, which could cause name clashes.
+* Implementation https://github.com/lampepfl/dotty/pull/16992
 
-### Only add `@binaryAPIAccessor`
-This would simplify the system and the user interaction with this feature. The drawback is that we would add code size and runtime overhead to all uses of this feature. It would not solve the [Removing deprecated APIs](#removing-deprecated-apis) motivation.
 
 ## Related work
 
-* Proof of concept: [https://github.com/lampepfl/dotty/pull/16992](https://github.com/lampepfl/dotty/pull/16992)
 * Initial discussions: [https://github.com/lampepfl/dotty/issues/16983](https://github.com/lampepfl/dotty/issues/16983)
+* Initial proof of concept (outdated): [https://github.com/lampepfl/dotty/pull/16992](https://github.com/lampepfl/dotty/pull/16992)
+* Single annotation proof of concept: [https://github.com/lampepfl/dotty/pull/18402](https://github.com/lampepfl/dotty/pull/18402)
 * Community migration analysis: [Gist](https://gist.github.com/nicolasstucki/003f7293941836b08a0d53dbcb913e3c)
 
 <!-- ## FAQ -->
