Skip to content

Fix nondeterministic private field ordering in Lifter#459

Merged
LPTK merged 10 commits intohkust-taco:hkmc2from
LPTK:copilot/fix-private-fields-order
Apr 15, 2026
Merged

Fix nondeterministic private field ordering in Lifter#459
LPTK merged 10 commits intohkust-taco:hkmc2from
LPTK:copilot/fix-private-fields-order

Conversation

@LPTK
Copy link
Copy Markdown
Contributor

@LPTK LPTK commented Apr 14, 2026

The private fields generated by the lifter had nondeterministic ordering because they were built by iterating over Map values derived from Set operations. Scala's Set and Map do not guarantee stable iteration order.

The fix uses sorted ordered lists (passedSymsOrdered, capturesOrdered, passedDefnsOrdered, and new liftedObjsOrdered) to look up map values in a deterministic order based on symbol UIDs, instead of iterating map values directly.

Agent-Logs-Url: https://github.com/LPTK/mlscript/sessions/07ac81ed-dfff-467c-af13-4a25818d2faa


Original PR comment by Copilot:

Private field order in generated code was nondeterministic across runs, caused by iterating Scala Map.values / Set elements which have no guaranteed iteration order. This manifested as "bump test" commits that only reordered fields like and swapping.

Changes

  • LiftedClass.extraPrivSyms: Replaced Map.values concatenation with lookups through the already-sorted passedDefnsOrdered, capturesOrdered, passedSymsOrdered lists — matching the ordering convention used by auxParams and formatArgs
// Before: nondeterministic Map.values iteration
private val extraPrivSyms = 
  liftedObjsSyms.values ++ passedSymsMap_.values.map(_.ts)
  ++ capSymsMap_.values.map(_.ts) ++ defnSymsMap_.values.map(_.ts)

// After: deterministic uid-sorted lookups
private lazy val extraPrivSyms: List[TermSymbol] = 
  liftedObjsOrdered.map(liftedObjsSyms)
  ::: passedDefnsOrdered.map(defnSymsMap_(_).ts)
  ::: capturesOrdered.map(capSymsMap_(_).ts)
  ::: passedSymsOrdered.map(passedSymsMap_(_).ts)
  • RewrittenClass / RewrittenCompanion: Replaced liftedObjsSyms.values.toList with liftedObjsOrdered.map(liftedObjsSyms) in privateFields construction
  • GenericRewrittenScope / ClsLikeRewrittenScope: Added liftedObjsOrdered: List[InnerSymbol] sorted by uid, paralleling the existing passedSymsOrdered / capturesOrdered / passedDefnsOrdered pattern

The private fields generated by the lifter had nondeterministic ordering
because they were built by iterating over Map values derived from Set
operations. Scala's Set and Map do not guarantee stable iteration order.

The fix uses sorted ordered lists (passedSymsOrdered, capturesOrdered,
passedDefnsOrdered, and new liftedObjsOrdered) to look up map values in
a deterministic order based on symbol UIDs, instead of iterating map
values directly.

Agent-Logs-Url: https://github.com/LPTK/mlscript/sessions/07ac81ed-dfff-467c-af13-4a25818d2faa
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses nondeterministic ordering of generated private fields in the lifter by avoiding iteration over Set/Map.values (which have no guaranteed stable order) and instead indexing map entries via UID-sorted key lists. This reduces “no-op” diffs in generated output caused by field reordering.

Changes:

  • Introduce liftedObjsOrdered (UID-sorted) and use it instead of liftedObjsSyms.values.toList when emitting privateFields for rewritten classes/companions.
  • Make LiftedClass.extraPrivSyms deterministic by building it via the already-ordered key lists (passedDefnsOrdered, capturesOrdered, passedSymsOrdered) instead of Map.values.
  • Update the golden lifter output test to reflect the new stable field order.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala Reworks private-field list construction to use UID-sorted keys rather than Map.values iteration.
hkmc2/shared/src/test/mlscript/lifter/ClassInFun.mls Updates expected output ordering for private fields in a lifter snapshot test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala Outdated
@LPTK LPTK closed this Apr 14, 2026
@LPTK LPTK deleted the copilot/fix-private-fields-order branch April 14, 2026 04:22
@LPTK LPTK restored the copilot/fix-private-fields-order branch April 14, 2026 04:22
@LPTK
Copy link
Copy Markdown
Contributor Author

LPTK commented Apr 14, 2026

Argh, closed because I mistakenly deleted the source branch...

@LPTK LPTK reopened this Apr 14, 2026
…le ScopedInfo ordering

Sort reqCaptures by the stable UIDs of the original symbols/scopes
(via a new given Ordering[ScopedInfo]) instead of by freshly-created
VarSymbol UIDs that depend on nondeterministic Set iteration order.
Also iterate reqCaptures in sorted order when building capSymsMap_
so that VarSymbol UID allocation is deterministic.

Agent-Logs-Url: https://github.com/LPTK/mlscript/sessions/27dbdac4-4044-4c61-b8ca-808142c295c6

Co-authored-by: LPTK <2545083+LPTK@users.noreply.github.com>
@LPTK
Copy link
Copy Markdown
Contributor Author

LPTK commented Apr 14, 2026

@CAG2Mark Copilot has annoying restrictions on cross repo PRs, so the PR continues there: LPTK#18

Copy link
Copy Markdown
Contributor

@CAG2Mark CAG2Mark left a comment

Choose a reason for hiding this comment

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

LGTM, but I have comments on the other PR

Also see the above comment by Copilot, which is valid.

Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala
…Uid[Symbol]] instead of toInt

- capSymsMap_ reverted to iterate reqCaptures directly (sorting before
  .toMap has no effect on the resulting Map)
- Use existing given Ordering[Uid[A]] instead of adding toInt extension
- Remove toInt from Uid.scala

Agent-Logs-Url: https://github.com/LPTK/mlscript/sessions/da829502-087c-4ed2-bf08-2d2834b1f5a4

Co-authored-by: LPTK <2545083+LPTK@users.noreply.github.com>
CAG2Mark and others added 3 commits April 15, 2026 18:55
…into copilot/fix-deterministic-ordering

# Conflicts:
#	hkmc2/shared/src/test/mlscript/handlers/Effects.mls
# Conflicts:
#	hkmc2/shared/src/test/mlscript/codegen/FirstClassFunctionTransform.mls
@LPTK LPTK force-pushed the copilot/fix-private-fields-order branch from be9bd44 to 6f91b57 Compare April 15, 2026 13:34
Copy link
Copy Markdown
Contributor

@CAG2Mark CAG2Mark left a comment

Choose a reason for hiding this comment

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

Pls merge this first and then it will be good to go: https://github.com/CAG2Mark/mlscript/commits/fix-ordering-2/

Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala Outdated
Co-authored-by: Mark Ng <55091936+CAG2Mark@users.noreply.github.com>
@LPTK LPTK merged commit d08014c into hkust-taco:hkmc2 Apr 15, 2026
1 check passed
@LPTK LPTK deleted the copilot/fix-private-fields-order branch April 15, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants