Skip to content

Commit

Permalink
Warn if inline accessors are generated
Browse files Browse the repository at this point in the history
  • Loading branch information
nicolasstucki committed Mar 23, 2023
1 parent 6ff1ffa commit f3636b5
Show file tree
Hide file tree
Showing 27 changed files with 312 additions and 83 deletions.
10 changes: 6 additions & 4 deletions compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import NameKinds.AvoidNameKind
import util.SimpleIdentitySet
import NullOpsDecorator.stripNull

import scala.annotation.binaryAPI

/** Methods for adding constraints and solving them.
*
* What goes into a Constraint as opposed to a ConstrainHandler?
Expand All @@ -39,12 +41,12 @@ trait ConstraintHandling {
private var addConstraintInvocations = 0

/** If the constraint is frozen we cannot add new bounds to the constraint. */
protected var frozenConstraint: Boolean = false
@binaryAPI protected var frozenConstraint: Boolean = false

/** Potentially a type lambda that is still instantiatable, even though the constraint
* is generally frozen.
*/
protected var caseLambda: Type = NoType
@binaryAPI protected var caseLambda: Type = NoType

/** If set, align arguments `S1`, `S2`when taking the glb
* `T1 { X = S1 } & T2 { X = S2 }` of a constraint upper bound for some type parameter.
Expand All @@ -56,7 +58,7 @@ trait ConstraintHandling {
/** We are currently comparing type lambdas. Used as a flag for
* optimization: when `false`, no need to do an expensive `pruneLambdaParams`
*/
protected var comparedTypeLambdas: Set[TypeLambda] = Set.empty
@binaryAPI protected var comparedTypeLambdas: Set[TypeLambda] = Set.empty

/** Used for match type reduction: If false, we don't recognize an abstract type
* to be a subtype type of any of its base classes. This is in place only at the
Expand Down Expand Up @@ -110,7 +112,7 @@ trait ConstraintHandling {
* of `1`. So the lower bound is `1 | x.M` and when we level-avoid that we
* get `1 | Int & String`, which simplifies to `Int`.
*/
private var myTrustBounds = true
@binaryAPI private var myTrustBounds = true

inline def withUntrustedBounds(op: => Type): Type =
val saved = myTrustBounds
Expand Down
6 changes: 4 additions & 2 deletions compiler/src/dotty/tools/dotc/core/Contexts.scala
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import classfile.ReusableDataReader
import StdNames.nme
import compiletime.uninitialized

import scala.annotation.binaryAPI
import scala.annotation.internal.sharable

import DenotTransformers.DenotTransformer
Expand Down Expand Up @@ -799,6 +800,7 @@ object Contexts {
* Note: plain TypeComparers always take on the kind of the outer comparer if they are in the same context.
* In other words: tracking or explaining is a sticky property in the same context.
*/
@binaryAPI
private def comparer(using Context): TypeComparer =
util.Stats.record("comparing")
val base = ctx.base
Expand Down Expand Up @@ -974,7 +976,7 @@ object Contexts {
private[core] var phasesPlan: List[List[Phase]] = uninitialized

/** Phases by id */
private[dotc] var phases: Array[Phase] = uninitialized
@binaryAPI private[dotc] var phases: Array[Phase] = uninitialized

/** Phases with consecutive Transforms grouped into a single phase, Empty array if fusion is disabled */
private[core] var fusedPhases: Array[Phase] = Array.empty[Phase]
Expand Down Expand Up @@ -1013,7 +1015,7 @@ object Contexts {
val generalContextPool = ContextPool()

private[Contexts] val comparers = new mutable.ArrayBuffer[TypeComparer]
private[Contexts] var comparersInUse: Int = 0
@binaryAPI private[Contexts] var comparersInUse: Int = 0

private var charArray = new Array[Char](256)

Expand Down
10 changes: 6 additions & 4 deletions compiler/src/dotty/tools/dotc/core/TypeComparer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import reporting.trace
import annotation.constructorOnly
import cc.{CapturingType, derivedCapturingType, CaptureSet, stripCapturing, isBoxedCapturing, boxed, boxedUnlessFun, boxedIfTypeParam, isAlwaysPure}

import scala.annotation.binaryAPI

/** Provides methods to compare types.
*/
class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling, PatternTypeConstrainer {
Expand All @@ -34,7 +36,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling
private var myContext: Context = initctx
def comparerContext: Context = myContext

protected given [DummySoItsADef]: Context = myContext
@binaryAPI protected given [DummySoItsADef]: Context = myContext

protected var state: TyperState = compiletime.uninitialized
def constraint: Constraint = state.constraint
Expand Down Expand Up @@ -115,7 +117,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling

private def isBottom(tp: Type) = tp.widen.isRef(NothingClass)

protected def gadtBounds(sym: Symbol)(using Context) = ctx.gadt.bounds(sym)
@binaryAPI protected def gadtBounds(sym: Symbol)(using Context) = ctx.gadt.bounds(sym)
protected def gadtAddBound(sym: Symbol, b: Type, isUpper: Boolean): Boolean = ctx.gadtState.addBound(sym, b, isUpper)

protected def typeVarInstance(tvar: TypeVar)(using Context): Type = tvar.underlying
Expand Down Expand Up @@ -156,7 +158,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling
private [this] var leftRoot: Type | Null = null

/** Are we forbidden from recording GADT constraints? */
private var frozenGadt = false
@binaryAPI private var frozenGadt = false
private inline def inFrozenGadt[T](inline op: T): T =
inFrozenGadtIf(true)(op)

Expand Down Expand Up @@ -187,7 +189,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling
inFrozenGadtIf(true)(inFrozenConstraint(op))

extension (sym: Symbol)
private inline def onGadtBounds(inline op: TypeBounds => Boolean): Boolean =
@binaryAPI private inline def onGadtBounds(inline op: TypeBounds => Boolean): Boolean =
val bounds = gadtBounds(sym)
bounds != null && op(bounds)

Expand Down
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import compiletime.uninitialized
import cc.{CapturingType, CaptureSet, derivedCapturingType, isBoxedCapturing, EventuallyCapturingType, boxedUnlessFun}
import CaptureSet.{CompareResult, IdempotentCaptRefMap, IdentityCaptRefMap}

import scala.annotation.binaryAPI
import scala.annotation.internal.sharable
import scala.annotation.threadUnsafe

Expand Down Expand Up @@ -5552,7 +5553,7 @@ object Types {

/** Common base class of TypeMap and TypeAccumulator */
abstract class VariantTraversal:
protected[dotc] var variance: Int = 1
@binaryAPI protected[dotc] var variance: Int = 1

inline protected def atVariance[T](v: Int)(op: => T): T = {
val saved = variance
Expand Down
33 changes: 28 additions & 5 deletions compiler/src/dotty/tools/dotc/inlines/PrepareInlineable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import transform.{AccessProxies, Splicer}
import staging.CrossStageSafety
import transform.SymUtils.*
import config.Printers.inlining
import util.SrcPos
import util.Property
import staging.StagingLevel

Expand Down Expand Up @@ -74,7 +75,7 @@ object PrepareInlineable {
def needsAccessor(sym: Symbol)(using Context): Boolean =
sym.isTerm &&
(sym.isOneOf(AccessFlags) || sym.privateWithin.exists) &&
(!sym.isBinaryAPI || (sym.is(Private) && !sym.owner.is(Trait))) &&
(!sym.isBinaryAPI || sym.is(Private)) &&
!(sym.isStableMember && sym.info.widenTermRefExpr.isInstanceOf[ConstantType]) &&
!sym.isInlineMethod &&
(Inlines.inInlineMethod || StagingLevel.level > 0)
Expand Down Expand Up @@ -107,6 +108,22 @@ object PrepareInlineable {
case tree: Apply if tree.symbol.isQuote => StagingLevel.quoteContext
case tree: Apply if tree.symbol.isExprSplice => StagingLevel.spliceContext
case _ => ctx

protected def unstableAccessorWarning(accessor: Symbol, accessed: Symbol, srcPos: SrcPos)(using Context): Unit =
val accessorDefTree = accessorDef(accessor, accessed)
val solution =
if accessed.is(Private) then
s"Annotate ${accessed.name} with `@binaryAPI` to generate a stable accessor."
else
s"Annotate ${accessed.name} with `@binaryAPI` to make it accessible."
val binaryCompat =
s"""Adding @binaryAPI may break binary compatibility if a previous version of this
|library was compiled with Scala 3.0-3.3, Binary compatibility should be checked
|using MiMa. To keep binary you can add the following accessor to ${accessor.owner.showKind} ${accessor.owner.name.stripModuleClassSuffix}:
| @binaryAPI private[${accessor.owner.name.stripModuleClassSuffix}] ${accessorDefTree.show}
|
|""".stripMargin
report.warning(em"Generated unstable inline accessor for $accessed defined in ${accessed.owner}.\n\n$solution\n\n$binaryCompat", srcPos)
}

/** Direct approach: place the accessor with the accessed symbol. This has the
Expand All @@ -121,7 +138,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 accessorTree = useAccessor(tree)
if !tree.symbol.isBinaryAPI && tree.symbol != accessorTree.symbol then
unstableAccessorWarning(accessorTree.symbol, tree.symbol, tree.srcPos)
accessorTree
case _ =>
tree
}
Expand Down Expand Up @@ -206,7 +227,9 @@ object PrepareInlineable {
localRefs.map(TypeTree(_)) ++ leadingTypeArgs, // TODO: pass type parameters in two sections?
(qual :: Nil) :: otherArgss
)
ref(accessor).appliedToArgss(argss1).withSpan(tree.span)
val accessorTree = ref(accessor).appliedToArgss(argss1).withSpan(tree.span)

unstableAccessorWarning(accessorTree.symbol, tree.symbol, tree.srcPos)

// TODO: Handle references to non-public types.
// This is quite tricky, as such types can appear anywhere, including as parts
Expand All @@ -218,6 +241,7 @@ object PrepareInlineable {
// myAccessors += TypeDef(accessor).withPos(tree.pos.focus)
// ref(accessor).withSpan(tree.span)
//
accessorTree
case _: TypeDef if tree.symbol.is(Case) =>
report.error(reporting.CaseClassInInlinedCode(tree), tree)
tree
Expand All @@ -229,7 +253,7 @@ object PrepareInlineable {
/** Create an inline accessor for this definition. */
def makePrivateBinaryAPIAccessor(sym: Symbol)(using Context): Unit =
assert(sym.is(Private))
if !sym.owner.is(Trait) then
if !sym.is(Accessor) then
val ref = tpd.ref(sym).asInstanceOf[RefTree]
val accessor = InsertPrivateBinaryAPIAccessors.useAccessor(ref)
if sym.is(Mutable) then
Expand All @@ -250,7 +274,6 @@ object PrepareInlineable {
// so no accessors are needed for them.
tree
else
// TODO: warn if MakeInlineablePassing or MakeInlineableDirect generate accessors
new MakeInlineablePassing(inlineSym).transform(
new MakeInlineableDirect(inlineSym).transform(tree))
}
Expand Down
4 changes: 3 additions & 1 deletion compiler/src/dotty/tools/dotc/parsing/Scanners.scala
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import config.Feature.{migrateTo3, fewerBracesEnabled}
import config.SourceVersion.`3.0`
import reporting.{NoProfile, Profile, Message}

import scala.annotation.binaryAPI

import java.util.Objects

object Scanners {
Expand Down Expand Up @@ -1596,7 +1598,7 @@ object Scanners {
protected def coversIndent(w: IndentWidth): Boolean =
knownWidth != null && w == indentWidth

private var myCommasExpected: Boolean = false
@binaryAPI private var myCommasExpected: Boolean = false

inline def withCommasExpected[T](inline op: => T): T =
val saved = myCommasExpected
Expand Down
7 changes: 4 additions & 3 deletions compiler/src/dotty/tools/dotc/parsing/xml/MarkupParsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import Decorators.{em, toMessage}
import util.SourceFile
import Utility._

import scala.annotation.binaryAPI

// XXX/Note: many/most of the functions in here are almost direct cut and pastes
// from another file - scala.xml.parsing.MarkupParser, it looks like.
Expand Down Expand Up @@ -51,7 +52,7 @@ object MarkupParsers {
override def getMessage: String = "input ended while parsing XML"
}

class MarkupParser(parser: Parser, final val preserveWS: Boolean)(using Context) extends MarkupParserCommon {
class MarkupParser(@binaryAPI parser: Parser, final val preserveWS: Boolean)(using @binaryAPI c: Context) extends MarkupParserCommon {

import Tokens.{ LBRACE, RBRACE }

Expand Down Expand Up @@ -92,8 +93,8 @@ object MarkupParsers {
var xEmbeddedBlock: Boolean = false

private var debugLastStartElement = List.empty[(Int, String)]
private def debugLastPos = debugLastStartElement.head._1
private def debugLastElem = debugLastStartElement.head._2
@binaryAPI private def debugLastPos = debugLastStartElement.head._1
@binaryAPI private def debugLastElem = debugLastStartElement.head._2

private def errorBraces() = {
reportSyntaxError("in XML content, please use '}}' to express '}'")
Expand Down
4 changes: 3 additions & 1 deletion compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,13 @@ import dotty.tools.dotc.util.SourcePosition
import dotty.tools.dotc.ast.untpd.{MemberDef, Modifiers, PackageDef, RefTree, Template, TypeDef, ValOrDefDef}
import cc.{CaptureSet, toCaptureSet, IllegalCaptureRef}

import scala.annotation.binaryAPI

class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {

/** A stack of enclosing DefDef, TypeDef, or ClassDef, or ModuleDefs nodes */
private var enclosingDef: untpd.Tree = untpd.EmptyTree
private var myCtx: Context = super.printerContext
@binaryAPI private var myCtx: Context = super.printerContext
private var printPos = ctx.settings.YprintPos.value
private val printLines = ctx.settings.printLines.value

Expand Down
5 changes: 3 additions & 2 deletions compiler/src/dotty/tools/dotc/reporting/trace.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import core.*, Contexts.*, Decorators.*
import config.*
import printing.Formatting.*

import scala.annotation.binaryAPI
import scala.compiletime.*

/** Exposes the {{{ trace("question") { op } }}} syntax.
Expand Down Expand Up @@ -76,9 +77,9 @@ trait TraceSyntax:
inline def apply[T](inline question: String)(inline op: T)(using Context): T =
apply[T](question, false)(op)

private val alwaysToString = (x: Any) => String.valueOf(x)
@binaryAPI private val alwaysToString = (x: Any) => String.valueOf(x)

private def doTrace[T](question: => String,
@binaryAPI private def doTrace[T](question: => String,
printer: Printers.Printer = Printers.default,
showOp: T => String)
(op: => T)(using Context): T =
Expand Down
50 changes: 27 additions & 23 deletions compiler/src/dotty/tools/dotc/transform/AccessProxies.scala
Original file line number Diff line number Diff line change
Expand Up @@ -32,29 +32,33 @@ 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 => {
def numTypeParams = accessed.info match {
case info: PolyType => info.paramNames.length
case _ => 0
}
val (targs, argss) = splitArgs(prefss)
val (accessRef, forwardedTpts, forwardedArgss) =
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)),
targs, argss)
val rhs =
if (accessor.name.isSetterName &&
forwardedArgss.nonEmpty && forwardedArgss.head.nonEmpty) // defensive conditions
accessRef.becomes(forwardedArgss.head.head)
else
accessRef
.appliedToTypeTrees(forwardedTpts)
.appliedToArgss(forwardedArgss)
.etaExpandCFT(using ctx.withOwner(accessor))
rhs.withSpan(accessed.span)
})
accessorDef(accessor, accessed)

/** The accessor definitions that need to be added to class `cls` */
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
}
val (targs, argss) = splitArgs(prefss)
val (accessRef, forwardedTpts, forwardedArgss) =
if (passReceiverAsArg(accessor.name))
(argss.head.head.select(accessed), targs.takeRight(numTypeParams), argss.tail)
else
(if (accessed.isStatic) ref(accessed) else ref(TermRef(accessor.owner.thisType, accessed)),
targs, argss)
val rhs =
if (accessor.name.isSetterName &&
forwardedArgss.nonEmpty && forwardedArgss.head.nonEmpty) // defensive conditions
accessRef.becomes(forwardedArgss.head.head)
else
accessRef
.appliedToTypeTrees(forwardedTpts)
.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
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package dotty.tools.dotc
package transform
package sjs

import scala.annotation.binaryAPI
import scala.collection.mutable

import ast.tpd
Expand Down Expand Up @@ -1122,7 +1123,7 @@ object PrepJSInterop {
val name: String = "prepjsinterop"
val description: String = "additional checks and transformations for Scala.js"

private final class OwnerKind private (private val baseKinds: Int) extends AnyVal {
private final class OwnerKind private (@binaryAPI private val baseKinds: Int) extends AnyVal {

inline def isBaseKind: Boolean =
Integer.lowestOneBit(baseKinds) == baseKinds && baseKinds != 0 // exactly 1 bit on
Expand Down
Loading

0 comments on commit f3636b5

Please sign in to comment.