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

Add inline changes semantics #15374

Closed
liufengyun opened this issue Jun 4, 2022 · 10 comments · Fixed by #15592
Closed

Add inline changes semantics #15374

liufengyun opened this issue Jun 4, 2022 · 10 comments · Fixed by #15592
Assignees
Milestone

Comments

@liufengyun
Copy link
Contributor

Compiler version

master

Minimized code

class A(val x: Int):
  class X:
    inline def foo() = A.this.foo()           // remove inline changes semantics

  private def foo(): Unit = println(x)

class B extends A(20):
  val a = new A(10)
  val y: Y = new Y

  class Y extends a.X

class C:
  var b = new B
  b.y.foo()  // should print 10, but prints 20

@main
def test = new C()

Output

20

Expectation

It should print 10, but it prints 20.

Initial Diagnosis

The problem seems to be related to the encoding of outer-select in inlining. The example above seems to suggest that hops alone is ambiguous.

@nicolasstucki
Copy link
Contributor

Seems related to #15327

@odersky
Copy link
Contributor

odersky commented Jun 12, 2022

Can somebody else take a look at it?

@odersky odersky removed their assignment Jun 12, 2022
@liufengyun
Copy link
Contributor Author

If you can give some hints, I can try to tackle the problem @odersky @nicolasstucki.

It seems the underlying problem is with the outer-select encoding: given e.n_<outer>, the number n alone is not sufficient to find the exact outer. Does the design encode the current class where the outer-select originates from in e or e.n_<outer> to resolve the ambiguity?

@ValeriePe
Copy link

This issue was picked for the Issue Spree 18 of July 5th which takes place in a week from now. @griggt @gagandeepkalra will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@nicolasstucki
Copy link
Contributor

When we inline the code we probably need to find the references to the outer this references and adapt them to the inline binding in the scope where they are inlined. Use -Xprint:inlining.

I see we get this

class C() extends Object() {
    var b: B = new B()
    def b_=(x$1: B): Unit = ()
    {
      val X_this: (B#y : B#Y) = this.b.y
      val A_this: (B#a : A) = X_this.1_<outer> // suspicious reference
      A_this.inline$foo():Unit
    }
  }

We should first try to:

  • Minimize the example
  • Figure out what should be on the RHS of A_this, or in general what code should be generated

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Jun 27, 2022

I imagine we need something like

val X_this: (B#y : B#Y) = this.b.y
val A_this: (B#a : A) = this.b.a
A_this.inline$foo():Unit

It seems that we even have the correct type projection in B#a.

@liufengyun
Copy link
Contributor Author

liufengyun commented Jun 27, 2022

I think I incorrectly put the number 15327 in the registration. It should be the current issue.

/cc: @ValeriePe @anatoliykmetyuk

@ValeriePe
Copy link

This issue was picked for the Issue Spree 18 of July 5th which takes place in a week from now. @liufengyun @jodersky @dwijnand @manojo will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@liufengyun
Copy link
Contributor Author

val X_this: (B#y : B#Y) = this.b.y
val A_this: (B#a : A) = this.b.a
A_this.inline$foo():Unit

The elaboration above is a little problematic, as the elaborated code now calls the mutable field getter b twice. If the getter is overridden and has side effects, that would change the semantics of the original program.

liufengyun added a commit to dotty-staging/dotty that referenced this issue Jul 5, 2022
…ss symbol

If we have an outer select `e.outer_<hops>`, we must make sure that the class symbol
of `e` is the class where the outer this is located in. Otherwise, the phase
`ElimOuterSelect` would get confused.

Co-authored-by: Dale Wijnand <[email protected]>
@dwijnand
Copy link
Member

dwijnand commented Jul 5, 2022

The PR seems viable. I was coming back to this to ask: seeing b isn't stable, is inlining off of a non-stable path meant to work? I only noticed because you can't call new on it (val y: Y = new b.Y).

liufengyun added a commit to dotty-staging/dotty that referenced this issue Jul 5, 2022
…ss symbol

If we have an outer select `e.outer_<hops>`, we must make sure that the class symbol
of `e` is the class where the outer this is located in. Otherwise, the phase
`ElimOuterSelect` would get confused.

Co-authored-by: Dale Wijnand <[email protected]>
odersky added a commit that referenced this issue Jul 6, 2022
Fix #15374: Make sure prefix of outer select has the correct class symbol
@Kordyjan Kordyjan added this to the 3.2.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants