Skip to content

Commit

Permalink
Correctly detect double defs involving param accessors
Browse files Browse the repository at this point in the history
  • Loading branch information
som-snytt committed Mar 25, 2024
1 parent 5aa3dc5 commit 4cf9bab
Show file tree
Hide file tree
Showing 7 changed files with 276 additions and 82 deletions.
78 changes: 28 additions & 50 deletions src/compiler/scala/tools/nsc/typechecker/Namers.scala
Expand Up @@ -206,26 +206,6 @@ trait Namers extends MethodSynthesis {
else innerNamer
}

// FIXME - this logic needs to be thoroughly explained
// and justified. I know it's wrong with respect to package
// objects, but I think it's also wrong in other ways.
protected def conflict(newS: Symbol, oldS: Symbol) = (
( !oldS.isSourceMethod
|| nme.isSetterName(newS.name)
|| newS.isTopLevel
) &&
!( // @M: allow repeated use of `_` for higher-order type params
(newS.owner.isTypeParameter || newS.owner.isAbstractType)
// FIXME: name comparisons not successful, are these underscores
// sometimes nme.WILDCARD and sometimes tpnme.WILDCARD?
&& (newS.name string_== nme.WILDCARD)
)
)

private def allowsOverload(sym: Symbol) = (
sym.isSourceMethod && sym.owner.isClass && !sym.isTopLevel
)

private def inCurrentScope(m: Symbol): Boolean = {
if (owner.isClass) owner == m.owner
else context.scope.lookupSymbolEntry(m) match {
Expand All @@ -237,44 +217,42 @@ trait Namers extends MethodSynthesis {
/** Enter symbol into context's scope and return symbol itself */
def enterInScope(sym: Symbol): sym.type = enterInScope(sym, context.scope)

// There is nothing which reconciles a package's scope with
// the package object's scope. This is the source of many bugs
// with e.g. defining a case class in a package object. When
// compiling against classes, the class symbol is created in the
// package and in the package object, and the conflict is undetected.
// There is also a non-deterministic outcome for situations like
// an object with the same name as a method in the package object.
/** Enter symbol into given scope and return symbol itself */
def enterInScope(sym: Symbol, scope: Scope): sym.type = {
// FIXME - this is broken in a number of ways.
//
// 1) If "sym" allows overloading, that is not itself sufficient to skip
// the check, because "prev.sym" also must allow overloading.
//
// 2) There is nothing which reconciles a package's scope with
// the package object's scope. This is the source of many bugs
// with e.g. defining a case class in a package object. When
// compiling against classes, the class symbol is created in the
// package and in the package object, and the conflict is undetected.
// There is also a non-deterministic outcome for situations like
// an object with the same name as a method in the package object.

// allow for overloaded methods
if (!allowsOverload(sym)) {
val prev = scope.lookupEntry(sym.name)
if ((prev ne null) && prev.owner == scope && conflict(sym, prev.sym)) {
if (sym.isSynthetic || prev.sym.isSynthetic) {
handleSyntheticNameConflict(sym, prev.sym)
handleSyntheticNameConflict(prev.sym, sym)
}
DoubleDefError(sym, prev.sym)
sym setInfo ErrorType
scope unlink prev.sym // let them co-exist...
// FIXME: The comment "let them co-exist" is confusing given that the
// line it comments unlinks one of them. What does it intend?
}
}
if (sym.isModule && sym.isSynthetic && sym.owner.isClass && !sym.isTopLevel) {
val entry = scope.lookupEntry(sym.name.toTypeName)
if (entry eq null)
scope enter sym
else
scope.enterBefore(sym, entry)
} else
scope enter sym
} else {
val disallowsOverload = !(sym.isSourceMethod && sym.owner.isClass && !sym.isTopLevel)
if (disallowsOverload) {
val prev = scope.lookupEntry(sym.name)
val dde =
(prev ne null) && prev.owner == scope &&
(!prev.sym.isSourceMethod || nme.isSetterName(sym.name) || sym.isTopLevel) &&
!((sym.owner.isTypeParameter || sym.owner.isAbstractType) && (sym.name string_== nme.WILDCARD))
// @M: allow repeated use of `_` for higher-order type params
if (dde) {
if (sym.isSynthetic || prev.sym.isSynthetic) {
handleSyntheticNameConflict(sym, prev.sym)
handleSyntheticNameConflict(prev.sym, sym)
}
DoubleDefError(sym, prev.sym)
sym.setInfo(ErrorType)
scope.unlink(prev.sym) // retain the new erroneous symbol in scope (was for IDE); see #scala/bug#2779
}
}
scope.enter(sym)
}
}

/** Logic to handle name conflicts of synthetically generated symbols
Expand Down
63 changes: 32 additions & 31 deletions src/compiler/scala/tools/nsc/typechecker/Typers.scala
Expand Up @@ -3410,49 +3410,50 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
result
}

// TODO: adapt to new trait field encoding, figure out why this exemption is made
// 'accessor' and 'accessed' are so similar it becomes very difficult to
//follow the logic, so I renamed one to something distinct.
def accesses(looker: Symbol, accessed: Symbol) = (
accessed.isLocalToThis
&& (accessed.isParamAccessor || looker.hasAccessorFlag && !accessed.hasAccessorFlag && accessed.isPrivate)
)

/* From the spec (refchecks checks other conditions regarding erasing to the same type and default arguments):
*
* A block expression [... its] statement sequence may not contain two definitions or
* declarations that bind the same name --> `inBlock`
*
* It is an error if a template directly defines two matching members.
*
* A member definition $M$ _matches_ a member definition $M'$, if $M$ and $M'$ bind the same name,
* and one of following holds:
* 1. Neither $M$ nor $M'$ is a method definition.
* 2. $M$ and $M'$ define both monomorphic methods with equivalent argument types.
* 3. $M$ defines a parameterless method and $M'$ defines a method with an empty parameter list `()` or _vice versa_.
* 4. $M$ and $M'$ define both polymorphic methods with equal number of argument types $\overline T$, $\overline T'$
* and equal numbers of type parameters $\overline t$, $\overline t'$, say,
* and $\overline T' = [\overline t'/\overline t]\overline T$.
*/
def checkNoDoubleDefs(scope: Scope): Unit = {
var e = scope.elems
while ((e ne null) && e.owner == scope) {
val sym = e.sym
var e1 = scope.lookupNextEntry(e)
while ((e1 ne null) && e1.owner == scope) {
val sym = e.sym
val sym1 = e1.sym

/* From the spec (refchecks checks other conditions regarding erasing to the same type and default arguments):
*
* A block expression [... its] statement sequence may not contain two definitions or
* declarations that bind the same name --> `inBlock`
*
* It is an error if a template directly defines two matching members.
*
* A member definition $M$ _matches_ a member definition $M'$, if $M$ and $M'$ bind the same name,
* and one of following holds:
* 1. Neither $M$ nor $M'$ is a method definition.
* 2. $M$ and $M'$ define both monomorphic methods with equivalent argument types.
* 3. $M$ defines a parameterless method and $M'$ defines a method with an empty parameter list `()` or _vice versa_.
* 4. $M$ and $M'$ define both polymorphic methods with equal number of argument types $\overline T$, $\overline T'$
* and equal numbers of type parameters $\overline t$, $\overline t'$, say,
* and $\overline T' = [\overline t'/\overline t]\overline T$.
*/
if (!(accesses(sym, sym1) || accesses(sym1, sym)) // TODO: does this purely defer errors until later?
&& (inBlock || !(sym.isMethod || sym1.isMethod) || (sym.tpe matches sym1.tpe))
def nn(m: Symbol): Boolean = m.isParamAccessor || m.hasAccessorFlag || !m.isMethod || {
def nn1(tp: Type): Boolean = tp match {
case MethodType(Nil, _) | NullaryMethodType(_) => true
//case PolyType(_, mt) => nn1(mt)
case _ => false
}
nn1(m.tpe)
}
def nullaryNilary: Boolean = nn(sym) && nn(sym1)

if ((inBlock || !(sym.isMethod || sym1.isMethod) || nullaryNilary || sym.tpe.matches(sym1.tpe))
// default getters are defined twice when multiple overloads have defaults.
// The error for this is deferred until RefChecks.checkDefaultsInOverloaded
&& !sym.isErroneous && !sym1.isErroneous && !sym.hasDefault) {
log("Double definition detected:\n " +
((sym.getClass, sym.info, sym.ownerChain)) + "\n " +
((sym1.getClass, sym1.info, sym1.ownerChain)))
log(sm"""Double definition detected:
| ${(sym.getClass, sym.info, sym.ownerChain)}
| ${(sym1.getClass, sym1.info, sym1.ownerChain)}""")

DefDefinedTwiceError(sym, sym1)
scope.unlink(e1) // need to unlink to avoid later problems with lub; see #2779
scope.unlink(e1) // need to unlink to avoid later problems with lub; see #scala/bug#2779
}
e1 = scope.lookupNextEntry(e1)
}
Expand Down
47 changes: 47 additions & 0 deletions test/files/neg/i20006.check
@@ -0,0 +1,47 @@
i20006.scala:8: error: method next is defined twice;
the conflicting value next was defined at line 6:7
override def next(): T = { // error
^
i20006.scala:7: error: method hasNext is defined twice;
the conflicting value hasNext was defined at line 5:7
override def hasNext: Boolean = first || hasNext(acc) // error
^
i20006.scala:22: error: method next is defined twice;
the conflicting value next was defined at line 18:65
override def next(): T = { // error
^
i20006.scala:21: error: method hasNext is defined twice;
the conflicting value hasNext was defined at line 18:42
override def hasNext: Boolean = first || hasNext(acc) // error
^
i20006.scala:36: error: method next is defined twice;
the conflicting value next was defined at line 32:65
def next(): T = { // error
^
i20006.scala:35: error: method hasNext is defined twice;
the conflicting value hasNext was defined at line 32:42
def hasNext: Boolean = first || hasNext(acc) // error
^
i20006.scala:47: error: x is already defined as value x
val x: String = "member" // error
^
i20006.scala:50: error: variable x is defined twice;
the conflicting value x was defined at line 49:9
private var x: Int = 42 // error
^
i20006.scala:53: error: x is already defined as value x
private[this] var x: Int = 42 // error
^
i20006.scala:56: error: method x is defined twice;
the conflicting value x was defined at line 55:9
def x(): Int = 42 // error
^
i20006.scala:63: error: method x is defined twice;
the conflicting value x was defined at line 62:21
def x(): Int = 42 // error
^
i20006.scala:67: error: method x is defined twice;
the conflicting method x was defined at line 66:21
def x(): Int = 42 // error
^
12 errors
146 changes: 146 additions & 0 deletions test/files/neg/i20006.scala
@@ -0,0 +1,146 @@

abstract class XIterateIterator[T](seed: T) extends collection.AbstractIterator[T] {
private var first = true
private var acc = seed
val hasNext: T => Boolean
val next: T => T
override def hasNext: Boolean = first || hasNext(acc) // error
override def next(): T = { // error
if (first) {
first = false
} else {
acc = next(acc)
}
acc
}
}

final class YIterateIterator[T](seed: T, hasNext: T => Boolean, next: T => T) extends collection.AbstractIterator[T] {
private var first = true
private var acc = seed
override def hasNext: Boolean = first || hasNext(acc) // error
override def next(): T = { // error
if (first) {
first = false
} else {
acc = next(acc)
}
acc
}
}

final class ZIterateIterator[T](seed: T, hasNext: T => Boolean, next: T => T) {
private var first = true
private var acc = seed
def hasNext: Boolean = first || hasNext(acc) // error
def next(): T = { // error
if (first) {
first = false
} else {
acc = next(acc)
}
acc
}
}

class C(x: String) {
val x: String = "member" // error
}
class D(x: String) {
private var x: Int = 42 // error
}
class E(x: String) {
private[this] var x: Int = 42 // error
}
class F(x: String) {
def x(): Int = 42 // error
}
class G(x: String) {
def x(i: Int): Int = i
}
class H {
private[this] val x: String = ""
def x(): Int = 42 // error
}
class I {
private[this] def x: String = ""
def x(): Int = 42 // error
}

/*
-- [E120] Naming Error: test/files/neg/i20006.scala:8:15 ---------------------------------------------------------------
8 | override def hasNext: Boolean = first || hasNext(acc)
| ^
| Double definition:
| val hasNext: T => Boolean in class XIterateIterator at line 6 and
| override def hasNext: Boolean in class XIterateIterator at line 8
-- [E120] Naming Error: test/files/neg/i20006.scala:9:15 ---------------------------------------------------------------
9 | override def next(): T = {
| ^
| Double definition:
| val next: T => T in class XIterateIterator at line 7 and
| override def next(): T in class XIterateIterator at line 9
-- [E120] Naming Error: test/files/neg/i20006.scala:22:15 --------------------------------------------------------------
22 | override def hasNext: Boolean = first || hasNext(acc)
| ^
| Double definition:
| private[this] val hasNext: T => Boolean in class YIterateIterator at line 19 and
| override def hasNext: Boolean in class YIterateIterator at line 22
-- [E120] Naming Error: test/files/neg/i20006.scala:23:15 --------------------------------------------------------------
23 | override def next(): T = {
| ^
| Double definition:
| private[this] val next: T => T in class YIterateIterator at line 19 and
| override def next(): T in class YIterateIterator at line 23
-- [E120] Naming Error: test/files/neg/i20006.scala:36:6 ---------------------------------------------------------------
36 | def hasNext: Boolean = first || hasNext(acc)
| ^
| Double definition:
| private[this] val hasNext: T => Boolean in class ZIterateIterator at line 33 and
| def hasNext: Boolean in class ZIterateIterator at line 36
-- [E120] Naming Error: test/files/neg/i20006.scala:37:6 ---------------------------------------------------------------
37 | def next(): T = {
| ^
| Double definition:
| private[this] val next: T => T in class ZIterateIterator at line 33 and
| def next(): T in class ZIterateIterator at line 37
-- [E120] Naming Error: test/files/neg/i20006.scala:48:6 ---------------------------------------------------------------
48 | val x: String = "member" // error
| ^
| Double definition:
| private[this] val x: String in class C at line 47 and
| val x: String in class C at line 48
-- [E120] Naming Error: test/files/neg/i20006.scala:51:14 --------------------------------------------------------------
51 | private var x: Int = 42 // error
| ^
| Double definition:
| private[this] val x: String in class D at line 50 and
| private[this] var x: Int in class D at line 51
-- [E120] Naming Error: test/files/neg/i20006.scala:54:20 --------------------------------------------------------------
54 | private[this] var x: Int = 42 // error
| ^
| Double definition:
| private[this] val x: String in class E at line 53 and
| private[this] var x: Int in class E at line 54
-- [E120] Naming Error: test/files/neg/i20006.scala:57:6 ---------------------------------------------------------------
57 | def x(): Int = 42 // error
| ^
| Double definition:
| private[this] val x: String in class F at line 56 and
| def x(): Int in class F at line 57
-- [E120] Naming Error: test/files/neg/i20006.scala:65:6 ---------------------------------------------------------------
65 | def x(): Int = 42
| ^
| Double definition:
| val x: String in class H at line 63 and
| def x(): Int in class H at line 65
-- Warning: test/files/neg/i20006.scala:54:16 --------------------------------------------------------------------------
54 | private[this] var x: Int = 42 // error
| ^
| Ignoring [this] qualifier.
| This syntax will be deprecated in the future; it should be dropped.
| See: https://docs.scala-lang.org/scala3/reference/dropped-features/this-qualifier.html
| This construct can be rewritten automatically under -rewrite -source 3.4-migration.
1 warning found
11 errors found
*/
2 changes: 1 addition & 1 deletion test/files/neg/t521.scala
Expand Up @@ -11,7 +11,7 @@ class PlainFile(val file : File) extends AbstractFile {}
class VirtualFile(val name : String, val path : String) extends AbstractFile {}

final class ZipArchive(val file : File, archive : ZipFile) extends PlainFile(file) {
class Entry(name : String, path : String) extends VirtualFile(name, path) {
class Entry(name : String, path0 : String) extends VirtualFile(name, path0) {
override def path = "";
}
}
5 changes: 5 additions & 0 deletions test/files/neg/t521b.check
@@ -0,0 +1,5 @@
t521b.scala:15: error: method path is defined twice;
the conflicting value path was defined at line 14:30
override def path = "";
^
1 error

0 comments on commit 4cf9bab

Please sign in to comment.