Skip to content

Commit d8aae86

Browse files
author
Som Snytt
committed
Correctly detect double defs involving param accessors
1 parent d76229e commit d8aae86

File tree

7 files changed

+279
-85
lines changed

7 files changed

+279
-85
lines changed

‎src/compiler/scala/tools/nsc/typechecker/Namers.scala

Lines changed: 28 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -206,26 +206,6 @@ trait Namers extends MethodSynthesis {
206206
else innerNamer
207207
}
208208

209-
// FIXME - this logic needs to be thoroughly explained
210-
// and justified. I know it's wrong with respect to package
211-
// objects, but I think it's also wrong in other ways.
212-
protected def conflict(newS: Symbol, oldS: Symbol) = (
213-
( !oldS.isSourceMethod
214-
|| nme.isSetterName(newS.name)
215-
|| newS.isTopLevel
216-
) &&
217-
!( // @M: allow repeated use of `_` for higher-order type params
218-
(newS.owner.isTypeParameter || newS.owner.isAbstractType)
219-
// FIXME: name comparisons not successful, are these underscores
220-
// sometimes nme.WILDCARD and sometimes tpnme.WILDCARD?
221-
&& (newS.name string_== nme.WILDCARD)
222-
)
223-
)
224-
225-
private def allowsOverload(sym: Symbol) = (
226-
sym.isSourceMethod && sym.owner.isClass && !sym.isTopLevel
227-
)
228-
229209
private def inCurrentScope(m: Symbol): Boolean = {
230210
if (owner.isClass) owner == m.owner
231211
else context.scope.lookupSymbolEntry(m) match {
@@ -237,44 +217,42 @@ trait Namers extends MethodSynthesis {
237217
/** Enter symbol into context's scope and return symbol itself */
238218
def enterInScope(sym: Symbol): sym.type = enterInScope(sym, context.scope)
239219

220+
// There is nothing which reconciles a package's scope with
221+
// the package object's scope. This is the source of many bugs
222+
// with e.g. defining a case class in a package object. When
223+
// compiling against classes, the class symbol is created in the
224+
// package and in the package object, and the conflict is undetected.
225+
// There is also a non-deterministic outcome for situations like
226+
// an object with the same name as a method in the package object.
240227
/** Enter symbol into given scope and return symbol itself */
241228
def enterInScope(sym: Symbol, scope: Scope): sym.type = {
242-
// FIXME - this is broken in a number of ways.
243-
//
244-
// 1) If "sym" allows overloading, that is not itself sufficient to skip
245-
// the check, because "prev.sym" also must allow overloading.
246-
//
247-
// 2) There is nothing which reconciles a package's scope with
248-
// the package object's scope. This is the source of many bugs
249-
// with e.g. defining a case class in a package object. When
250-
// compiling against classes, the class symbol is created in the
251-
// package and in the package object, and the conflict is undetected.
252-
// There is also a non-deterministic outcome for situations like
253-
// an object with the same name as a method in the package object.
254-
255-
// allow for overloaded methods
256-
if (!allowsOverload(sym)) {
257-
val prev = scope.lookupEntry(sym.name)
258-
if ((prev ne null) && prev.owner == scope && conflict(sym, prev.sym)) {
259-
if (sym.isSynthetic || prev.sym.isSynthetic) {
260-
handleSyntheticNameConflict(sym, prev.sym)
261-
handleSyntheticNameConflict(prev.sym, sym)
262-
}
263-
DoubleDefError(sym, prev.sym)
264-
sym setInfo ErrorType
265-
scope unlink prev.sym // let them co-exist...
266-
// FIXME: The comment "let them co-exist" is confusing given that the
267-
// line it comments unlinks one of them. What does it intend?
268-
}
269-
}
270229
if (sym.isModule && sym.isSynthetic && sym.owner.isClass && !sym.isTopLevel) {
271230
val entry = scope.lookupEntry(sym.name.toTypeName)
272231
if (entry eq null)
273232
scope enter sym
274233
else
275234
scope.enterBefore(sym, entry)
276-
} else
277-
scope enter sym
235+
} else {
236+
val disallowsOverload = !(sym.isSourceMethod && sym.owner.isClass && !sym.isTopLevel)
237+
if (disallowsOverload) {
238+
val prev = scope.lookupEntry(sym.name)
239+
val dde =
240+
(prev ne null) && prev.owner == scope &&
241+
(!prev.sym.isSourceMethod || nme.isSetterName(sym.name) || sym.isTopLevel) &&
242+
!((sym.owner.isTypeParameter || sym.owner.isAbstractType) && (sym.name string_== nme.WILDCARD))
243+
// @M: allow repeated use of `_` for higher-order type params
244+
if (dde) {
245+
if (sym.isSynthetic || prev.sym.isSynthetic) {
246+
handleSyntheticNameConflict(sym, prev.sym)
247+
handleSyntheticNameConflict(prev.sym, sym)
248+
}
249+
DoubleDefError(sym, prev.sym)
250+
sym.setInfo(ErrorType)
251+
scope.unlink(prev.sym) // retain the new erroneous symbol in scope (was for IDE); see #scala/bug#2779
252+
}
253+
}
254+
scope.enter(sym)
255+
}
278256
}
279257

280258
/** Logic to handle name conflicts of synthetically generated symbols

‎src/compiler/scala/tools/nsc/typechecker/Typers.scala

Lines changed: 35 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -3405,49 +3405,50 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
34053405
result
34063406
}
34073407

3408-
// TODO: adapt to new trait field encoding, figure out why this exemption is made
3409-
// 'accessor' and 'accessed' are so similar it becomes very difficult to
3410-
//follow the logic, so I renamed one to something distinct.
3411-
def accesses(looker: Symbol, accessed: Symbol) = (
3412-
accessed.isLocalToThis
3413-
&& (accessed.isParamAccessor || looker.hasAccessorFlag && !accessed.hasAccessorFlag && accessed.isPrivate)
3414-
)
3415-
3408+
/* From the spec (refchecks checks other conditions regarding erasing to the same type and default arguments):
3409+
*
3410+
* A block expression [... its] statement sequence may not contain two definitions or
3411+
* declarations that bind the same name --> `inBlock`
3412+
*
3413+
* It is an error if a template directly defines two matching members.
3414+
*
3415+
* A member definition $M$ _matches_ a member definition $M'$, if $M$ and $M'$ bind the same name,
3416+
* and one of following holds:
3417+
* 1. Neither $M$ nor $M'$ is a method definition.
3418+
* 2. $M$ and $M'$ define both monomorphic methods with equivalent argument types.
3419+
* 3. $M$ defines a parameterless method and $M'$ defines a method with an empty parameter list `()` or _vice versa_.
3420+
* 4. $M$ and $M'$ define both polymorphic methods with equal number of argument types $\overline T$, $\overline T'$
3421+
* and equal numbers of type parameters $\overline t$, $\overline t'$, say,
3422+
* and $\overline T' = [\overline t'/\overline t]\overline T$.
3423+
*/
34163424
def checkNoDoubleDefs(scope: Scope): Unit = {
34173425
var e = scope.elems
34183426
while ((e ne null) && e.owner == scope) {
3427+
val sym = e.sym
34193428
var e1 = scope.lookupNextEntry(e)
34203429
while ((e1 ne null) && e1.owner == scope) {
3421-
val sym = e.sym
34223430
val sym1 = e1.sym
34233431

3424-
/* From the spec (refchecks checks other conditions regarding erasing to the same type and default arguments):
3425-
*
3426-
* A block expression [... its] statement sequence may not contain two definitions or
3427-
* declarations that bind the same name --> `inBlock`
3428-
*
3429-
* It is an error if a template directly defines two matching members.
3430-
*
3431-
* A member definition $M$ _matches_ a member definition $M'$, if $M$ and $M'$ bind the same name,
3432-
* and one of following holds:
3433-
* 1. Neither $M$ nor $M'$ is a method definition.
3434-
* 2. $M$ and $M'$ define both monomorphic methods with equivalent argument types.
3435-
* 3. $M$ defines a parameterless method and $M'$ defines a method with an empty parameter list `()` or _vice versa_.
3436-
* 4. $M$ and $M'$ define both polymorphic methods with equal number of argument types $\overline T$, $\overline T'$
3437-
* and equal numbers of type parameters $\overline t$, $\overline t'$, say,
3438-
* and $\overline T' = [\overline t'/\overline t]\overline T$.
3439-
*/
3440-
if (!(accesses(sym, sym1) || accesses(sym1, sym)) // TODO: does this purely defer errors until later?
3441-
&& (inBlock || !(sym.isMethod || sym1.isMethod) || (sym.tpe matches sym1.tpe))
3442-
// default getters are defined twice when multiple overloads have defaults.
3443-
// The error for this is deferred until RefChecks.checkDefaultsInOverloaded
3444-
&& !sym.isErroneous && !sym1.isErroneous && !sym.hasDefault) {
3445-
log("Double definition detected:\n " +
3446-
((sym.getClass, sym.info, sym.ownerChain)) + "\n " +
3447-
((sym1.getClass, sym1.info, sym1.ownerChain)))
3432+
def nullaryNilary: Boolean = {
3433+
def nn(m: Symbol): Boolean = m.isParamAccessor || m.hasAccessorFlag || !m.isMethod || {
3434+
m.tpe match {
3435+
case MethodType(Nil, _) | NullaryMethodType(_) => true
3436+
case _ => false
3437+
}
3438+
}
3439+
nn(sym) && nn(sym1)
3440+
}
3441+
val conflicted = inBlock || (!sym.isMethod && !sym1.isMethod) || nullaryNilary || sym.tpe.matches(sym1.tpe)
3442+
3443+
// default getters are defined twice when multiple overloads have defaults.
3444+
// The error for this is deferred until RefChecks.checkDefaultsInOverloaded
3445+
if (conflicted && !sym.isErroneous && !sym1.isErroneous && !sym.hasDefault) {
3446+
log(sm"""Double definition detected:
3447+
| ${(sym.getClass, sym.info, sym.ownerChain)}
3448+
| ${(sym1.getClass, sym1.info, sym1.ownerChain)}""")
34483449

34493450
DefDefinedTwiceError(sym, sym1)
3450-
scope.unlink(e1) // need to unlink to avoid later problems with lub; see #2779
3451+
scope.unlink(e1) // need to unlink to avoid later problems with lub; see #scala/bug#2779
34513452
}
34523453
e1 = scope.lookupNextEntry(e1)
34533454
}

‎test/files/neg/i20006.check

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
i20006.scala:8: error: method next is defined twice;
2+
the conflicting value next was defined at line 6:7
3+
override def next(): T = { // error
4+
^
5+
i20006.scala:7: error: method hasNext is defined twice;
6+
the conflicting value hasNext was defined at line 5:7
7+
override def hasNext: Boolean = first || hasNext(acc) // error
8+
^
9+
i20006.scala:22: error: method next is defined twice;
10+
the conflicting value next was defined at line 18:65
11+
override def next(): T = { // error
12+
^
13+
i20006.scala:21: error: method hasNext is defined twice;
14+
the conflicting value hasNext was defined at line 18:42
15+
override def hasNext: Boolean = first || hasNext(acc) // error
16+
^
17+
i20006.scala:36: error: method next is defined twice;
18+
the conflicting value next was defined at line 32:65
19+
def next(): T = { // error
20+
^
21+
i20006.scala:35: error: method hasNext is defined twice;
22+
the conflicting value hasNext was defined at line 32:42
23+
def hasNext: Boolean = first || hasNext(acc) // error
24+
^
25+
i20006.scala:47: error: x is already defined as value x
26+
val x: String = "member" // error
27+
^
28+
i20006.scala:50: error: variable x is defined twice;
29+
the conflicting value x was defined at line 49:9
30+
private var x: Int = 42 // error
31+
^
32+
i20006.scala:53: error: x is already defined as value x
33+
private[this] var x: Int = 42 // error
34+
^
35+
i20006.scala:56: error: method x is defined twice;
36+
the conflicting value x was defined at line 55:9
37+
def x(): Int = 42 // error
38+
^
39+
i20006.scala:63: error: method x is defined twice;
40+
the conflicting value x was defined at line 62:21
41+
def x(): Int = 42 // error
42+
^
43+
i20006.scala:67: error: method x is defined twice;
44+
the conflicting method x was defined at line 66:21
45+
def x(): Int = 42 // error
46+
^
47+
12 errors

‎test/files/neg/i20006.scala

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
2+
abstract class XIterateIterator[T](seed: T) extends collection.AbstractIterator[T] {
3+
private var first = true
4+
private var acc = seed
5+
val hasNext: T => Boolean
6+
val next: T => T
7+
override def hasNext: Boolean = first || hasNext(acc) // error
8+
override def next(): T = { // error
9+
if (first) {
10+
first = false
11+
} else {
12+
acc = next(acc)
13+
}
14+
acc
15+
}
16+
}
17+
18+
final class YIterateIterator[T](seed: T, hasNext: T => Boolean, next: T => T) extends collection.AbstractIterator[T] {
19+
private var first = true
20+
private var acc = seed
21+
override def hasNext: Boolean = first || hasNext(acc) // error
22+
override def next(): T = { // error
23+
if (first) {
24+
first = false
25+
} else {
26+
acc = next(acc)
27+
}
28+
acc
29+
}
30+
}
31+
32+
final class ZIterateIterator[T](seed: T, hasNext: T => Boolean, next: T => T) {
33+
private var first = true
34+
private var acc = seed
35+
def hasNext: Boolean = first || hasNext(acc) // error
36+
def next(): T = { // error
37+
if (first) {
38+
first = false
39+
} else {
40+
acc = next(acc)
41+
}
42+
acc
43+
}
44+
}
45+
46+
class C(x: String) {
47+
val x: String = "member" // error
48+
}
49+
class D(x: String) {
50+
private var x: Int = 42 // error
51+
}
52+
class E(x: String) {
53+
private[this] var x: Int = 42 // error
54+
}
55+
class F(x: String) {
56+
def x(): Int = 42 // error
57+
}
58+
class G(x: String) {
59+
def x(i: Int): Int = i
60+
}
61+
class H {
62+
private[this] val x: String = ""
63+
def x(): Int = 42 // error
64+
}
65+
class I {
66+
private[this] def x: String = ""
67+
def x(): Int = 42 // error
68+
}
69+
70+
/*
71+
-- [E120] Naming Error: test/files/neg/i20006.scala:8:15 ---------------------------------------------------------------
72+
8 | override def hasNext: Boolean = first || hasNext(acc)
73+
| ^
74+
| Double definition:
75+
| val hasNext: T => Boolean in class XIterateIterator at line 6 and
76+
| override def hasNext: Boolean in class XIterateIterator at line 8
77+
-- [E120] Naming Error: test/files/neg/i20006.scala:9:15 ---------------------------------------------------------------
78+
9 | override def next(): T = {
79+
| ^
80+
| Double definition:
81+
| val next: T => T in class XIterateIterator at line 7 and
82+
| override def next(): T in class XIterateIterator at line 9
83+
-- [E120] Naming Error: test/files/neg/i20006.scala:22:15 --------------------------------------------------------------
84+
22 | override def hasNext: Boolean = first || hasNext(acc)
85+
| ^
86+
| Double definition:
87+
| private[this] val hasNext: T => Boolean in class YIterateIterator at line 19 and
88+
| override def hasNext: Boolean in class YIterateIterator at line 22
89+
-- [E120] Naming Error: test/files/neg/i20006.scala:23:15 --------------------------------------------------------------
90+
23 | override def next(): T = {
91+
| ^
92+
| Double definition:
93+
| private[this] val next: T => T in class YIterateIterator at line 19 and
94+
| override def next(): T in class YIterateIterator at line 23
95+
-- [E120] Naming Error: test/files/neg/i20006.scala:36:6 ---------------------------------------------------------------
96+
36 | def hasNext: Boolean = first || hasNext(acc)
97+
| ^
98+
| Double definition:
99+
| private[this] val hasNext: T => Boolean in class ZIterateIterator at line 33 and
100+
| def hasNext: Boolean in class ZIterateIterator at line 36
101+
-- [E120] Naming Error: test/files/neg/i20006.scala:37:6 ---------------------------------------------------------------
102+
37 | def next(): T = {
103+
| ^
104+
| Double definition:
105+
| private[this] val next: T => T in class ZIterateIterator at line 33 and
106+
| def next(): T in class ZIterateIterator at line 37
107+
-- [E120] Naming Error: test/files/neg/i20006.scala:48:6 ---------------------------------------------------------------
108+
48 | val x: String = "member" // error
109+
| ^
110+
| Double definition:
111+
| private[this] val x: String in class C at line 47 and
112+
| val x: String in class C at line 48
113+
-- [E120] Naming Error: test/files/neg/i20006.scala:51:14 --------------------------------------------------------------
114+
51 | private var x: Int = 42 // error
115+
| ^
116+
| Double definition:
117+
| private[this] val x: String in class D at line 50 and
118+
| private[this] var x: Int in class D at line 51
119+
-- [E120] Naming Error: test/files/neg/i20006.scala:54:20 --------------------------------------------------------------
120+
54 | private[this] var x: Int = 42 // error
121+
| ^
122+
| Double definition:
123+
| private[this] val x: String in class E at line 53 and
124+
| private[this] var x: Int in class E at line 54
125+
-- [E120] Naming Error: test/files/neg/i20006.scala:57:6 ---------------------------------------------------------------
126+
57 | def x(): Int = 42 // error
127+
| ^
128+
| Double definition:
129+
| private[this] val x: String in class F at line 56 and
130+
| def x(): Int in class F at line 57
131+
-- [E120] Naming Error: test/files/neg/i20006.scala:65:6 ---------------------------------------------------------------
132+
65 | def x(): Int = 42
133+
| ^
134+
| Double definition:
135+
| val x: String in class H at line 63 and
136+
| def x(): Int in class H at line 65
137+
-- Warning: test/files/neg/i20006.scala:54:16 --------------------------------------------------------------------------
138+
54 | private[this] var x: Int = 42 // error
139+
| ^
140+
| Ignoring [this] qualifier.
141+
| This syntax will be deprecated in the future; it should be dropped.
142+
| See: https://docs.scala-lang.org/scala3/reference/dropped-features/this-qualifier.html
143+
| This construct can be rewritten automatically under -rewrite -source 3.4-migration.
144+
1 warning found
145+
11 errors found
146+
*/

‎test/files/neg/t521.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ class PlainFile(val file : File) extends AbstractFile {}
1111
class VirtualFile(val name : String, val path : String) extends AbstractFile {}
1212

1313
final class ZipArchive(val file : File, archive : ZipFile) extends PlainFile(file) {
14-
class Entry(name : String, path : String) extends VirtualFile(name, path) {
14+
class Entry(name : String, path0 : String) extends VirtualFile(name, path0) {
1515
override def path = "";
1616
}
1717
}

‎test/files/neg/t521b.check

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
t521b.scala:15: error: method path is defined twice;
2+
the conflicting value path was defined at line 14:30
3+
override def path = "";
4+
^
5+
1 error

0 commit comments

Comments
 (0)