Skip to content

Unchecked assign case object discriminator#117

Merged
nitely merged 4 commits into
status-im:masterfrom
nitely:unchecked_assign
Oct 24, 2025
Merged

Unchecked assign case object discriminator#117
nitely merged 4 commits into
status-im:masterfrom
nitely:unchecked_assign

Conversation

@nitely

@nitely nitely commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

Fixes #9

  • Adds {.cast(uncheckedAssign).} around case object field discriminator assignment. This should behave the same as -d:nimOldCaseObjects.
  • parseCmdArg(type enum, string): enum includes the first letter in the normalization. So it's consistent with the config argument parsing. I checked and it's not being used in status-im, vac, codex, waku, and it's unlikely to break user code since the enum would need to contain elements such as "Foobar" and "foobar", and then it could not be used as a config argument anyway.
  • -d:nimOldCaseObjects is no longer needed in Nim >= 2.0
  • Nim 1.6 still requires -d:nimOldCaseObjects because fix #19266; allow reassign discriminant field  nim-lang/Nim#19567 was never backported.
  • Using Nimble 0.20.1 to resolve nim-ser requiring nim >= 2.0

@nitely
nitely marked this pull request as ready for review October 15, 2025 19:36
@nitely
nitely requested a review from jangko October 15, 2025 19:36
@nitely
nitely requested a review from arnetheduck October 17, 2025 22:05
@arnetheduck

Copy link
Copy Markdown
Member

the way to really fix this is slightly more complex: one needs to create a "flat" object to hold the union of all fields across all branches, then parse the command line into those objects, then when parsing is done, construct the case object from the flat object picking out the right fields. It's a bit of a mess but the only correct way to do it really with a single parsing pass.

@arnetheduck

Copy link
Copy Markdown
Member

in particular, consider if you have a case object on branch A and one of the fields is a string/seq/ref and is currently assigned - when switching to branch B, those fields need to be destroyed and afair this does not work correctly under orc/arc, which is why the syntax was .. discouraged.

@nitely

nitely commented Oct 24, 2025

Copy link
Copy Markdown
Contributor Author

Given this (AFAICT) has the same behavior as -d:nimOldCaseObjects, merging this should not harm. I've added a comment to rework it into a more general solution as discussed privately.

@nitely
nitely merged commit 20ae94b into status-im:master Oct 24, 2025
24 checks passed
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.

"assignment to discriminant changes object branch"

2 participants