Refactor `Value.Ref` into `Value.{SimpleRef,MemberRef,InnerRef}` by Derppening · Pull Request #491 · hkust-taco/mlscript

Derppening · 2026-05-16T05:38:07Z

TODO:

Migrate case Value.Ref into apply and unapply functions

Derppening · 2026-05-16T05:58:40Z

[error] -- Error: /home/runner/work/mlscript/mlscript/hkmc2DiffTests/src/test/scala/hkmc2/JSBackendDiffMaker.scala:293:38 
[error] 293 |                    case sym => Value.Ref(sym, N),
[error]     |                                ^^^^^^^^^
[error]     |object Ref in object Value is deprecated: Use Value.SimpleRef, Value.MemberRef, or Value.This instead.
[error] one error found
[error] (hkmc2DiffTests / Test / compileIncremental) Compilation failed

I have no idea what's up with the CI failure. It is only deprecated but it's treated as an error for some reason.

Derppening · 2026-05-16T05:59:20Z

@LPTK You can take a look at the changes first. I will mark this as ready for review once the TODO is also addressed.

LPTK

I feel like this should have been marked as a draft and that you didn't properly review the diff. Given the amount of duplicated logic, a lot of it looks like thoughtless slop 😕

PS: thoughtless slop is perfectly fine as long as the PR is marked as Draft and you eventually clean it up 😉

LPTK · 2026-05-16T08:53:10Z

      override def applyValue(v: Value): Unit =
        v match
-        case Value.Ref(l, disamb) if !inCtx(l) && l.asClsLike.isEmpty => freeVars.add(l)
+        case Value.SimpleRef(l) if !inCtx(l) && l.asClsLike.isEmpty => freeVars.add(l)


I don't think asClsLike can be true for SimpleRef. Just look at its impl.

LPTK · 2026-05-16T08:55:20Z

-        case Value.Ref(l, disamb) if !inCtx(l) && l.asClsLike.isEmpty => freeVars.add(l)
+        case Value.SimpleRef(l) if !inCtx(l) && l.asClsLike.isEmpty => freeVars.add(l)
+        case Value.MemberRef(bms, _) if !inCtx(bms) && bms.asClsLike.isEmpty => freeVars.add(bms)
+        case Value.InnerRef(l) if !inCtx(l) && l.asClsLike.isEmpty => freeVars.add(l)


I think this will never meaningfully match. this references are almost always to a class-ike thing. @ychenfo what's the intent, here?

LPTK · 2026-05-16T08:56:53Z

+      case bms: BlockMemberSymbol => Value.MemberRef(bms, bms.defaultDisamb.get)
+      case sym: TempSymbol => Value.SimpleRef(sym)
+      case sym: VarSymbol => Value.SimpleRef(sym)
+      case sym: (LocalSymbol | BuiltinSymbol) => Value.SimpleRef(sym)
+      case sym: InnerSymbol => Value.InnerRef(sym)


This jungle of cases is almost certainly unnecessary. Check what type of symbol flows here and split/refine the type of this extension method, isntead of using pattern matching. Perhaps @ychenfo can help.

LPTK · 2026-05-16T08:58:02Z

              case _ => ()
            generatedProdVars(sym).asProdStrat
-          case _: Value.Ref => lastWords("already handled in `RefLike` case")
+          case _: (Value.SimpleRef | Value.MemberRef) => lastWords("already handled in `RefLike` case")


I see that this Value.SimpleRef | Value.MemberRef pattern seems common enough to make them extend a base Value.Ref sealed trait that could be used here instead.

LPTK · 2026-05-16T09:00:08Z

+        classCtorSymbol(sym) orElse S(sym)
+        classCtorSymbol(sym) orElse S(sym)


Duplicated line...

LPTK · 2026-05-16T09:12:38Z

+                  sym match
+                    case sym: TempSymbol => Value.SimpleRef(sym)
+                    case sym: VarSymbol => Value.SimpleRef(sym)
+                    case sym: BlockMemberSymbol => Value.MemberRef(sym, sym.defaultDisamb.get)
+                    case sym: (LocalSymbol | BuiltinSymbol) => Value.SimpleRef(sym)
+                    case sym: InnerSymbol => Value.InnerRef(sym)


Again some duplicated (and suspicious) logic.

LPTK · 2026-05-16T09:12:46Z

@@ -1,6 +1,5 @@
 :js

-


LPTK · 2026-05-16T09:13:57Z

+    case Value.SimpleRef(l) =>
      mapping.get(l) match
        case None => super.applyValue(v)(k)
        case Some(newBms: BlockMemberSymbol) =>


Will the mapping ever map a SimpleRef symbol to a BlockMemberSymbol?! 🤔

LPTK · 2026-05-16T09:14:23Z

    warnStmt
-    k(loweringCtx(Value.Ref(sym, disamb).withLocOf(ref)))
+    (sym, disamb) match
+      case (sym: TopLevelSymbol, _) =>


Useless case.

LPTK · 2026-05-16T09:15:02Z

+      case (sym: TempSymbol, _) =>
+        k(loweringCtx(Value.SimpleRef(sym).withLocOf(ref)))
+      case (sym: VarSymbol, _) =>
+        k(loweringCtx(Value.SimpleRef(sym).withLocOf(ref)))


Collapse using sym: (TempSymbol | VarSymbol)

Derppening · 2026-05-17T03:31:15Z

Sorry for all the redundant logic - I will convert this as a draft to clean this up first, then when it's ready I will re-request for your review.

`LocalSymbol | BuiltinSymbol` cannot be class-like.

Derppening and others added 20 commits May 16, 2026 00:40

WIP: Deprecate Value.Ref

f72d79a

WIP: Refactor Value.Ref(TopLevelSymbol, _) to Value.This

7e8e448

WIP: Refactor Value.Ref(BuiltinSymbol, _) to Value.SimpleRef

f7a9ff0

WIP: Refactor Value.Ref(TempSymbol, _) to Value.SimpleRef

cb83172

WIP: Refactor Value.Ref(VarSymbol, _) to Value.SimpleRef

dc0708f

WIP: Refactor Value.Ref(BlockMemberSymbol, _) to Value.MemberRef

6771400

Apply refactoring on more usages of Value.Ref

b4a64a3

WIP: Refactor Value.Ref(InnerSymbol, _) into Value.InnerRef

9bf92a3

WIP: Broaden SimpleRef to LocalSymbol | BuiltinSymbol

2f18203

WIP: Refactor SpecializedSwitch

11ccae1

WIP: Add assertions for TypeAliasSymbol and ErrorSymbol

2ed7327

Explicitly construct MemberRef in ReflectionInstrumenter

2768bb2

Make disamb for Value.MemberRef mandatory

d241913

Refine assertion for Ref(FlowSymbol, _)

5e785f6

Remove stale comment

d71139e

Lower Value.Ref(InnerSymbol, _) to Value.InnerRef

66b2ac5

Add other Value*Ref variants to staged codegen

9d717e0

Move away from Value.Ref(NoSymbol, _)

87d76f7

Remove stale comments

05565d2

Minor error/deprecation message update

8a4f7a3

Derppening added 3 commits May 16, 2026 14:59

Revert HkScratch.mls to upstream version

30f9710

Migrate away from Value.Ref in pattern matching arms

85e68a5

Remove remaining constructions of Value.Ref

c6dd687

Derppening marked this pull request as ready for review May 16, 2026 07:57

Derppening requested a review from LPTK May 16, 2026 07:57

Merge branch 'hkmc2' into refactor/ref-symbol

5192069

LPTK requested changes May 16, 2026

View reviewed changes

Derppening marked this pull request as draft May 17, 2026 03:31

Derppening and others added 7 commits May 18, 2026 15:06

test: Restore missing blank line

9f6e89e

codegen/Block: Remove redundant logic for globalThis

0a1c572

codegen/deforest: Remove redundant asClsLike guard

604cffe

`LocalSymbol | BuiltinSymbol` cannot be class-like.

codegen/SymbolRefresher: Simplify match expression

b7a7c47

codegen/Lowering: Simplify patterns

0738f09

codegen: Fully remove case Value.Ref, refactor remaining constructions

6b284ac

Merge branch 'hkmc2' into refactor/ref-symbol

880748b

		classCtorSymbol(sym) orElse S(sym)
		classCtorSymbol(sym) orElse S(sym)

Conversation

Derppening commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Derppening commented May 16, 2026

Uh oh!

Derppening commented May 16, 2026

Uh oh!

LPTK left a comment • 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!

Derppening commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Derppening commented May 16, 2026 •

edited

Loading

LPTK left a comment •

edited

Loading