Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix constant expressions sometimes having the wrong value #1198

Merged
merged 2 commits into from
Feb 18, 2024

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Feb 18, 2024

Summary

Fix a critical bug with the constant-data table, which led to, in rare
cases, constant aggregate construction expressions evaluating to the
wrong value at run-time. All backends were affected.

Details

The MirNode comparison procedure of DataTable didn't take all
relevant fields into account, meaning that unequal mnkObjConstr,
mnkConstr, and mnkField were treated as equal. For example, the
comparison would result in 'true' for Obj(a: 1) and Obj(b: 1).

This problem was hidden by the hashing procedure properly considering
all relevant MirNode fields; only when there were hash or bucket
collisions did the equality problem surface. All fields are taken into
account by datatables.== now, and the used case statement is made
exhaustive in order to prevent similar issues in the future.

Since a language-level test would be rather contrived and brittle (a
hash/bucket collision is required), a unit test for the comparison
used by datatables is added.

Only the comparison operation was affected, `hash` already worked
correctly.
A language-level test for the bug would be rather contrived and
brittle, so instead, a unit-like test is added for `datatables.cmp`.
@zerbina zerbina added bug Something isn't working compiler/backend Related to backend system of the compiler labels Feb 18, 2024
@zerbina zerbina added this to the MIR phase milestone Feb 18, 2024
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

LoL, I was going to ask today about how to trace a few more things about mirgen. Because the mir tree being fetched on the jsgen side made no sense given that the actual mirgen for the constructor expression seemed fine. I was stuck on that bit last night. Thanks for the fix, I totally didn't think that it might be constant hashing/lookup. 😅

@saem
Copy link
Collaborator

saem commented Feb 18, 2024

/merge

Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue Feb 18, 2024
Merged via the queue into nim-works:devel with commit ee40a2f Feb 18, 2024
31 checks passed
@zerbina zerbina deleted the fix-datatables-node-comparison branch March 1, 2024 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants