Skip to content

Commit

Permalink
Add @publicInBinary annotation and -WunstableInlineAccessors
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
nicolasstucki committed Oct 26, 2023
1 parent 8046a8b commit 1327818
Show file tree
Hide file tree
Showing 31 changed files with 1,195 additions and 17 deletions.
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ class Compiler {
new RefChecks) :: // Various checks mostly related to abstract members and overriding
List(new semanticdb.ExtractSemanticDB.AppendDiagnostics) :: // Attach warnings to extracted SemanticDB and write to .semanticdb file
List(new init.Checker) :: // Check initialization of objects
List(new ProtectedAccessors, // Add accessors for protected members
List(new PublicInBinary, // Makes @publicInBinary definitions public
new ProtectedAccessors, // Add accessors for protected members
new ExtensionMethods, // Expand methods of value classes with extension methods
new UncacheGivenAliases, // Avoid caching RHS of simple parameterless given aliases
new ElimByName, // Map by-name parameters to functions
Expand Down
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/config/ScalaSettings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ private sealed trait WarningSettings:
val WvalueDiscard: Setting[Boolean] = BooleanSetting("-Wvalue-discard", "Warn when non-Unit expression results are unused.")
val WNonUnitStatement = BooleanSetting("-Wnonunit-statement", "Warn when block statements are non-Unit expressions.")
val WimplausiblePatterns = BooleanSetting("-Wimplausible-patterns", "Warn if comparison with a pattern value looks like it might always fail.")
val WunstableInlineAccessors = BooleanSetting("-WunstableInlineAccessors", "Warn an inline methods has references to non-stable binary APIs.")
val Wunused: Setting[List[ChoiceWithHelp[String]]] = MultiChoiceHelpSetting(
name = "-Wunused",
helpArg = "warning",
Expand Down
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1056,6 +1056,7 @@ class Definitions {
@tu lazy val RequiresCapabilityAnnot: ClassSymbol = requiredClass("scala.annotation.internal.requiresCapability")
@tu lazy val RetainsAnnot: ClassSymbol = requiredClass("scala.annotation.retains")
@tu lazy val RetainsByNameAnnot: ClassSymbol = requiredClass("scala.annotation.retainsByName")
@tu lazy val PublicInBinaryAnnot: ClassSymbol = requiredClass("scala.annotation.publicInBinary")

@tu lazy val JavaRepeatableAnnot: ClassSymbol = requiredClass("java.lang.annotation.Repeatable")

Expand Down
7 changes: 7 additions & 0 deletions compiler/src/dotty/tools/dotc/core/SymDenotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,13 @@ object SymDenotations {
isOneOf(EffectivelyErased)
|| is(Inline) && !isRetainedInline && !hasAnnotation(defn.ScalaStaticAnnot)

/** Is this a member that will become public in the generated binary */
def hasPublicInBinary(using Context): Boolean =
isTerm && (
hasAnnotation(defn.PublicInBinaryAnnot) ||
allOverriddenSymbols.exists(sym => sym.hasAnnotation(defn.PublicInBinaryAnnot))
)

/** ()T and => T types should be treated as equivalent for this symbol.
* Note: For the moment, we treat Scala-2 compiled symbols as loose matching,
* because the Scala library does not always follow the right conventions.
Expand Down
16 changes: 15 additions & 1 deletion compiler/src/dotty/tools/dotc/inlines/PrepareInlineable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import transform.SymUtils.*
import config.Printers.inlining
import util.Property
import staging.StagingLevel
import dotty.tools.dotc.reporting.Message
import dotty.tools.dotc.util.SrcPos

object PrepareInlineable {
import tpd._
Expand Down Expand Up @@ -72,6 +74,7 @@ object PrepareInlineable {
sym.isTerm &&
(sym.isOneOf(AccessFlags) || sym.privateWithin.exists) &&
!sym.isContainedIn(inlineSym) &&
!sym.hasPublicInBinary &&
!(sym.isStableMember && sym.info.widenTermRefExpr.isInstanceOf[ConstantType]) &&
!sym.isInlineMethod &&
(Inlines.inInlineMethod || StagingLevel.level > 0)
Expand All @@ -87,6 +90,11 @@ object PrepareInlineable {

override def transform(tree: Tree)(using Context): Tree =
postTransform(super.transform(preTransform(tree)))

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

/** Direct approach: place the accessor with the accessed symbol. This has the
Expand All @@ -101,7 +109,11 @@ object PrepareInlineable {
report.error("Implementation restriction: cannot use private constructors in inline methods", tree.srcPos)
tree // TODO: create a proper accessor for the private constructor
}
else useAccessor(tree)
else
val accessor = useAccessor(tree)
if tree != accessor then
checkUnstableAccessor(tree, accessor.symbol)
accessor
case _ =>
tree
}
Expand Down Expand Up @@ -180,6 +192,8 @@ object PrepareInlineable {
accessorInfo = abstractQualType(addQualType(dealiasMap(accessedType))),
accessed = accessed)

checkUnstableAccessor(tree, accessor)

val (leadingTypeArgs, otherArgss) = splitArgs(argss)
val argss1 = joinArgs(
localRefs.map(TypeTree(_)) ++ leadingTypeArgs, // TODO: pass type parameters in two sections?
Expand Down
15 changes: 10 additions & 5 deletions compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,19 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
else if homogenizedView then isEmptyPrefix(sym) // drop <root> and anonymous classes, but not scala, Predef.
else if sym.isPackageObject then isOmittablePrefix(sym.owner)
else isOmittablePrefix(sym)
def isSkippedPackageObject(sym: Symbol) =
sym.isPackageObject && !homogenizedView && !printDebug

tp.prefix match {
case thisType: ThisType if isOmittable(thisType.cls) =>
""
case termRef @ TermRef(pre, _) =>
case thisType: ThisType =>
val sym = thisType.cls
if isSkippedPackageObject(sym) then toTextPrefixOf(sym.typeRef)
else if isOmittable(sym) then ""
else super.toTextPrefixOf(tp)
case termRef: TermRef =>
val sym = termRef.symbol
if sym.isPackageObject && !homogenizedView && !printDebug then toTextPrefixOf(termRef)
else if (isOmittable(sym)) ""
if isSkippedPackageObject(sym) then toTextPrefixOf(termRef)
else if isOmittable(sym) then ""
else super.toTextPrefixOf(tp)
case _ => super.toTextPrefixOf(tp)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
case VarArgsParamCannotBeGivenID // errorNumber: 188
case ExtractorNotFoundID // errorNumber: 189
case PureUnitExpressionID // errorNumber: 190
case UnstableInlineAccessorID // errorNumber: 191

def errorNumber = ordinal - 1

Expand Down
37 changes: 37 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3085,3 +3085,40 @@ class ImplausiblePatternWarning(pat: tpd.Tree, selType: Type)(using Context)
|$pat could match selector of type $selType
|only if there is an `equals` method identifying elements of the two types."""
def explain(using Context) = ""

class UnstableInlineAccessor(accessed: Symbol, accessorTree: tpd.Tree)(using Context)
extends Message(UnstableInlineAccessorID) {
def kind = MessageKind.Compatibility

def msg(using Context) =
i"""Unstable inline accessor ${accessor.name} was generated in $where."""

def explain(using Context) =
i"""Access to non-public $accessed 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
|$accessed is public in the binary API.
| * Option 1: Annotate $accessed with @publicInBinary
| * Option 2: Make $accessed 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 $where:
| @publicInBinary private[$within] ${accessorTree.show}
|"""

private def accessor = accessorTree.symbol

private def where =
if accessor.owner.name.isPackageObjectName then s"package ${within}"
else if accessor.owner.is(Module) then s"object $within"
else s"class $within"

private def within =
if accessor.owner.name.isPackageObjectName then accessor.owner.owner.name.stripModuleClassSuffix
else accessor.owner.name.stripModuleClassSuffix
}
13 changes: 9 additions & 4 deletions compiler/src/dotty/tools/dotc/transform/AccessProxies.scala
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ abstract class AccessProxies {
/** The accessor definitions that need to be added to class `cls` */
private def accessorDefs(cls: Symbol)(using Context): Iterator[DefDef] =
for accessor <- cls.info.decls.iterator; accessed <- accessedBy.get(accessor) yield
DefDef(accessor.asTerm, prefss => {
accessorDef(accessor, accessed)

protected def accessorDef(accessor: Symbol, accessed: Symbol)(using Context): DefDef =
DefDef(accessor.asTerm,
prefss => {
def numTypeParams = accessed.info match {
case info: PolyType => info.paramNames.length
case _ => 0
Expand All @@ -42,7 +46,7 @@ abstract class AccessProxies {
if (passReceiverAsArg(accessor.name))
(argss.head.head.select(accessed), targs.takeRight(numTypeParams), argss.tail)
else
(if (accessed.isStatic) ref(accessed) else ref(TermRef(cls.thisType, accessed)),
(if (accessed.isStatic) ref(accessed) else ref(TermRef(accessor.owner.thisType, accessed)),
targs, argss)
val rhs =
if (accessor.name.isSetterName &&
Expand All @@ -54,7 +58,8 @@ abstract class AccessProxies {
.appliedToArgss(forwardedArgss)
.etaExpandCFT(using ctx.withOwner(accessor))
rhs.withSpan(accessed.span)
})
}
)

/** Add all needed accessors to the `body` of class `cls` */
def addAccessorDefs(cls: Symbol, body: List[Tree])(using Context): List[Tree] = {
Expand Down Expand Up @@ -149,7 +154,7 @@ abstract class AccessProxies {
def accessorIfNeeded(tree: Tree)(using Context): Tree = tree match {
case tree: RefTree if needsAccessor(tree.symbol) =>
if (tree.symbol.isConstructor) {
report.error("Implementation restriction: cannot use private constructors in inlineable methods", tree.srcPos)
report.error("Cannot use private constructors in inline methods. You can use @publicInBinary to make constructor accessible in inline methods.", tree.srcPos)
tree // TODO: create a proper accessor for the private constructor
}
else useAccessor(tree)
Expand Down
5 changes: 4 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/PostTyper.scala
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,13 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
atPhase(thisPhase)(cls.annotationsCarrying(Set(defn.CompanionMethodMetaAnnot)))
++ sym.annotations)
else
val publicInBinaryAnnotOpt = sym.getAnnotation(defn.PublicInBinaryAnnot)
if sym.is(Param) then
sym.keepAnnotationsCarrying(thisPhase, Set(defn.ParamMetaAnnot), orNoneOf = defn.NonBeanMetaAnnots)
else if sym.is(ParamAccessor) then
sym.keepAnnotationsCarrying(thisPhase, Set(defn.GetterMetaAnnot, defn.FieldMetaAnnot))
// FIXME: copyAndKeepAnnotationsCarrying is dropping defn.PublicInBinaryAnnot
sym.keepAnnotationsCarrying(thisPhase, Set(defn.GetterMetaAnnot, defn.FieldMetaAnnot, defn.PublicInBinaryAnnot, defn.PublicInBinaryAnnot))
for publicInBinaryAnnot <- publicInBinaryAnnotOpt do sym.addAnnotation(publicInBinaryAnnot)
else
sym.keepAnnotationsCarrying(thisPhase, Set(defn.GetterMetaAnnot, defn.FieldMetaAnnot), orNoneOf = defn.NonBeanMetaAnnots)
if sym.isScala2Macro && !ctx.settings.XignoreScala2Macros.value then
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package dotty.tools.dotc.transform

import dotty.tools.dotc.core.Contexts.*
import dotty.tools.dotc.core.DenotTransformers.SymTransformer
import dotty.tools.dotc.core.Flags.*
import dotty.tools.dotc.core.Symbols.NoSymbol
import dotty.tools.dotc.core.SymDenotations.SymDenotation
import dotty.tools.dotc.transform.MegaPhase.MiniPhase
import dotty.tools.dotc.typer.RefChecks

/** Makes @publicInBinary definitions public.
*
* This makes it possible to elide the generations of some unnecessary accessors.
*/
class PublicInBinary extends MiniPhase with SymTransformer:

override def runsAfterGroupsOf: Set[String] = Set(RefChecks.name)

override def phaseName: String = PublicInBinary.name
override def description: String = PublicInBinary.description

def transformSym(d: SymDenotation)(using Context): SymDenotation = {
if d.hasPublicInBinary then
d.resetFlag(Protected)
d.setPrivateWithin(NoSymbol)
if d.is(Module) then
val moduleClass = d.moduleClass
moduleClass.resetFlag(Protected)
moduleClass.setPrivateWithin(NoSymbol)
d
}

object PublicInBinary:
val name: String = "publicInBinary"
val description: String = "makes @publicInBinary definitions public"
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/typer/Checking.scala
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,11 @@ object Checking {
fail(em"Inline methods cannot be @tailrec")
if sym.hasAnnotation(defn.TargetNameAnnot) && sym.isClass && sym.isTopLevelClass then
fail(TargetNameOnTopLevelClass(sym))
if sym.hasAnnotation(defn.PublicInBinaryAnnot) then
if sym.is(Enum) then fail(em"@publicInBinary cannot be used on enum definitions")
else if sym.isType && !sym.is(Module) && !(sym.is(Given) || sym.companionModule.is(Given)) then fail(em"@publicInBinary cannot be used on ${sym.showKind} definitions")
else if !sym.owner.isClass && !(sym.is(Param) && sym.owner.isConstructor) then fail(em"@publicInBinary cannot be used on local definitions")
else if sym.is(Private) && !sym.privateWithin.exists && !sym.isConstructor then fail(em"@publicInBinary cannot be used on private definitions\n\nCould the definition `private[${sym.owner.name}]` or `protected` instead")
if (sym.hasAnnotation(defn.NativeAnnot)) {
if (!sym.is(Deferred))
fail(NativeMembersMayNotHaveImplementation(sym))
Expand Down
5 changes: 3 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,9 @@ trait TypeAssigner {
val tpe1 = accessibleType(tpe, superAccess)
if tpe1.exists then tpe1
else tpe match
case tpe: NamedType => inaccessibleErrorType(tpe, superAccess, pos)
case NoType => tpe
case tpe: NamedType if !tpe.termSymbol.hasPublicInBinary =>
inaccessibleErrorType(tpe, superAccess, pos)
case _ => tpe

/** Return a potentially skolemized version of `qualTpe` to be used
* as a prefix when selecting `name`.
Expand Down
Loading

0 comments on commit 1327818

Please sign in to comment.