Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix of #39 #42

Merged
merged 1 commit into from

3 participants

@odersky
Owner

Two fixes:
1) Avoid the infinite recursion in checkAccessible if the accessibility check fails.
2) Make accessibility succeed for the test, and in general if the target denotation does not have a symbol.

Added original test in pos and a negative test which makes accessibility fail.

Review by @DarkDimius, @samuelgruetter

@odersky odersky Fix of #39
Two fixes:
1) Avoid the infinite recursion in checkAccessible if the accessibility check fails.
2) Make accessibility succeed for the test, and in general if the target denotation does not have a symbol.

Added original test in pos and a negative test which makes accessibility fail.
f196e07
@DarkDimius DarkDimius commented on the diff
test/dotc/tests.scala
@@ -40,6 +40,7 @@ class tests extends CompilerTest {
@Test def pos_overloaded() = compileFile(posDir, "overloaded")
@Test def pos_templateParents() = compileFile(posDir, "templateParents")
@Test def pos_structural() = compileFile(posDir, "structural")
+ @Test def pos_i39 = compileFile(posDir, "i39")
@DarkDimius Owner

It would be good to have a http link for this issue on github, as "i39" is not so obvious.

@odersky Owner
odersky added a note

I think should have a naming convention for issue tests, then it will become obvious. Scala 2 has t123, should we do the same or stick with i123?

@DarkDimius Owner

Having such convention seems good idea.
I'd prefer i123.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@DarkDimius DarkDimius commented on the diff
test/dotc/tests.scala
@@ -49,6 +50,7 @@ class tests extends CompilerTest {
@Test def neg_privates() = compileFile(negDir, "privates", xerrors = 2)
@Test def neg_rootImports = compileFile(negDir, "rootImplicits", xerrors = 2)
@Test def neg_templateParents() = compileFile(negDir, "templateParents", xerrors = 3)
+ @Test def neg_i39 = compileFile(negDir, "i39", xerrors = 1)
@DarkDimius Owner

Same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@DarkDimius
Owner

Otherwise Ok for me

@DarkDimius
Owner

LGTM

@samuelgruetter

Not sure if this is an issue: In

object test241 {

  trait A {
    def foo(i: Int): String = ???
    def foo(s: String): String = ???
  }

  class B extends A {
    private def foo(s: String): String = ???
  }

  println((new B).foo(""))

}

I get

error: type mismatch:
found   : String("")
required: Int
println((new B).foo(""))
                    ^

so the foo(String) of super cannot be accessed. But if I remove the foo(Int), the foo(String) of super can be accessed. This is inconsistant. But, on the other hand, Scala 2.10.3 behaves the same.

@samuelgruetter samuelgruetter commented on the diff
src/dotty/tools/dotc/typer/Typer.scala
((31 lines not shown))
+ val d2 = pre.nonPrivateMember(name)
+ if (reallyExists(d2) && firstTry) test(pre.select(name, d2), false)
+ else {
+ val alts = tpe.denot.alternatives.map(_.symbol).filter(_.exists)
+ val what = alts match {
+ case Nil =>
+ name.toString
+ case sym :: Nil =>
+ if (sym.owner == pre.typeSymbol) sym.show else sym.showLocated
+ case _ =>
+ i"none of the overloaded alternatives named $name"
+ }
+ val where = if (ctx.owner.exists) s" from ${ctx.owner.enclosingClass}" else ""
+ val whyNot = new StringBuffer
+ val addendum =
+ alts foreach (_.isAccessibleFrom(pre, superAccess, whyNot))
@samuelgruetter Owner

why do you assign Unit to a val?

@odersky Owner
odersky added a note

Good catch. I will do a shortcut in the interest of time and fix it in the next commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@odersky odersky merged commit 6ada020 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 4, 2014
  1. @odersky

    Fix of #39

    odersky authored
    Two fixes:
    1) Avoid the infinite recursion in checkAccessible if the accessibility check fails.
    2) Make accessibility succeed for the test, and in general if the target denotation does not have a symbol.
    
    Added original test in pos and a negative test which makes accessibility fail.
This page is out of date. Refresh to see the latest.
View
2  src/dotty/tools/dotc/core/Denotations.scala
@@ -393,7 +393,7 @@ object Denotations {
exists && p(this)
def accessibleFrom(pre: Type, superAccess: Boolean)(implicit ctx: Context): Denotation =
- if (symbol isAccessibleFrom (pre, superAccess)) this else NoDenotation
+ if (!symbol.exists || symbol.isAccessibleFrom(pre, superAccess)) this else NoDenotation
def atSignature(sig: Signature)(implicit ctx: Context): SingleDenotation =
if (sig matches signature) this else NoDenotation
View
69 src/dotty/tools/dotc/typer/Typer.scala
@@ -116,40 +116,43 @@ class Typer extends Namer with Applications with Implicits {
* current context. Return the type with those alternatives as denotations
* which are accessible.
*/
- def checkAccessible(tpe: Type, superAccess: Boolean, pos: Position)(implicit ctx: Context): Type = tpe match {
- case tpe: NamedType =>
- val pre = tpe.prefix
- val name = tpe.name
- val d = tpe.denot.accessibleFrom(pre, superAccess)
- if (!d.exists) {
- val d2 = pre.nonPrivateMember(name)
- if (reallyExists(d2) && (d2 ne tpe.denot))
- checkAccessible(pre.select(name, d2), superAccess, pos)
- else {
- val alts = tpe.denot.alternatives.map(_.symbol).filter(_.exists)
- val what = alts match {
- case Nil =>
- name.toString
- case sym :: Nil =>
- if (sym.owner == pre.typeSymbol) sym.show else sym.showLocated
- case _ =>
- i"none of the overloaded alternatives named $name"
+ def checkAccessible(tpe: Type, superAccess: Boolean, pos: Position)(implicit ctx: Context): Type = {
+ def test(tpe: Type, firstTry: Boolean): Type = tpe match {
+ case tpe: NamedType =>
+ val pre = tpe.prefix
+ val name = tpe.name
+ val d = tpe.denot.accessibleFrom(pre, superAccess)
+ if (!d.exists) {
+ // it could be that we found an inaccessbile private member, but there is
+ // an inherited non-private member with the same name and signature.
+ val d2 = pre.nonPrivateMember(name)
+ if (reallyExists(d2) && firstTry) test(pre.select(name, d2), false)
+ else {
+ val alts = tpe.denot.alternatives.map(_.symbol).filter(_.exists)
+ val what = alts match {
+ case Nil =>
+ name.toString
+ case sym :: Nil =>
+ if (sym.owner == pre.typeSymbol) sym.show else sym.showLocated
+ case _ =>
+ i"none of the overloaded alternatives named $name"
+ }
+ val where = if (ctx.owner.exists) s" from ${ctx.owner.enclosingClass}" else ""
+ val whyNot = new StringBuffer
+ val addendum =
+ alts foreach (_.isAccessibleFrom(pre, superAccess, whyNot))
@samuelgruetter Owner

why do you assign Unit to a val?

@odersky Owner
odersky added a note

Good catch. I will do a shortcut in the interest of time and fix it in the next commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (!tpe.isError)
+ ctx.error(i"$what cannot be accessed as a member of $pre$where.$whyNot", pos)
+ ErrorType
}
- val where = if (ctx.owner.exists) s" from ${ctx.owner.enclosingClass}" else ""
- val whyNot = new StringBuffer
- val addendum =
- alts foreach (_.isAccessibleFrom(pre, superAccess, whyNot))
- if (!tpe.isError)
- ctx.error(i"$what cannot be accessed as a member of $pre$where.$whyNot", pos)
- ErrorType
- }
- }
- else if (d.symbol is TypeParamAccessor) // always dereference type param accessors
- checkAccessible(d.info.bounds.hi, superAccess, pos)
- else
- tpe withDenot d
- case _ =>
- tpe
+ } else if (d.symbol is TypeParamAccessor) // always dereference type param accessors
+ checkAccessible(d.info.bounds.hi, superAccess, pos)
+ else
+ tpe withDenot d
+ case _ =>
+ tpe
+ }
+ test(tpe, true)
}
/** The enclosing class, except if we are in a super call, in which case
View
2  test/dotc/tests.scala
@@ -40,6 +40,7 @@ class tests extends CompilerTest {
@Test def pos_overloaded() = compileFile(posDir, "overloaded")
@Test def pos_templateParents() = compileFile(posDir, "templateParents")
@Test def pos_structural() = compileFile(posDir, "structural")
+ @Test def pos_i39 = compileFile(posDir, "i39")
@DarkDimius Owner

It would be good to have a http link for this issue on github, as "i39" is not so obvious.

@odersky Owner
odersky added a note

I think should have a naming convention for issue tests, then it will become obvious. Scala 2 has t123, should we do the same or stick with i123?

@DarkDimius Owner

Having such convention seems good idea.
I'd prefer i123.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Test def neg_blockescapes() = compileFile(negDir, "blockescapesNeg", xerrors = 1)
@Test def neg_typedapply() = compileFile(negDir, "typedapply", xerrors = 4)
@@ -49,6 +50,7 @@ class tests extends CompilerTest {
@Test def neg_privates() = compileFile(negDir, "privates", xerrors = 2)
@Test def neg_rootImports = compileFile(negDir, "rootImplicits", xerrors = 2)
@Test def neg_templateParents() = compileFile(negDir, "templateParents", xerrors = 3)
+ @Test def neg_i39 = compileFile(negDir, "i39", xerrors = 1)
@DarkDimius Owner

Same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Test def dotc = compileDir(dotcDir + "tools/dotc")
@Test def dotc_ast = compileDir(dotcDir + "tools/dotc/ast")
View
19 tests/neg/i39.scala
@@ -0,0 +1,19 @@
+object i39neg {
+
+ trait B {
+ type D <: { type T }
+ def d: D
+ }
+
+ val bc: B = new B {
+ def d: D = ???
+ private def pd: D = ???
+ }
+
+ val d: bc.D = bc.d
+ val pd: bc.D = bc.pd
+
+ // infinite loop in Typer
+ val asT: d.T = ???
+
+}
View
17 tests/pos/i39.scala
@@ -0,0 +1,17 @@
+object i39 {
+
+ trait B {
+ type D <: { type T }
+ def d: D
+ }
+
+ val bc: B = new B {
+ def d: D = ???
+ }
+
+ val d: bc.D = bc.d
+
+ // infinite loop in Typer
+ val asT: d.T = ???
+
+}
Something went wrong with that request. Please try again.