Rewrite resolveThis in global init checker #23282

EnzeXing · 2025-05-29T13:52:22Z

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

[test_scala2_library_tasty]

olhotak · 2025-05-29T14:20:40Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

@@ -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)


Should we keep an assertion that isRefSet || isEnvSet?

olhotak · 2025-05-29T14:27:25Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

@@ -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


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

olhotak · 2025-06-02T12:13:01Z

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 · 2025-06-03T15:24:57Z

@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:

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
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

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 · 2025-06-03T15:28:12Z

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 · 2025-06-05T04:39:25Z

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:

scala3/compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

Lines 1207 to 1216 in 0be2091

    
              *  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.

olhotak · 2025-06-18T13:16:57Z

[test_scala2_library_tasty]

olhotak · 2025-06-18T13:38:19Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

+          val outerEnvs = envSet.joinOuterEnvs
+          if outerEnvs != NoEnv then // Search for the outerEnvs of the current envSet
+            resolveEnvRecur(target, outerEnvs, bySymbol)
+          else // Search through the outerEnvs of the instances represented by `this`


Does this really make sense as a fallback?

We should know statically whether a symbol that we're looking for is in a local env (in that case the TermRef has NoPrefix) or whether it is in the body of the this object (or one of its outers) (in that case the TermRef has some form of the this object as prefix). It's odd to search both of these.

I have added another test local-class.scala to explain this case. In the test, method bar in loca class D tries to access c, which is a local variable without prefix. The scope in the Config is LocalEnv(bar) when this access happens, but c is defined in foo, which is not a method in D. When we find that bar does not have any direct enclosing method, we have to look through the outerEnv of D since c could be defined there, or defined in other outer methods that encloses some other local classes that encloses D.

olhotak · 2025-06-18T13:42:49Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

      // meth == NoSymbol for poly functions
      if meth.name == nme.tupled then
        value // a call like `fun.tupled`
      else
        code match
        case ddef: DefDef =>
          if meth.name == nme.apply then
-            given Scope = Env.ofDefDef(ddef, args.map(_.value), ScopeSet(Set(env)))
+            val funEnv = scope match {
+              case ref: Ref => Env.ofDefDef(ddef, args.map(_.value), thisV, Env.NoEnv)


Is it correct to pass thisV here, or should it be ref?

It's supposed to pass in the value of thisV when the closure is created, and it is recorded in Fun. I have renamed thisV in this line to thisVOfClosure for clarification

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

liufengyun

LGTM, I only have a few minor comments for consideration.

liufengyun · 2025-06-18T13:32:33Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

-   * valsMap = sym -> val                                                // maps variables to their values
-   * outersMap = sym -> ScopeSet                                         // maps the possible outer scopes for a corresponding (parent) class
-   * heap.MutableData = Scope -> (valsMap, outersMap)                    // heap is mutable
+   * Config ::= (thisV: Value, scope: Scope, Heap, EnvMap)


Is it possible for scope in Config to be Ref? The same question for Fun.

These are the only place where the type Scope appears. If we can remove one concept, that would be nice (Occam's razor).

Minor: regarding the name LocalEnv, maybe we can use something like EnvAddr or EnvRef?

liufengyun · 2025-06-18T13:41:12Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

@@ -187,12 +202,13 @@ class Objects(using Context @constructorOnly):

  object OfClass:


Minor (for future): We can consider rename OfClass to InstanceRef to make its nature of being an abstract address clear. The same for OfArray.

liufengyun · 2025-06-18T13:46:35Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

+      def initVar(field: Symbol, value: Value)(using Context, EnvMap.EnvMapMutableData) = log("Initialize " + field.show + " = " + value + " for " + this, printer) {
+        assert(field.is(Flags.Mutable), "Field is not mutable: " + field.show)
+        EnvMap.writeJoinVal(this, field, value)
+      }


Minor (for future): Given that Var and Val have same logic, we can consider generalize a little the code.

liufengyun · 2025-06-18T13:55:51Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

   * Ref ::= ObjectRef | OfClass | OfArray                               // values that represent a reference to some (global or instance) object
-   * ThisValue ::= Ref | Set(Ref)                                        // possible values for 'this'
+   * RefSet ::= Set(ref)                                                 // set of refs


Minor (for future): There is some overlapping between ValueSet and RefSet. It seems the former is a subset of the latter. If that is the case, we can consider introducing an opaque type with a special constructor to encode the invariant.

Edit: I see RefSet is made a subclass of ValueSet. That is a clever trick. Might be worth adding a comment there warning that equality is delegated to ValueSet thus no more fields should be added.

liufengyun · 2025-06-18T14:00:30Z

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

            case outer : ThisValue =>
              if klass.owner.is(Flags.Package) then
                report.warning("[Internal error] top-level class should have `Package` as outer, class = " + klass.show + ", outer = " + outer.show + ", " + Trace.show, Trace.position)
-                Env.NoEnv
+                (Bottom, Env.NoEnv)


Minor (for future): Here we use Bottom to mean absence of value. I think it is OK. /cc: @olhotak

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

EnzeXing force-pushed the global-init-checker-redesign branch from 798fd49 to 1e0eb9f Compare May 29, 2025 13:56

olhotak reviewed May 29, 2025

View reviewed changes

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

EnzeXing force-pushed the global-init-checker-redesign branch from 8b89ae9 to f2f9df3 Compare June 17, 2025 02:47

EnzeXing marked this pull request as ready for review June 17, 2025 11:53

olhotak reviewed Jun 18, 2025

View reviewed changes

liufengyun approved these changes Jun 18, 2025

View reviewed changes

EnzeXing added 4 commits June 19, 2025 22:41

Rewrite resolveThis; removed assertions

d15b220

Add complex tests

776801e

Update abstract domain and tests

b44185b

Updates on outerEnvs stored in InstanceBody; address comments

b1d5eae

EnzeXing force-pushed the global-init-checker-redesign branch from f2f9df3 to b1d5eae Compare June 20, 2025 02:42

		@@ -187,12 +202,13 @@ class Objects(using Context @constructorOnly):

		object OfClass:

Rewrite resolveThis in global init checker #23282

Are you sure you want to change the base?

Rewrite resolveThis in global init checker #23282

Conversation

EnzeXing commented May 29, 2025 • edited by liufengyun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olhotak commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EnzeXing commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EnzeXing commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liufengyun commented Jun 5, 2025

Uh oh!

olhotak commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EnzeXing commented May 29, 2025 •

edited by liufengyun

Loading

olhotak commented Jun 2, 2025 •

edited

Loading

EnzeXing commented Jun 3, 2025 •

edited

Loading

EnzeXing commented Jun 3, 2025 •

edited

Loading