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

Add @publicInBinary annotation and -WunstableInlineAccessors linting flag #18402

Merged
merged 1 commit into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -1059,6 +1059,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 @@ -1070,6 +1071,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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead have a refcheck to enforce that overrides have the annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the SIP we ended up with

A binary API is a definition that is annotated with @publicInBinary or overrides a definition annotated with @publicInBinary.

We should follow this or amend this detail in the SIP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

)

/** ()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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 @@ -122,14 +122,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 @@ -205,6 +205,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
case ExtractorNotFoundID // errorNumber: 189
case PureUnitExpressionID // errorNumber: 190
case MatchTypeLegacyPatternID // errorNumber: 191
case UnstableInlineAccessorID // errorNumber: 192

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 @@ -3091,3 +3091,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))
smarter marked this conversation as resolved.
Show resolved Hide resolved
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
Loading