Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Correctly detect double defs involving param accessors & nilary methods #10727

Open
wants to merge 1 commit into
base: 2.13.x
Choose a base branch
from
Open

Correctly detect double defs involving param accessors & nilary methods #10727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 28 additions & 50 deletions src/compiler/scala/tools/nsc/typechecker/Namers.scala
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
@@ -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
View file
Open in desktop
@@ -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
View file
Open in desktop
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
View file
Open in desktop
@@ -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