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 Dec 14, 2023
1 parent c88c0fe commit e5e3dcf
Show file tree
Hide file tree
Showing 33 changed files with 1,289 additions and 17 deletions.
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 @@ -166,6 +166,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
3 changes: 3 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1060,6 +1060,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 All @@ -1071,6 +1072,8 @@ class Definitions {
// A list of meta-annotations that are relevant for fields and accessors
@tu lazy val NonBeanMetaAnnots: Set[Symbol] =
Set(FieldMetaAnnot, GetterMetaAnnot, ParamMetaAnnot, SetterMetaAnnot, CompanionClassMetaAnnot, CompanionMethodMetaAnnot)
@tu lazy val NonBeanParamAccessorAnnots: Set[Symbol] =
Set(PublicInBinaryAnnot)
@tu lazy val MetaAnnots: Set[Symbol] =
NonBeanMetaAnnots + BeanGetterMetaAnnot + BeanSetterMetaAnnot

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 @@ -22,6 +22,8 @@ import staging.CrossStageSafety
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 @@ -71,6 +73,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 @@ -86,6 +89,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 @@ -100,7 +108,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 @@ -179,6 +191,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 @@ -121,14 +121,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 @@ -3087,3 +3087,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 @@ -31,7 +31,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 @@ -41,7 +45,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 @@ -53,7 +57,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 @@ -148,7 +153,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
3 changes: 3 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/PostTyper.scala
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,10 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
if sym.is(Param) then
sym.keepAnnotationsCarrying(thisPhase, Set(defn.ParamMetaAnnot), orNoneOf = defn.NonBeanMetaAnnots)
else if sym.is(ParamAccessor) then
// @publicInBinary is not a meta-annotation and therefore not kept by `keepAnnotationsCarrying`
val publicInBinaryAnnotOpt = sym.getAnnotation(defn.PublicInBinaryAnnot)
sym.keepAnnotationsCarrying(thisPhase, Set(defn.GetterMetaAnnot, defn.FieldMetaAnnot))
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
Expand Up @@ -37,7 +37,7 @@ object ProtectedAccessors {
* is not in a subclass or subtrait of `sym`?
*/
def needsAccessorIfNotInSubclass(sym: Symbol)(using Context): Boolean =
sym.isTerm && sym.is(Protected) &&
sym.isTerm && sym.is(Protected) && !sym.hasPublicInBinary &&
!sym.owner.is(Trait) && // trait methods need to be handled specially, are currently always public
!insideBoundaryOf(sym)

Expand Down
6 changes: 6 additions & 0 deletions compiler/src/dotty/tools/dotc/typer/Checking.scala
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,12 @@ 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(ParamAccessor) && sym.is(Private) then fail(em"@publicInBinary cannot be non `val` constructor parameters")
else if sym.is(Private) && !sym.privateWithin.exists && !sym.isConstructor then fail(em"@publicInBinary cannot be used on private definitions\n\nConsider using `private[${sym.owner.name}]` or `protected` instead")
if (sym.hasAnnotation(defn.NativeAnnot)) {
if (!sym.is(Deferred))
fail(NativeMembersMayNotHaveImplementation(sym))
Expand Down
6 changes: 4 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,10 @@ 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 && tpd.enclosingInlineds.nonEmpty then tpe
else inaccessibleErrorType(tpe, superAccess, pos)
case _ => tpe

/** Return a potentially skolemized version of `qualTpe` to be used
* as a prefix when selecting `name`.
Expand Down
2 changes: 1 addition & 1 deletion compiler/test-resources/repl/i15493
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,4 @@ scala> Vector.unapplySeq(Vector(2))
val res35: scala.collection.SeqFactory.UnapplySeqWrapper[Int] = scala.collection.SeqFactory$UnapplySeqWrapper@df507bfd

scala> new scala.concurrent.duration.DurationInt(5)
val res36: scala.concurrent.duration.package.DurationInt = scala.concurrent.duration.package$DurationInt@5
val res36: scala.concurrent.duration.DurationInt = scala.concurrent.duration.package$DurationInt@5
Loading

0 comments on commit e5e3dcf

Please sign in to comment.