Merge pull request #4813 from dotty-staging/fix-#4564

Fix #4564: Invalidate clashing case class methods

Author: allanrenucci

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -20,9 +20,18 @@ object desugar {
   /** Info of a variable in a pattern: The named tree and its type */
   private type VarInfo = (NameTree, Tree)
 
-  /** Names of methods that are added unconditionally to case classes */
+  /** Is `name` the name of a method that can be invalidated as a compiler-generated
+   *  case class method that clashes with a user-defined method?
+   */
+  def isRetractableCaseClassMethodName(name: Name)(implicit ctx: Context): Boolean = name match {
+    case nme.apply | nme.unapply | nme.copy => true
+    case DefaultGetterName(nme.copy, _) => true
+    case _ => false
+  }
+
+  /** Is `name` the name of a method that is added unconditionally to case classes? */
   def isDesugaredCaseClassMethodName(name: Name)(implicit ctx: Context): Boolean =
-    name == nme.copy || name.isSelectorName
+    isRetractableCaseClassMethodName(name) || name.isSelectorName
 
 // ----- DerivedTypeTrees -----------------------------------
 
@@ -207,8 +216,7 @@ object desugar {
             tpt = TypeTree(),
             rhs = vparam.rhs
           )
-          .withMods(Modifiers(mods.flags & AccessFlags, mods.privateWithin))
-          .withFlags(Synthetic)
+          .withMods(Modifiers(mods.flags & (AccessFlags | Synthetic), mods.privateWithin))
         val rest = defaultGetters(vparams :: vparamss1, n + 1)
         if (vparam.rhs.isEmpty) rest else defaultGetter :: rest
       case Nil :: vparamss1 =>

compiler/src/dotty/tools/dotc/typer/Namer.scala

@@ -768,6 +768,28 @@ class Namer { typer: Typer =>
       case _ =>
     }
 
+    /** Invalidate `denot` by overwriting its info with `NoType` if
+     *  `denot` is a compiler generated case class method that clashes
+     *  with a user-defined method in the same scope with a matching type.
+     */
+    private def invalidateIfClashingSynthetic(denot: SymDenotation): Unit = {
+      def isCaseClass(owner: Symbol) =
+        owner.isClass && {
+          if (owner.is(Module)) owner.linkedClass.is(CaseClass)
+          else owner.is(CaseClass)
+        }
+      val isClashingSynthetic =
+        denot.is(Synthetic) &&
+        desugar.isRetractableCaseClassMethodName(denot.name) &&
+        isCaseClass(denot.owner) &&
+        denot.owner.info.decls.lookupAll(denot.name).exists(alt =>
+          alt != denot.symbol && alt.info.matchesLoosely(denot.info))
+      if (isClashingSynthetic) {
+        typr.println(i"invalidating clashing $denot in ${denot.owner}")
+        denot.info = NoType
+      }
+    }
+
     /** Intentionally left without `implicit ctx` parameter. We need
      *  to pick up the context at the point where the completer was created.
      */
@@ -776,6 +798,7 @@ class Namer { typer: Typer =>
       addAnnotations(sym)
       addInlineInfo(sym)
       denot.info = typeSig(sym)
+      invalidateIfClashingSynthetic(denot)
       Checking.checkWellFormed(sym)
       denot.info = avoidPrivateLeaks(sym, sym.pos)
     }

compiler/src/dotty/tools/dotc/typer/Typer.scala

@@ -1413,7 +1413,12 @@ class Typer extends Namer
     }
   }
 
-  def typedDefDef(ddef: untpd.DefDef, sym: Symbol)(implicit ctx: Context) = track("typedDefDef") {
+  def typedDefDef(ddef: untpd.DefDef, sym: Symbol)(implicit ctx: Context): Tree = track("typedDefDef") {
+    if (!sym.info.exists) { // it's a discarded synthetic case class method, drop it
+      assert(sym.is(Synthetic) && desugar.isRetractableCaseClassMethodName(sym.name))
+      sym.owner.info.decls.openForMutations.unlink(sym)
+      return EmptyTree
+    }
     val DefDef(name, tparams, vparamss, tpt, _) = ddef
     completeAnnotations(ddef, sym)
     val tparams1 = tparams mapconserve (typed(_).asInstanceOf[TypeDef])
@@ -1911,7 +1916,8 @@ class Typer extends Namer
                     enumContexts(mdef1.symbol) = ctx
                   case _ =>
                 }
-                buf += mdef1
+                if (!mdef1.isEmpty) // clashing synthetic case methods are converted to empty trees
+                  buf += mdef1
             }
             traverse(rest)
         }
@@ -1986,7 +1992,6 @@ class Typer extends Namer
    *                (but do this at most once per tree).
    *
    *  After that, two strategies are tried, and the first that is successful is picked.
-   *  If neither of the strategies are successful, continues with`fallBack`.
    *
    *  1st strategy: Try to insert `.apply` so that the result conforms to prototype `pt`.
    *                This strategy is not tried if the prototype represents already
@@ -1995,6 +2000,10 @@ class Typer extends Namer
    *  2nd strategy: If tree is a select `qual.name`, try to insert an implicit conversion
    *    around the qualifier part `qual` so that the result conforms to the expected type
    *    with wildcard result type.
+   *
+   *  If neither of the strategies are successful, continues with the `apply` result
+   *  if an apply insertion was tried and `tree` has an `apply` method, or continues
+   *  with `fallBack` otherwise. `fallBack` is supposed to always give an error.
    */
   def tryInsertApplyOrImplicit(tree: Tree, pt: ProtoType, locked: TypeVars)(fallBack: => Tree)(implicit ctx: Context): Tree = {
 
@@ -2016,7 +2025,7 @@ class Typer extends Namer
       else try adapt(simplify(sel, pt, locked), pt, locked) finally sel.removeAttachment(InsertedApply)
     }
 
-    def tryImplicit =
+    def tryImplicit(fallBack: => Tree) =
       tryInsertImplicitOnQualifier(tree, pt, locked).getOrElse(fallBack)
 
     pt match {
@@ -2027,8 +2036,17 @@ class Typer extends Namer
         pt.markAsDropped()
         tree
       case _ =>
-        if (isApplyProto(pt) || isMethod(tree) || isSyntheticApply(tree)) tryImplicit
-        else tryEither(tryApply(_))((_, _) => tryImplicit)
+        if (isApplyProto(pt) || isMethod(tree) || isSyntheticApply(tree)) tryImplicit(fallBack)
+        else tryEither(tryApply(_)) { (app, appState) =>
+          tryImplicit {
+            if (tree.tpe.member(nme.apply).exists) {
+              // issue the error about the apply, since it is likely more informative than the fallback
+              appState.commit()
+              app
+            }
+            else fallBack
+          }
+        }
      }
   }
 

tests/neg/i4564.scala

@@ -0,0 +1,46 @@
+object ClashOverloadNoSig {
+
+  private def apply(x: Int) = if (x > 0) new ClashOverloadNoSig(x) else apply("") // error: overloaded method apply needs result type
+
+  def apply(x: String): ClashOverloadNoSig = ???
+}
+
+case class ClashOverloadNoSig private(x: Int)
+
+object ClashRecNoSig {
+  private def apply(x: Int) = if (x > 0) ClashRecNoSig(1) else ???    // error: recursive method apply needs result type
+}
+
+case class ClashRecNoSig private(x: Int)
+
+object NoClashNoSig {
+  private def apply(x: Boolean) = if (x) NoClashNoSig(1) else ???   // error: overloaded method apply needs result type
+}
+
+case class NoClashNoSig private(x: Int)
+
+object NoClashOverload {
+  private def apply(x: Boolean) = if (x) NoClashOverload(1) else apply("")   // error // error: overloaded method apply needs result type (twice)
+
+  def apply(x: String): NoClashOverload = ???
+}
+
+case class NoClashOverload private(x: Int)
+
+
+class BaseNCNSP[T] {
+  def apply(x: T) = if (???) NoClashNoSigPoly(1) else ???    // error: overloaded method apply needs result type
+}
+
+object NoClashNoSigPoly extends BaseNCNSP[Boolean]
+case class NoClashNoSigPoly private(x: Int)   // ok, since `apply` is in base class
+
+
+
+class BaseCNSP[T] {
+  def apply(x: T) = if (???) ClashNoSigPoly(1) else ???    // error: recursive method apply needs result type
+}
+
+object ClashNoSigPoly extends BaseCNSP[Int]
+// TODO: improve error message
+case class ClashNoSigPoly private(x: Int)  // error: found: ClashNoSigPoly required: Nothing

tests/pos/i4564.scala

@@ -0,0 +1,62 @@
+case class A(x: Int) {
+  def copy(x: Int = x) = A(x)
+}
+
+// NOTE: the companion inherits a public apply method from Function1!
+case class NeedsCompanion private (x: Int)
+
+object ClashNoSig { // ok
+  private def apply(x: Int) = if (x > 0) new ClashNoSig(x) else ???
+  def unapply(x: ClashNoSig) = x
+
+  ClashNoSig(2) match {
+    case c @ ClashNoSig(y) => c.copy(y + c._1)
+  }
+}
+case class ClashNoSig private (x: Int) {
+  def copy(y: Int) = ClashNoSig(y)
+}
+
+object Clash {
+  private def apply(x: Int) = if (x > 0) new Clash(x) else ???
+}
+case class Clash private (x: Int)
+
+object ClashSig {
+  private def apply(x: Int): ClashSig = if (x > 0) new ClashSig(x) else ???
+}
+case class ClashSig private (x: Int)
+
+object ClashOverload {
+  private def apply(x: Int): ClashOverload = if (x > 0) new ClashOverload(x) else apply("")
+  def apply(x: String): ClashOverload = ???
+}
+case class ClashOverload private (x: Int)
+
+object NoClashSig {
+  private def apply(x: Boolean): NoClashSig = if (x) NoClashSig(1) else ???
+}
+case class NoClashSig private (x: Int)
+
+object NoClashOverload {
+  // needs full sig
+  private def apply(x: Boolean): NoClashOverload = if (x) NoClashOverload(1) else apply("")
+  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)

tests/pos/t5816-noclash.scala

@@ -0,0 +1,13 @@
+object Foo {
+  // spurious error if:
+  //   - this definition precedes that of apply (which is overloaded with the synthetic one derived from the case class)
+  //   - AND `Foo.apply` is explicitly applied to `[A]` (no error if `[A]` is inferred)
+  //
+  def referToPolyOverloadedApply[A]: Foo[A] = Foo.apply[A]("bla")
+  //                                                   ^
+  //  found   : String("bla")
+  //  required: Int
+
+  def apply[A](x: Int): Foo[A] = ???
+}
+case class Foo[A](x: String)