From 2ff3008c3f165f6bcd47aa150b4bc175801e1d70 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 22 Jun 2019 19:47:14 +0200 Subject: [PATCH 1/6] Change rules for exports to make them more useful for enums Two changes: - The forwarder alising a value definition gets a singleton type referring to the alias - Synthetic members are not exported by a wildcard export This means we can now safely write ``` enum MatchDegree { case NoMatch, ParamMatch, FullMatch } export MatchDegree._ ``` and export just the three enum values with the most precise types. (precision is necessary for exhaustiveness checks, among others). --- compiler/src/dotty/tools/dotc/typer/Namer.scala | 14 +++++++------- docs/docs/reference/enums/desugarEnums.md | 2 +- docs/docs/reference/other-new-features/export.md | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index 9a1f89ca7650..c29448d0abf0 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -976,12 +976,11 @@ class Namer { typer: Typer => fwdInfo(path.tpe.select(mbr.symbol), mbr.info), coord = span) else { - val maybeStable = if (mbr.symbol.isStableMember) StableRealizable else EmptyFlags - ctx.newSymbol( - cls, alias, - Exported | Method | Final | maybeStable | mbr.symbol.flags & RetainedExportFlags, - mbr.info.ensureMethodic, - coord = span) + val (maybeStable, mbrInfo) = + if (mbr.symbol.isStableMember) (StableRealizable, ExprType(path.tpe.select(mbr.symbol))) + else (EmptyFlags, mbr.info.ensureMethodic) + val mbrFlags = Exported | Method | Final | maybeStable | mbr.symbol.flags & RetainedExportFlags + ctx.newSymbol(cls, alias, mbrFlags, mbrInfo, coord = span) } val forwarderDef = if (forwarder.isType) tpd.TypeDef(forwarder.asType) @@ -1010,7 +1009,8 @@ class Namer { typer: Typer => } def addForwardersExcept(seen: List[TermName], span: Span): Unit = - for (mbr <- path.tpe.allMembers) { + for (mbr <- path.tpe.membersBasedOnFlags( + required = EmptyFlags, excluded = PrivateOrSynthetic)) { val alias = mbr.name.toTermName if (!seen.contains(alias)) addForwarder(alias, mbr, span) } diff --git a/docs/docs/reference/enums/desugarEnums.md b/docs/docs/reference/enums/desugarEnums.md index 19e2d2cc70dc..cf80da163399 100644 --- a/docs/docs/reference/enums/desugarEnums.md +++ b/docs/docs/reference/enums/desugarEnums.md @@ -157,7 +157,7 @@ map into case classes or vals. Non-generic enums `E` that define one or more singleton cases are called _enumerations_. Companion objects of enumerations define -the following additional members. +the following additional synthetic members. - A method `valueOf(name: String): E`. It returns the singleton case value whose `toString` representation is `name`. diff --git a/docs/docs/reference/other-new-features/export.md b/docs/docs/reference/other-new-features/export.md index 147626168cb0..4d0effacc2e0 100644 --- a/docs/docs/reference/other-new-features/export.md +++ b/docs/docs/reference/other-new-features/export.md @@ -54,7 +54,7 @@ of one of the following forms: - An _omitting selector_ `x => _` prevents `x` from being aliased by a subsequent wildcard selector. - A _wildcard selector_ creates aliases for all eligible members of `path` except for - those members that are named by a previous simple, renaming, or omitting selector. + synthetic members generated by the compiler and those members that are named by a previous simple, renaming, or omitting selector. A member is _eligible_ if all of the following holds: @@ -75,7 +75,7 @@ Export aliases are always `final`. Aliases of delegates are again defines as del not marked `override`. - However, export aliases can implement deferred members of base classes. -Export aliases for value definitions are marked by the compiler as "stable". This means +Export aliases for value definitions are marked by the compiler as "stable" and their result types are the singleton types of the aliased definitions. This means that they can be used as parts of stable identifier paths, even though they are technically methods. For instance, the following is OK: ```scala class C { type T } From 03e1aee98b507b8200cd898608b22b4d58114882 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 22 Jun 2019 20:06:29 +0200 Subject: [PATCH 2/6] Make values/valueOf methods synthetic members --- compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala index 59dd65fef043..e70ed1a5cad2 100644 --- a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala +++ b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala @@ -95,6 +95,7 @@ object DesugarEnums { private def enumScaffolding(implicit ctx: Context): List[Tree] = { val valuesDef = DefDef(nme.values, Nil, Nil, TypeTree(), Select(valuesDot(nme.values), nme.toArray)) + .withFlags(Synthetic) val privateValuesDef = ValDef(nme.DOLLAR_VALUES, TypeTree(), New(TypeTree(defn.EnumValuesType.appliedTo(enumClass.typeRef :: Nil)), ListOfNil)) @@ -114,6 +115,7 @@ object DesugarEnums { ) val valueOfDef = DefDef(nme.valueOf, Nil, List(param(nme.nameDollar, defn.StringType) :: Nil), TypeTree(), valuesOfBody) + .withFlags(Synthetic) valuesDef :: privateValuesDef :: From f11239eeb49f531e557b3ea9fa19cf526e559973 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 5 Jul 2019 13:33:11 +0200 Subject: [PATCH 3/6] Check exports for private leaks --- .../tools/dotc/core/tasty/TreeUnpickler.scala | 2 +- .../src/dotty/tools/dotc/typer/Checking.scala | 4 ++-- .../src/dotty/tools/dotc/typer/Namer.scala | 5 +++-- .../dotty/tools/dotc/typer/TypeAssigner.scala | 4 ++-- tests/neg/export-leaks.scala | 22 +++++++++++++++++++ tests/pos/export-enum.scala | 15 +++++++++++++ 6 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 tests/neg/export-leaks.scala create mode 100644 tests/pos/export-enum.scala diff --git a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala index 98132e6d4bcd..9fae3f855669 100644 --- a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala +++ b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala @@ -844,7 +844,7 @@ class TreeUnpickler(reader: TastyReader, goto(end) setSpan(start, tree) if (!sym.isType) { // Only terms might have leaky aliases, see the documentation of `checkNoPrivateLeaks` - sym.info = ta.avoidPrivateLeaks(sym, tree.sourcePos) + sym.info = ta.avoidPrivateLeaks(sym) } if (ctx.mode.is(Mode.ReadComments)) { diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index 8bb262a98494..d70a013f5f43 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -470,7 +470,7 @@ object Checking { * * @return The `info` of `sym`, with problematic aliases expanded away. */ - def checkNoPrivateLeaks(sym: Symbol, pos: SourcePosition)(implicit ctx: Context): Type = { + def checkNoPrivateLeaks(sym: Symbol)(implicit ctx: Context): Type = { class NotPrivate extends TypeMap { var errors: List[() => String] = Nil @@ -531,7 +531,7 @@ object Checking { } val notPrivate = new NotPrivate val info = notPrivate(sym.info) - notPrivate.errors.foreach(error => ctx.errorOrMigrationWarning(error(), pos)) + notPrivate.errors.foreach(error => ctx.errorOrMigrationWarning(error(), sym.sourcePos)) info } diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index c29448d0abf0..a44240094313 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -873,7 +873,7 @@ class Namer { typer: Typer => denot.info = typeSig(sym) invalidateIfClashingSynthetic(denot) Checking.checkWellFormed(sym) - denot.info = avoidPrivateLeaks(sym, sym.sourcePos) + denot.info = avoidPrivateLeaks(sym) } } @@ -982,6 +982,7 @@ class Namer { typer: Typer => val mbrFlags = Exported | Method | Final | maybeStable | mbr.symbol.flags & RetainedExportFlags ctx.newSymbol(cls, alias, mbrFlags, mbrInfo, coord = span) } + forwarder.info = avoidPrivateLeaks(forwarder) val forwarderDef = if (forwarder.isType) tpd.TypeDef(forwarder.asType) else { @@ -1146,7 +1147,7 @@ class Namer { typer: Typer => Checking.checkWellFormed(cls) if (isDerivedValueClass(cls)) cls.setFlag(Final) - cls.info = avoidPrivateLeaks(cls, cls.sourcePos) + cls.info = avoidPrivateLeaks(cls) cls.baseClasses.foreach(_.invalidateBaseTypeCache()) // we might have looked before and found nothing cls.setNoInitsFlags(parentsKind(parents), bodyKind(rest)) if (cls.isNoInitsClass) cls.primaryConstructor.setFlag(StableRealizable) diff --git a/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala b/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala index 96c84adb2b67..d850c410a2dd 100644 --- a/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala +++ b/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala @@ -155,8 +155,8 @@ trait TypeAssigner { def avoidingType(expr: Tree, bindings: List[Tree])(implicit ctx: Context): Type = avoid(expr.tpe, localSyms(bindings).filter(_.isTerm)) - def avoidPrivateLeaks(sym: Symbol, pos: SourcePosition)(implicit ctx: Context): Type = - if (!sym.isOneOf(PrivateOrSynthetic) && sym.owner.isClass) checkNoPrivateLeaks(sym, pos) + def avoidPrivateLeaks(sym: Symbol)(implicit ctx: Context): Type = + if (!sym.isOneOf(PrivateOrSynthetic) && sym.owner.isClass) checkNoPrivateLeaks(sym) else sym.info private def toRepeated(tree: Tree, from: ClassSymbol)(implicit ctx: Context): Tree = diff --git a/tests/neg/export-leaks.scala b/tests/neg/export-leaks.scala new file mode 100644 index 000000000000..10d54685a41e --- /dev/null +++ b/tests/neg/export-leaks.scala @@ -0,0 +1,22 @@ +// Check that exports do not leak private types +object Signature { + + private type T + + object O1 { + private[Signature] def bar: T = ??? + } + export O1._ // error: non-private method bar refers to private type T + + object O2 { + private[Signature] val foo: T = ??? + } + export O2._ // OK + // The reason this works is that private escape checking only looks at real private members, + // not at private[C] members. So, by itself the expansion of the export + // def foo: O2.foo.type = O2.foo + // is legal. The underlying type of `O2.foo.type` does violate no-escape rules, but we do not + // check for it. Maybe we should. But then the question comes up whether we should + // also check all possible supertypes of a type for privacy violations. These are more + // general questions that are not related to exports. +} \ No newline at end of file diff --git a/tests/pos/export-enum.scala b/tests/pos/export-enum.scala new file mode 100644 index 000000000000..c0830e1c70fc --- /dev/null +++ b/tests/pos/export-enum.scala @@ -0,0 +1,15 @@ +object Signature { + + enum MatchDegree { + case NoMatch, ParamMatch, FullMatch + } + export MatchDegree._ + + // Check that exported values have singeleton types + val x: MatchDegree.NoMatch.type = NoMatch + + // Check that the following two methods are not exported. + // Exporting them would lead to a double definition. + def values: Array[MatchDegree] = ??? + def valueOf($name: String): MatchDegree = ??? +} From 19d96eb7ae6ee5921722161cbc1fa8da740d165f Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 8 Jul 2019 14:02:19 +0200 Subject: [PATCH 4/6] Generalize leak avoidance Leaks can be avoided by following type aliases. Previously, this was only done if the checked symbol is a term. But it is not clear why types should not get the same treatment. This avoids a problem in pos/reference/exports.scala. --- compiler/src/dotty/tools/dotc/typer/Checking.scala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index d70a013f5f43..eddff408ff6f 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -499,11 +499,12 @@ object Checking { var tp1 = if (isLeaked(tp.symbol)) { errors = - (() => em"non-private $sym refers to private ${tp.symbol}\nin its type signature ${sym.info}") :: errors + (() => em"non-private ${sym.showLocated} refers to private ${tp.symbol}\nin its type signature ${sym.info}") :: + errors tp } else mapOver(tp) - if ((errors ne prevErrors) && !sym.isType && tp.info.isTypeAlias) { + if ((errors ne prevErrors) && tp.info.isTypeAlias) { // try to dealias to avoid a leak error val savedErrors = errors errors = prevErrors From 5e8258138de6705787312666c4dd7bebf25d313e Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 8 Jul 2019 14:03:35 +0200 Subject: [PATCH 5/6] Refine export types We export values under their singleton types only if the original value was already public. Otherwise this would provoke a leak. --- .../src/dotty/tools/dotc/core/SymDenotations.scala | 3 +++ compiler/src/dotty/tools/dotc/typer/Namer.scala | 6 ++++-- docs/docs/reference/other-new-features/export.md | 3 +-- tests/neg/export-leaks.scala | 10 +--------- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala index df91418e7979..19f7ee238d99 100644 --- a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala +++ b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala @@ -1251,6 +1251,9 @@ object SymDenotations { else defn.RootClass } + final def isPublic(implicit ctx: Context): Boolean = + accessBoundary(owner) == defn.RootClass + /** The primary constructor of a class or trait, NoSymbol if not applicable. */ def primaryConstructor(implicit ctx: Context): Symbol = NoSymbol diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index a44240094313..9f2a64d555f8 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -977,8 +977,10 @@ class Namer { typer: Typer => coord = span) else { val (maybeStable, mbrInfo) = - if (mbr.symbol.isStableMember) (StableRealizable, ExprType(path.tpe.select(mbr.symbol))) - else (EmptyFlags, mbr.info.ensureMethodic) + if (mbr.symbol.isStableMember && mbr.symbol.isPublic) + (StableRealizable, ExprType(path.tpe.select(mbr.symbol))) + else + (EmptyFlags, mbr.info.ensureMethodic) val mbrFlags = Exported | Method | Final | maybeStable | mbr.symbol.flags & RetainedExportFlags ctx.newSymbol(cls, alias, mbrFlags, mbrInfo, coord = span) } diff --git a/docs/docs/reference/other-new-features/export.md b/docs/docs/reference/other-new-features/export.md index 4d0effacc2e0..cefc43683dab 100644 --- a/docs/docs/reference/other-new-features/export.md +++ b/docs/docs/reference/other-new-features/export.md @@ -75,8 +75,7 @@ Export aliases are always `final`. Aliases of delegates are again defines as del not marked `override`. - However, export aliases can implement deferred members of base classes. -Export aliases for value definitions are marked by the compiler as "stable" and their result types are the singleton types of the aliased definitions. This means -that they can be used as parts of stable identifier paths, even though they are technically methods. For instance, the following is OK: +Export aliases for public value definitions are marked by the compiler as "stable" and their result types are the singleton types of the aliased definitions. This means that they can be used as parts of stable identifier paths, even though they are technically methods. For instance, the following is OK: ```scala class C { type T } object O { val c: C = ... } diff --git a/tests/neg/export-leaks.scala b/tests/neg/export-leaks.scala index 10d54685a41e..0c35e1021345 100644 --- a/tests/neg/export-leaks.scala +++ b/tests/neg/export-leaks.scala @@ -11,12 +11,4 @@ object Signature { object O2 { private[Signature] val foo: T = ??? } - export O2._ // OK - // The reason this works is that private escape checking only looks at real private members, - // not at private[C] members. So, by itself the expansion of the export - // def foo: O2.foo.type = O2.foo - // is legal. The underlying type of `O2.foo.type` does violate no-escape rules, but we do not - // check for it. Maybe we should. But then the question comes up whether we should - // also check all possible supertypes of a type for privacy violations. These are more - // general questions that are not related to exports. -} \ No newline at end of file + export O2._ // error: non-private method foo refers to private type T \ No newline at end of file From 6edfe8313374fdf877ca87dd9a77ade62dfce7c2 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 8 Jul 2019 14:35:35 +0200 Subject: [PATCH 6/6] Fix test --- tests/neg/export-leaks.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/neg/export-leaks.scala b/tests/neg/export-leaks.scala index 0c35e1021345..fcc63b0781fc 100644 --- a/tests/neg/export-leaks.scala +++ b/tests/neg/export-leaks.scala @@ -11,4 +11,5 @@ object Signature { object O2 { private[Signature] val foo: T = ??? } - export O2._ // error: non-private method foo refers to private type T \ No newline at end of file + export O2._ // error: non-private method foo refers to private type T +} \ No newline at end of file