Harden Period creation and use of Run/PhaseId by SolalPirelli · Pull Request #25484 · scala/scala3

SolalPirelli · 2026-03-10T11:02:16Z

Found while investigating #24970 but unfortunately does not fix that problem.

How much have your relied on LLM-based tools in this contribution?

not

How was the solution tested?

existing tests

SolalPirelli · 2026-03-10T11:38:01Z

compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala

+          if typerPhase.period == Periods.InvalidPeriod then
+            getMember(owner, innerName.toTypeName)
+          else
+            atPhase(typerPhase)(getMember(owner, innerName.toTypeName))


⚠️ bugfix, this is not great, but it's better than doing atPhase with an uninitialized phase.

SolalPirelli · 2026-03-10T11:38:26Z

compiler/src/dotty/tools/dotc/core/Denotations.scala

     */
    def initial: SingleDenotation =
-      if (validFor.firstPhaseId <= 1) this
+      if (validFor.firstPhaseId == FirstPhaseId) this


cannot be == 0 here unless something has gone terribly wrong

SolalPirelli · 2026-03-10T11:38:34Z

compiler/src/dotty/tools/dotc/core/Denotations.scala

     */
    def skipRemoved(using Context): SingleDenotation =
-      if (validFor.code <= 0) nextDefined else this
+      if (validFor == Nowhere) nextDefined else this


code is nonnegative

SolalPirelli · 2026-03-10T11:38:43Z

compiler/src/dotty/tools/dotc/core/Denotations.scala

      end goBack

-      if valid.code <= 0 then
+      if valid == Nowhere then


code is nonnegative

SolalPirelli · 2026-03-10T11:38:54Z

compiler/src/dotty/tools/dotc/core/Periods.scala

   *     last phase id:        7 bits
   *     #phases before last:  7 bits
   *
-   *     // Dmitry: sign == 0 isn't actually always true, in some cases phaseId == -1 is used for shifts, that easily creates code < 0


no longer true

SolalPirelli · 2026-03-10T11:39:30Z

compiler/src/dotty/tools/dotc/core/Periods.scala

-          case InitialPeriod                     => "InitialPeriod"
-          case InvalidPeriod                     => "InvalidPeriod"
-          case Period(NoRunId, 0, PhaseMask)     => s"Period(NoRunId.all)"
-          case Period(runId, 0, PhaseMask)       => s"Period($runId.all)"


this + the next few diffs: no reason to use 0 as the first phase ID to say all when we already have a constant defined to mean that

also, PhaseMask is logically the wrong thing here, even though it's equivalent to MaxPossiblePhaseId

SolalPirelli · 2026-03-10T11:40:13Z

compiler/src/dotty/tools/dotc/core/Periods.scala

    }
  }

-  inline val NowhereCode = 0


moved down so earlier vals don't depend on later ones

SolalPirelli · 2026-03-10T11:40:30Z

compiler/src/dotty/tools/dotc/core/Phases.scala

 import scala.collection.mutable.ListBuffer
 import dotty.tools.dotc.transform.MegaPhase.*
 import dotty.tools.dotc.transform.*
-import Periods.*


duplicate import with line 5

SolalPirelli · 2026-03-10T11:41:19Z

compiler/src/dotty/tools/dotc/core/Types.scala

+            && ctx.phaseId <= denot.validFor.lastPhaseId
            && lastDenot != null
-            && lastDenot.validFor.lastPhaseId > denot.validFor.firstPhaseId
+            && denot.validFor.firstPhaseId < lastDenot.validFor.lastPhaseId


⚠️ bugfix, plus rewrite of the expr to be consistent between the checks: before this, we didn't have the second check, and CC would end up creating periods with an end phase before the start phase

SolalPirelli · 2026-03-10T11:42:08Z

compiler/src/dotty/tools/dotc/Run.scala

+  given runContext[Dummy_so_its_a_def]: Context =
+    if myCtx eq null then myCtx = rootContext(using ictx)
+    assert(myCtx.nn.runId <= Periods.MaxPossibleRunId)
+    myCtx.nn


⚠️ bugfix, before this reset would leave the run in an invalid state since it sets myCtx to null but there was no code to re-initialize it (in practice we never actually reuse a run that we've reset so this didn't cause problems)

SolalPirelli · 2026-03-10T11:42:52Z

compiler/src/dotty/tools/dotc/core/Periods.scala

+      this.code < that.code
+
+    inline def >(that: Period): Boolean =
+      this.code > that.code


I didn't dare make these virtual methods in any way because of perf concerns, but I also don't think any code outside of Period should be accessing its .code

SolalPirelli · 2026-03-11T14:42:28Z

Found even more bugs, will put back in draft until those are fixed + I'm running benchmarks to make sure this doesn't kill perf

SolalPirelli · 2026-03-11T15:30:11Z

compiler/test/dotty/tools/dotc/core/PeriodsTest.scala

@@ -0,0 +1,40 @@
+package dotty.tools.dotc.core


noticed there was a worksheet testing Period, so I ported it to actual tests

SolalPirelli · 2026-03-11T15:31:10Z

compiler/src/dotty/tools/dotc/core/Periods.scala

-      //      lastDiff + d2 <= d1
-      //  iff X == 0 && l1 - l2 >= 0 && l1 - l2 + d2 <= d1
-      //  iff r1 == r2 & l1 >= l2 && l1 - d1 <= l2 - d2
-      //  q.e.d


this proof was wrong, counter-example this = (1, 56, 1), that = (1, 51, 32), found by Z3 with:

(declare-fun r1 () (_ BitVec 17)) (declare-fun l1 () (_ BitVec 7)) (declare-fun d1 () (_ BitVec 7)) (declare-fun r2 () (_ BitVec 17)) (declare-fun l2 () (_ BitVec 7)) (declare-fun d2 () (_ BitVec 7)) ; d <= l (assert (bvule d1 l1)) (assert (bvule d2 l2)) ; l < 64 (assert (bvult l1 #b1000000)) (assert (bvult l2 #b1000000)) ; r > 0 (assert (bvult #b00000000000000000 r1)) (assert (bvult #b00000000000000000 r2)) (declare-fun code1 () (_ BitVec 32)) (assert (= code1 (concat #b0 r1 l1 d1))) (declare-fun code2 () (_ BitVec 32)) (assert (= code2 (concat #b0 r2 l2 d2))) (declare-fun phaseWidth () (_ BitVec 32)) (declare-fun phaseMask () (_ BitVec 32)) (assert (= phaseWidth #x00000007)) (assert (= phaseMask #x0000007F)) (declare-fun lastDiff () (_ BitVec 32)) (assert (= lastDiff (bvlshr (bvsub code1 code2) phaseWidth))) (declare-fun result () Bool) (assert (= result (bvsle (bvadd lastDiff (bvand code1 phaseMask)) (bvand code2 phaseMask)))) (declare-fun ideal () Bool) (assert (= ideal (and (= r1 r2) (bvuge l1 l2) (bvule (bvsub l1 d1) (bvsub l2 d2))))) (assert (not (= result ideal))) (check-sat) (get-model)

SolalPirelli · 2026-03-11T15:31:41Z

compiler/src/dotty/tools/dotc/core/Periods.scala

    def runId: RunId = code >>> (PhaseWidth * 2)

-    /** The phase identifier of this single-phase period. */
-    def phaseId: PhaseId = (code >>> PhaseWidth) & PhaseMask


this was used on periods with more than one phase... but in practice it seems it's used as a "first phase ID", so I modified callers to just use that

SolalPirelli commented Mar 10, 2026

View reviewed changes

SolalPirelli requested a review from sjrd March 10, 2026 17:57

SolalPirelli marked this pull request as ready for review March 10, 2026 17:57

SolalPirelli force-pushed the solal/harden-period branch from 96a7fe5 to e622291 Compare March 11, 2026 14:41

SolalPirelli marked this pull request as draft March 11, 2026 14:42

SolalPirelli commented Mar 11, 2026

View reviewed changes

SolalPirelli added 7 commits March 12, 2026 17:46

Harden Period creation and use of Run/PhaseId

d79ec90

Fix Period.contains; counter-ex: this=(1,56,1) that=(1,51,32)

16c17c0

Store first phase ID directly

da04386

Delete Period.phaseId, force explicit use of first/last

6accf62

Perf?

f922858

More perf?

0d01e22

Silly me

877fc2c

SolalPirelli force-pushed the solal/harden-period branch from d465112 to 877fc2c Compare March 12, 2026 16:48

Conversation

SolalPirelli commented Mar 10, 2026

How much have your relied on LLM-based tools in this contribution?

How was the solution tested?

Uh oh!

SolalPirelli Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

SolalPirelli commented Mar 11, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

SolalPirelli Mar 10, 2026 •

edited

Loading