From 31a56077af5c5b35049fec456204e12a19bb6701 Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Tue, 28 Feb 2017 14:14:11 -0800 Subject: Improvements based on reviews by Lukas & Jason --- spec/05-classes-and-objects.md | 5 +-- .../scala/tools/nsc/typechecker/Namers.scala | 48 ++++++++++++++-------- .../scala/reflect/internal/tpe/FindMembers.scala | 14 +++++++ test/files/neg/userdefined_apply.check | 20 +++++++-- test/files/neg/userdefined_apply.scala | 26 ++++++++++++ test/files/pos/userdefined_apply.scala | 18 ++++++++ 6 files changed, 107 insertions(+), 24 deletions(-) diff --git a/spec/05-classes-and-objects.md b/spec/05-classes-and-objects.md index 246e579c94..ffb65979f7 100644 --- a/spec/05-classes-and-objects.md +++ b/spec/05-classes-and-objects.md @@ -876,10 +876,9 @@ If a type parameter section is missing in the class, it is also missing in the ` If the companion object $c$ is already defined, the `apply` and `unapply` methods are added to the existing object. +If the object $c$ already has a [matching](#definition-matching) +`apply` (or `unapply`) member, no new definition is added. The definition of `apply` is omitted if class $c$ is `abstract`. -If the object $c$ already defines a [matching](#definition-matching) member of the -same name as the synthetic member to be added, the synthetic member -is not added (overloading or mutual recursion is allowed, however). If the case class definition contains an empty value parameter list, the `unapply` method returns a `Boolean` instead of an `Option` type and diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala index 8c5f4590b9..51df750951 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala @@ -650,6 +650,8 @@ trait Namers extends MethodSynthesis { def applyUnapplyMethodCompleter(un_applyDef: DefDef, companionContext: Context): TypeCompleter = new CompleterWrapper(completerOf(un_applyDef)) { override def complete(sym: Symbol): Unit = { + assert(sym hasAllFlags CASE | SYNTHETIC, sym.defString) + super.complete(sym) // don't propagate e.g. @volatile annot to apply's argument @@ -658,23 +660,37 @@ trait Namers extends MethodSynthesis { sym.info.paramss.foreach(_.foreach(retainOnlyParamAnnots)) + // owner won't be locked + val ownerInfo = companionContext.owner.info + // If there's a same-named locked symbol, we're currently completing its signature. - // This means it (may) refer to us, and is thus either overloaded or recursive without a signature. - // rule out locked symbols from the owner.info.member call - val scopePartiallyCompleted = - companionContext.scope.lookupAll(sym.name).exists(existing => existing != sym && existing.hasFlag(LOCKED)) - - val suppress = - scopePartiallyCompleted || { - val userDefined = companionContext.owner.info.member(sym.name).filter(_ != sym) + // If `scopePartiallyCompleted`, the program is known to have a type error, since + // this means a user-defined method is missing a result type while its rhs refers to `sym` or an overload. + // This is an error because overloaded/recursive methods must have a result type. + // The method would be overloaded if its signature, once completed, would not match the synthetic method's, + // or recursive if it turned out we should unlink our synthetic method (matching sig). + // In any case, error out. We don't unlink the symbol so that `symWasOverloaded` says yes, + // which would be wrong if the method is in fact recursive, but it seems less confusing. + val scopePartiallyCompleted = new HasMember(ownerInfo, sym.name, BridgeFlags | SYNTHETIC, LOCKED).apply() + + // Check `scopePartiallyCompleted` first to rule out locked symbols from the owner.info.member call, + // as FindMember will call info on a locked symbol (while checking type matching to assemble an overloaded type), + // and throw a TypeError, so that we are aborted. + // Do not consider deferred symbols, as suppressing our concrete implementation would be an error regardless + // of whether the signature matches (if it matches, we omitted a valid implementation, if it doesn't, + // we would get an error for the missing implementation it isn't implemented by some overload other than our synthetic one) + val suppress = scopePartiallyCompleted || { + // can't exclude deferred members using DEFERRED flag here (TODO: why?) + val userDefined = ownerInfo.memberBasedOnName(sym.name, BridgeFlags | SYNTHETIC) + (userDefined != NoSymbol) && { - userDefined.info match { - // TODO: do we have something for this already? the synthetic symbol can't be overloaded, right? - case OverloadedType(pre, alternatives) => - // pre probably relevant because of inherited overloads? - alternatives.exists(_.isErroneous) || alternatives.exists(alt => pre.memberInfo(alt) matches pre.memberInfo(sym)) - case tp => - (tp eq ErrorType) || tp.matches(sym.info) + assert(userDefined != sym) + val alts = userDefined.alternatives // could be just the one, if this member isn't overloaded + // don't compute any further `memberInfo`s if there's an error somewhere + alts.exists(_.isErroneous) || { + val self = companionContext.owner.thisType + val memberInfo = self.memberInfo(sym) + alts.exists(alt => !alt.isDeferred && (self.memberInfo(alt) matches memberInfo)) } } } @@ -744,8 +760,6 @@ trait Namers extends MethodSynthesis { val bridgeFlag = if (mods hasAnnotationNamed tpnme.bridgeAnnot) BRIDGE | ARTIFACT else 0 val sym = assignAndEnterSymbol(tree) setFlag bridgeFlag - // copy/apply/unapply synthetics are added using the addIfMissing mechanism, - // which ensures the owner has its preliminary info (we may add another decl here) val completer = if (sym hasFlag SYNTHETIC) { if (name == nme.copy) copyMethodCompleter(tree) diff --git a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala index 6ba48cb44d..510d76793e 100644 --- a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala +++ b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala @@ -285,4 +285,18 @@ trait FindMembers { initBaseClasses.head.newOverloaded(tpe, members) } } + + private[scala] final class HasMember(tpe: Type, name: Name, excludedFlags: Long, requiredFlags: Long) extends FindMemberBase[Boolean](tpe, name, excludedFlags, requiredFlags) { + private[this] var _result = false + override protected def result: Boolean = _result + + protected def shortCircuit(sym: Symbol): Boolean = { + _result = true + true // prevents call to addMemberIfNew + } + + // Not used + protected def addMemberIfNew(sym: Symbol): Unit = {} + } + } diff --git a/test/files/neg/userdefined_apply.check b/test/files/neg/userdefined_apply.check index ca0154885d..c8c8976f5f 100644 --- a/test/files/neg/userdefined_apply.check +++ b/test/files/neg/userdefined_apply.check @@ -1,13 +1,25 @@ userdefined_apply.scala:3: error: overloaded method apply needs result type private def apply(x: Int) = if (x > 0) new ClashOverloadNoSig(x) else apply("") ^ -userdefined_apply.scala:12: error: overloaded method apply needs result type +userdefined_apply.scala:14: error: overloaded method apply needs result type private def apply(x: Int) = if (x > 0) ClashRecNoSig(1) else ??? ^ -userdefined_apply.scala:19: error: overloaded method apply needs result type +userdefined_apply.scala:21: error: overloaded method apply needs result type private def apply(x: Boolean) = if (x) NoClashNoSig(1) else ??? ^ -userdefined_apply.scala:26: error: overloaded method apply needs result type +userdefined_apply.scala:28: error: overloaded method apply needs result type private def apply(x: Boolean) = if (x) NoClashOverload(1) else apply("") ^ -four errors found +userdefined_apply.scala:45: error: recursive method apply needs result type +case class NoClashNoSigPoly private(x: Int) + ^ +userdefined_apply.scala:39: error: NoClashNoSigPoly.type does not take parameters + def apply(x: T) = if (???) NoClashNoSigPoly(1) else ??? + ^ +userdefined_apply.scala:57: error: recursive method apply needs result type +case class ClashNoSigPoly private(x: Int) + ^ +userdefined_apply.scala:51: error: ClashNoSigPoly.type does not take parameters + def apply(x: T) = if (???) ClashNoSigPoly(1) else ??? + ^ +8 errors found diff --git a/test/files/neg/userdefined_apply.scala b/test/files/neg/userdefined_apply.scala index 1f2aff6e82..0a0d960b39 100644 --- a/test/files/neg/userdefined_apply.scala +++ b/test/files/neg/userdefined_apply.scala @@ -8,6 +8,8 @@ object ClashOverloadNoSig { case class ClashOverloadNoSig private(x: Int) object ClashRecNoSig { + // TODO: status quo is that the error refers to an overloaded method, which is actually recursive + // (we should have unlinked the symbol in the `if(suppress)` part of `applyUnapplyMethodCompleter`) // error: recursive method apply needs result type private def apply(x: Int) = if (x > 0) ClashRecNoSig(1) else ??? } @@ -29,3 +31,27 @@ object NoClashOverload { } case class NoClashOverload private(x: Int) + + +class BaseNCNSP[T] { + // TODO: suppress the following error + // error: NoClashNoSigPoly.type does not take parameters + def apply(x: T) = if (???) NoClashNoSigPoly(1) else ??? +} + +object NoClashNoSigPoly extends BaseNCNSP[Boolean] +// TODO: position error at definition of apply in superclass instead of on case clss +// error: recursive method apply needs result type +case class NoClashNoSigPoly private(x: Int) + + +class BaseCNSP[T] { + // TODO: suppress the following error + // error: ClashNoSigPoly.type does not take parameters + def apply(x: T) = if (???) ClashNoSigPoly(1) else ??? +} + +object ClashNoSigPoly extends BaseCNSP[Int] +// TODO: position error at definition of apply in superclass instead of on case clss +// error: recursive method apply needs result type +case class ClashNoSigPoly private(x: Int) diff --git a/test/files/pos/userdefined_apply.scala b/test/files/pos/userdefined_apply.scala index ca563f1dc5..e29f9f5141 100644 --- a/test/files/pos/userdefined_apply.scala +++ b/test/files/pos/userdefined_apply.scala @@ -34,3 +34,21 @@ object NoClashOverload { def apply(x: String): NoClashOverload = ??? } case class NoClashOverload private (x: Int) + + + +class BaseNCP[T] { + // error: overloaded method apply needs result type + def apply(x: T): NoClashPoly = if (???) NoClashPoly(1) else ??? +} + +object NoClashPoly extends BaseNCP[Boolean] +case class NoClashPoly private(x: Int) + + +class BaseCP[T] { + // error: overloaded method apply needs result type + def apply(x: T): ClashPoly = if (???) ClashPoly(1) else ??? +} +object ClashPoly extends BaseCP[Int] +case class ClashPoly private(x: Int) -- cgit v1.2.3