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.

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

OuterClass.this.type loses type refinement #5130

Closed
scabug opened this issue Oct 31, 2011 · 12 comments
Closed

OuterClass.this.type loses type refinement #5130

scabug opened this issue Oct 31, 2011 · 12 comments
Assignees
Labels
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Oct 31, 2011

scala> class A {
     |   this_a =>
     |   def b= new B
     |   class B {
     |     def a: this_a.type= this_a
     |   }
     | }
defined class A

scala> def a= new A {
     |   def c { }
     | }
a: A{def c: Unit}

scala> a.b.a.c
<console>:10: error: value c is not a member of A
       a.b.a.c
             ^

Checking the type of a.b.a shows why:

scala> a.b.a
res1: A = $anon$1@fe210a

Should be A{def c: Unit}

Surprisingly, changing the "def a" into a "val" works around the issue in this particular case:

scala> val a= new A {
     |   def c { }
     | }
a: A{def c: Unit} = $anon$1@2f0bd7

scala> a.b.a
res3: a.type = $anon$1@2f0bd7

scala> a.b.a.c
@scabug
Copy link
Author

@scabug scabug commented Oct 31, 2011

Imported From: https://issues.scala-lang.org/browse/SI-5130?orig=1
Reporter: @jsalvata
Affected Versions: 2.9.0, 2.9.1

Loading

@scabug
Copy link
Author

@scabug scabug commented Oct 31, 2011

@jsalvata said:
Note that using A.this.type instead of this_a.type doesn't help -- same behaviour.

A general workaround requires passing this_a.type as a type parameter:

scala> class A {
     |   this_a =>
     |   def b= new B[this_a.type]
     |   class B[E >: this_a.type] {
     |     def a: E= this_a
     |   }
     | }
defined class A

scala> def a= new A {
     |   def c { }
     | }
a: A{def c: Unit}

scala> a.b.a.c

scala> 

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 17, 2011

@jsalvata said:
This also affects mix-ins:

scala>   abstract class A {
     |     this_a =>
     |     class B {
     |       def a: this_a.type= this_a
     |     }
     |     def b= new B
     |   }
defined class A

scala>   trait A2 {
     |     def c { }
     |   }
defined trait A2

scala>   def a= new A with A2
a: A with A2

scala>   a.b.a.c
<console>:11: error: value c is not a member of A
         a.b.a.c
               ^

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 18, 2011

@jsalvata said:
I've traced the problem to this fragment in method typeRef in scala.reflect.internal.Types:

      val pre1 = pre match {
       case x: SuperType if sym1.isEffectivelyFinal || sym1.isDeferred =>
         x.thistpe
       case _: CompoundType if sym1.isClass =>
         // sharpen prefix so that it is maximal and still contains the class.
         pre.parents.reverse dropWhile (_.member(sym1.name) != sym1) match {
           case Nil         => pre
           case parent :: _ => parent
         }
       case _ => pre
     }

I don't see why we want to loose type information of CompoundTypes in this manner. Maybe it is useful to make inferred types wider? I can't see how, except in the presence of a this.type -- which is exactly where it can break things... but of course my sight here is really limited.

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 18, 2011

@paulp said:
That's some good detective work. I can add the observation that removing the second case in its entirety has zero observable impact anywhere in the distribution or test suite, except that a.b.a.c starts working. Assigning to martin for an opinion on what it is trying to accomplish. Any code which can be removed without observable impact must be presumed buggy: either it isn't doing what is intended, or the bug is the absence of a test case which backstops its purpose by breaking in its absence.

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 18, 2011

@jsalvata said (edited on Nov 18, 2011 8:22:43 PM UTC):
[Comment deleted: a third partest run passed smoothly, so the OOM issue is most likely unrelated to this change.]

Loading

@scabug
Copy link
Author

@scabug scabug commented Nov 19, 2011

@jsalvata said:
Proposed a fix: scala/scala#116

Loading

@scabug
Copy link
Author

@scabug scabug commented Jul 19, 2012

@odersky said:
It seems that pullrequest has been superseded with something else. Can you point me to the proposed fix again? Sorry for dropping the ball here.

Loading

@scabug
Copy link
Author

@scabug scabug commented Jul 19, 2012

@paulp said:
I don't know what the proposed fix held, but here is what I meant - at the time I wrote the above, this change had no observable impact except to fix this ticket.

https://github.com/paulp/scala/tree/issue/5130

Loading

@scabug
Copy link
Author

@scabug scabug commented Sep 29, 2012

Loading

@scabug
Copy link
Author

@scabug scabug commented Dec 4, 2012

@retronym said:
Merged in scala/scala@5ac896d

Loading

@scabug
Copy link
Author

@scabug scabug commented Jan 23, 2013

@adriaanm said:
fixed for 2.11 in scala/scala@5ac896d -- potential backport pending

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants