Add `@publicInBinary` annotation and `-WunstableInlineAccessors` linting flag #18402

nicolasstucki · 2023-08-15T14:42:57Z

Introduces SIP-52 as an experimental feature.

`@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.

tests/neg/binaryAPI-not-accessible.check

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

smarter · 2023-12-12T20:40:52Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

+    def hasPublicInBinary(using Context): Boolean =
+      isTerm && (
+        hasAnnotation(defn.PublicInBinaryAnnot) ||
+        allOverriddenSymbols.exists(sym => sym.hasAnnotation(defn.PublicInBinaryAnnot))


Hmm, allOverriddenSymbols might be expensive and no other annotations is inherited as far as I know, so I'd really prefer if we were more strict in the implementation (the sip is still experimental, so the implementation and spec don't need to exactly match yet)

smarter · 2023-12-12T20:43:15Z

library/src/scala/annotation/publicInBinary.scala

+package scala.annotation
+
+/** 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.


The SIP also says that class constructors are supported, but this isn't mentioned here.

smarter · 2023-12-12T20:46:41Z

library/src/scala/annotation/publicInBinary.scala

+ *
+ *  This can be used to access private[T]/protected definitions within inline definitions.
+ *
+ *  Removing this annotation from a non-public definition is a binary incompatible change.


Adding it is also binary incompatible since it means some accessors won't be generated, right?

Yes. Added doc.

smarter · 2023-12-12T20:51:09Z

library/src/scala/annotation/publicInBinary.scala

+
+/** 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.


Can we clarify the behavior of this annotation in this documentation? As far as I can tell, we've established that this annotation doesn't change the bytecode representation of the thing that is annotated, because everything that is allowed to be annotated will already be public, so the documentation as-is might be confusing. Instead the effect of this annotation is:

no unnecessary accessors will be generated/used (which I guess should be considered a bug-fix since these accessors were never actually necessary and could break compilation?)

Tools like mima/tasty-mima can rely on this annotation to determine if it is safe for Scala binary-compatibility to change/remove a method that is public in the bytecode.

Rewrote part of the doc

smarter · 2023-12-12T20:55:53Z

compiler/src/dotty/tools/dotc/inlines/PrepareInlineable.scala

+
+      protected def checkUnstableAccessor(accessedTree: Tree, accessor: Symbol)(using Context): Unit =
+        if ctx.settings.WunstableInlineAccessors.value then
+          val accessorTree = accessorDef(accessor, accessedTree.symbol)


Something I'm now wondering: if this annotation doesn't in fact change the bytecode we generate, then why do we bother going through the inline accessor when calling non-annotated protected/package-private methods? Couldn't we always make a direct call even if the method isn't annotated?

We need the annotation as a way to tell MiMa that this protected/package-private method can be used from outside the scope where it is defined.

We might be able to do this optimization once we have established -WunstableInlineAccessors and can break the old accessors. In this case we would still need to warn if that definition is not annotated with @publicInBinary.

We could start using some of those package-private methods directly now but still generate the accessor. I will look into it after this PR and after the 3.4 release.

nicolasstucki · 2023-12-18T08:59:02Z

The commit (9789d51) that changes the overriding spec is causing failures in akka-actors. I cannot debug this as these tests cannot be executed on a Mac M1 yet. @smarter do you know what could be causing this in 9789d51?

[error] -- Error: /__w/dotty/dotty/community-build/community-projects/akka/akka-actor-tests/src/test/java/akka/actor/ActorCreationTest.java:53:18 
[error] 53 |    public Object create() {
[error]    |                  ^
[error]    |bad parameter reference G.this.T at typer
[error]    |the parameter is type T in trait Creator but the prefix (G.this : akka.actor.ActorCreationTest.G)
[error]    |does not define any corresponding arguments.
[error]    |idx = 0, args = ,
[error]    |constraint =  uninstantiated variables:
[error]    | constrained types:
[error]    | bounds:
[error]    | ordering:
[error]    | co-deps:
[error]    | contra-deps:

smarter · 2023-12-18T16:36:02Z

No clue, but instead of doing this in Typer, I would recommend adding a case else other.hasAnnotation(...) to checkOverride in RefChecks.

Introduces [SIP-52](scala/improvement-proposals#58) as experimental feature. 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. 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. 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`.

nicolasstucki · 2023-12-19T07:25:17Z

In the Dotty meeting, we decided to not include 9789d51 in this PR. It is now in #19296. Ideally we should also merge the other PR before releasing 3.4.0-RC1, but if we do not manage, we will include it in 3.4.0-RC2.

We managed to fix it for 3.4.0-RC1

Followup of #18402 (see #18402 (comment)). This was split from the main PR due to the blocker described in #18402 (comment). During the Dotty meeting, we decided to merge #18402 without the second commit ([Require @publicInBinary on overrides](34f3dee)). If we manage to fix this issue before 3.4.0-RC1 we will include it there, otherwise it will be part of 3.4.0-RC2.

* Change suggestion scala/scala3#18402 (comment) * Changed in scala/scala3@eae8831

nicolasstucki self-assigned this Aug 15, 2023

nicolasstucki force-pushed the add-binaryAPI-annot branch 7 times, most recently from 138aae3 to b579c5a Compare August 16, 2023 08:32

nicolasstucki changed the title ~~Add @binaryAPI annotation~~ Add @binaryAPI annotation and -WunstableInlineAccessors linting flag Aug 16, 2023

nicolasstucki mentioned this pull request Aug 16, 2023

SIP-52 - Binary APIs scala/improvement-proposals#58

Merged

nicolasstucki added the needs-minor-release This PR cannot be merged until the next minor release label Aug 16, 2023

nicolasstucki force-pushed the add-binaryAPI-annot branch from b579c5a to a696e00 Compare August 16, 2023 11:15

nicolasstucki commented Aug 16, 2023

View reviewed changes

tests/neg/binaryAPI-not-accessible.check Outdated Show resolved Hide resolved

nicolasstucki force-pushed the add-binaryAPI-annot branch from a696e00 to addd6c8 Compare August 24, 2023 07:18

nicolasstucki changed the title ~~Add @binaryAPI annotation and -WunstableInlineAccessors linting flag~~ Add @publicInBinary annotation and -WunstableInlineAccessors linting flag Aug 24, 2023

nicolasstucki added the stat:do not merge label Aug 24, 2023

nicolasstucki force-pushed the add-binaryAPI-annot branch 3 times, most recently from c203441 to 1327818 Compare October 26, 2023 08:06

nicolasstucki removed the stat:do not merge label Oct 26, 2023

nicolasstucki added this to the 3.4.0 milestone Oct 26, 2023

nicolasstucki marked this pull request as ready for review October 26, 2023 08:28

nicolasstucki force-pushed the add-binaryAPI-annot branch 2 times, most recently from 44bd552 to a077c15 Compare October 26, 2023 10:04

nicolasstucki commented Oct 26, 2023

View reviewed changes

compiler/src/dotty/tools/dotc/transform/PostTyper.scala Show resolved Hide resolved

nicolasstucki force-pushed the add-binaryAPI-annot branch 2 times, most recently from 8095426 to e029796 Compare October 26, 2023 11:37

nicolasstucki added the release-notes Should be mentioned in the release notes label Oct 26, 2023

nicolasstucki requested a review from smarter October 26, 2023 12:59

nicolasstucki assigned smarter Oct 26, 2023

smarter removed their assignment Dec 11, 2023

nicolasstucki force-pushed the add-binaryAPI-annot branch 3 times, most recently from eb7e18d to 6ce6ab4 Compare December 12, 2023 14:23

nicolasstucki assigned smarter and unassigned nicolasstucki Dec 12, 2023

nicolasstucki requested a review from smarter December 12, 2023 16:06

smarter requested changes Dec 12, 2023

View reviewed changes

smarter assigned nicolasstucki and unassigned smarter Dec 12, 2023

nicolasstucki force-pushed the add-binaryAPI-annot branch 2 times, most recently from 2f525de to 1b3cc3a Compare December 14, 2023 09:01

nicolasstucki assigned smarter and unassigned nicolasstucki Dec 14, 2023

nicolasstucki requested a review from smarter December 14, 2023 09:04

smarter approved these changes Dec 14, 2023

View reviewed changes

nicolasstucki enabled auto-merge December 14, 2023 14:24

nicolasstucki force-pushed the add-binaryAPI-annot branch from 1b3cc3a to e5e3dcf Compare December 15, 2023 10:42

nicolasstucki force-pushed the add-binaryAPI-annot branch from 9789d51 to 583d5ae Compare December 19, 2023 07:17

nicolasstucki mentioned this pull request Dec 19, 2023

@publicInBinary override spec change #19296

Merged

nicolasstucki merged commit 9af39e3 into scala:main Dec 19, 2023
17 checks passed

nicolasstucki deleted the add-binaryAPI-annot branch December 19, 2023 08:36

nicolasstucki added a commit to dotty-staging/improvement-proposals that referenced this pull request Jan 9, 2024

Change overriding rules for @publicInBinary

604c9ec

* Change suggestion scala/scala3#18402 (comment) * Changed in scala/scala3@eae8831

nicolasstucki mentioned this pull request Jan 9, 2024

Update SIP-52 scala/improvement-proposals#71

Merged

odersky pushed a commit to odersky/improvement-proposals that referenced this pull request Jul 18, 2024

Change overriding rules for @publicInBinary

103bca2

* Change suggestion scala/scala3#18402 (comment) * Changed in scala/scala3@eae8831

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `@publicInBinary` annotation and `-WunstableInlineAccessors` linting flag #18402

Add `@publicInBinary` annotation and `-WunstableInlineAccessors` linting flag #18402

nicolasstucki commented Aug 15, 2023 •

edited

Loading

smarter Dec 12, 2023

smarter Dec 12, 2023

nicolasstucki Dec 14, 2023

smarter Dec 12, 2023

nicolasstucki Dec 14, 2023

smarter Dec 12, 2023

nicolasstucki Dec 14, 2023

smarter Dec 12, 2023

nicolasstucki Dec 14, 2023

nicolasstucki Dec 14, 2023

nicolasstucki commented Dec 18, 2023 •

edited

Loading

smarter commented Dec 18, 2023

nicolasstucki commented Dec 19, 2023 •

edited

Loading

Add @publicInBinary annotation and -WunstableInlineAccessors linting flag #18402

Add @publicInBinary annotation and -WunstableInlineAccessors linting flag #18402

Conversation

nicolasstucki commented Aug 15, 2023 • edited Loading

@publicInBinary

Interaction with inline

-WunstableInlineAccessors

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki commented Dec 18, 2023 • edited Loading

smarter commented Dec 18, 2023

nicolasstucki commented Dec 19, 2023 • edited Loading

Add `@publicInBinary` annotation and `-WunstableInlineAccessors` linting flag #18402

Add `@publicInBinary` annotation and `-WunstableInlineAccessors` linting flag #18402

nicolasstucki commented Aug 15, 2023 •

edited

Loading

`@publicInBinary`

`-WunstableInlineAccessors`

nicolasstucki commented Dec 18, 2023 •

edited

Loading

nicolasstucki commented Dec 19, 2023 •

edited

Loading