Skip to content

Rewrite resolveThis in global init checker #23282

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

EnzeXing
Copy link
Contributor

This PR resolves bugs in the redesigned global initialization checker; specifically, it now correctly resolves the value of parent.this in a child class, and removes the assertions that fail in community build projects

@EnzeXing EnzeXing force-pushed the global-init-checker-redesign branch from 798fd49 to 1e0eb9f Compare May 29, 2025 13:56
@@ -287,11 +287,19 @@ class Objects(using Context @constructorOnly):
def toScopeSet: ScopeSet = ScopeSet(values.asInstanceOf[Set[Scope]])

case class ScopeSet(scopes: Set[Scope]):
assert(scopes.forall(_.isRef) || scopes.forall(_.isEnv), "All scopes should have the same type!")
def isRefSet = scopes.forall(_.isRef)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep an assertion that isRefSet || isEnvSet?

@@ -1969,6 +1977,11 @@ class Objects(using Context @constructorOnly):
case _ =>
report.warning("[Internal error] unexpected thisV = " + thisV + ", target = " + target.show + Trace.show, Trace.position)
Bottom
if resolveResult == Bottom && thisV.filterClass(target) == thisV then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that target could be a parent class of an outer class?

@olhotak
Copy link
Contributor

olhotak commented Jun 2, 2025

Here's an example of a ScopeSet that can have both classes and methods.

class A {
  var x = 42
  def change = (x = 44)
  class B {
    val i = 5
    def fooz = A.this.x // if this is an instance of class C, does A.this resolve to the C outer or to the B outer ???
    def fooz2 = A.this.x
    class D {
       def bar = fooz
       def bar2 = fooz2
    }
  }
}
class AA extends A {
  def foo = {
    //val y = 43
    val a = if(*) new A else new AAA
    class C /*outer AA*/ extends a.B /*outer A or AAA*/ {
       override val i = 6
       override def fooz2 = x
    }
    val bb: B = if(false) new a.B else new C
    val d =  new bb.D
    d.bar   // reads AA.x
    d.bar2 // reads AA.x
    }
}

d --outer-> {B or C} --outer-> {A or foo} --outer --> {A}
d.bar, looking for x
reach {A or foo}
A has x, foo doesn't
do I continue searching the outer of foo ???

@EnzeXing EnzeXing force-pushed the global-init-checker-redesign branch from 1e0eb9f to 8b89ae9 Compare June 3, 2025 15:09
@EnzeXing
Copy link
Contributor Author

EnzeXing commented Jun 3, 2025

@olhotak @liufengyun I have added two rather complex tests, feel free to run them with scala-cli.
With regard to multiple-outers.scala, here are my explanations:

  1. When accessing a field, the access chain from the enclosing class of the select expression to the enclosing class of the field is composed with two kinds of edges: $\overrightarrow{\text{outer}}$ and $\overrightarrow{\text{parent}}$. $\overrightarrow{\text{outer}}$ corresponds to a call of resolveThis, and $\overrightarrow{\text{parent}}$ relates to how we handle inherited fields
  2. In an access chain, $\overrightarrow{\text{outer}}$ can be followed by $\overrightarrow{\text{parent}}$, but $\overrightarrow{\text{parent}}$ cannot be followed by $\overrightarrow{\text{outer}}$. Example:
class A {
  val field_a = 5
  def bar(): Int = A.this.field_a
}

class B extends A {
  val field_b = field_a
  class C {
    val field_c = bar() // expands to B.this.bar()
    val field_c2 = field_a // C --> outer B --> parent A
  }
}

object O:
  val b = new B
  class D extends b.C {
    val field_d = field_b // D --> parent C --> outer B (invalid)
    // error: Not found: field_b
  }
  val d = new D
  1. At a particular step, if both two kinds of edges are valid, compiler issues an error for ambiguous access. Example:
class A {
  val x = 5
}

class B {
  val x = 6
  class C extends A {
    val f = x // error: Reference to x is ambiguous.
  }
}

@EnzeXing
Copy link
Contributor Author

EnzeXing commented Jun 3, 2025

The second test anon-class.scala is a tricky one, since it has the edge B $\overrightarrow{outer}$ B with the same dynamic class, so calling resolveThis cannot terminate. Maybe the potential solution is to stop when the combined outer set reaches a fixed point

@liufengyun
Copy link
Contributor

The second test anon-class.scala is a tricky one, since it has the edge B o u t e r → B with the same dynamic class, so calling resolveThis cannot terminate. Maybe the potential solution is to stop when the combined outer set reaches a fixed point

It's not clear why resolveThis should be a problem. I see the last PR changed resolveThis by removing the parameter klass: that is problematic semantically and I think it also caused the loop.

The following comment might help:

* Note that `klass` might be a super class of the object referred by `thisV`.
* The parameter `klass` is needed for `this` resolution. Consider the following code:
*
* class A {
* A.this
* class B extends A { A.this }
* }
*
* As can be seen above, the meaning of the expression `A.this` depends on where
* it is located.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants