Skip to content

Add invariant checking #444

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

Merged
merged 21 commits into from
May 3, 2022
Merged

Add invariant checking #444

merged 21 commits into from
May 3, 2022

Conversation

sjakobi
Copy link
Member

@sjakobi sjakobi commented Apr 26, 2022

Addresses #366.


TODO:

  • Add more tests
  • Document invariants
  • Extract validation code into D.HM.I.Debug?!

@sjakobi
Copy link
Member Author

sjakobi commented Apr 26, 2022

I've checked that this tooling could have caught #420. Sample output:

        intersection produces valid HashMap: FAIL
          *** Failed! Falsified (after 61 tests and 10 shrinks):
          fromList [(K {hash = 265, _x = D},()),(K {hash = 16649, _x = A},())]
          fromList [(K {hash = 265, _x = D},()),(K {hash = 16649, _x = A},())]
          Invalid (INV2_misplaced_hash 265) (Cons 0 Root) /= Valid
          Use --quickcheck-replay=665566 to reproduce.
          Use -p '/intersection/&&/Data.HashMap.Lazy.difference and intersection.intersection produces valid HashMap/' to rerun this test only.

@sjakobi
Copy link
Member Author

sjakobi commented Apr 26, 2022

Hmm, is there another bug in the new intersection implementation?! oO

@sjakobi
Copy link
Member Author

sjakobi commented Apr 26, 2022

Hmm, is there another bug in the new intersection implementation?! oO

Probably not. I'm getting the same errors with the naive old intersection implementation. I must have made a mistake in the validation logic.

@sjakobi sjakobi force-pushed the sjakobi/366-invariants branch from a23fed8 to 214711c Compare April 27, 2022 15:56
@sjakobi sjakobi force-pushed the sjakobi/366-invariants branch from ebb0cc2 to b03df23 Compare April 27, 2022 21:28
@sjakobi sjakobi marked this pull request as ready for review April 29, 2022 16:52
@sjakobi
Copy link
Member Author

sjakobi commented Apr 29, 2022

This should be ready for review. I'll hold off on adding more validity tests until I've refactored the existing property tests a bit.

hashMatchesSubHashPath (SubHashPath ph l) h = maskToLength h l == ph
where
-- Note: This needs to use `shiftL` instead of `unsafeShiftL` because
-- @l'@ may be greater than 32/64 at the deepest level.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Greater than, or just equal to?

Copy link
Member Author

Choose a reason for hiding this comment

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

l is 65 when the deepest leaves are checked on a 64-bit computer.

@sjakobi sjakobi merged commit 70fdaa7 into master May 3, 2022
@sjakobi sjakobi deleted the sjakobi/366-invariants branch May 3, 2022 14:31
@treeowl
Copy link
Collaborator

treeowl commented May 3, 2022

Huzzah!

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.

2 participants