-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix deficiencies in meta.hasUniqueRepresentation #25693
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
Conversation
Fix several errors in hasUniqueRepresentation, both false negatives and
false positives. Additionally, improve documentation surrounding the
function, and split its ever-growing unit test into multiple seperate tests.
One of the false positives in question was pointers to comptime-only
types. Their representation is not defined, but to solve this, it was
necessary to introduce a function to determine whether a type is
comptime-only. Since this is a widely-applicable function, I made it
public in meta.isComptimeOnly.
Some rules of thumb for how hasUniqueRepresentation works now:
- False negatives can be returned, but never false positives
- Comptime-only types always return false
- Types with a well-defined layout never return a false negative or a
false positive
- Types with no well-defined layout may return true if the type
provably has a unique representation, but may also return a false
negative
|
Ah, I stepped away from my computer to do something, but in a few minutes I will fix those checks and add tests for what failed |
Remove some debug code left in the hasUniqueRepresentation implementation and fix auto_hash's incorrect usage of hasUniqueRepresentation. (While we're at it, we also gave autoHash a new coat of paint and cleaned it up a little)
|
Unfortunately, it's looking like determining if a type is comptime-only, and by extension determining whether a pointer hasUniqueRepresentation, take unseemly amount of eval branch quota. It looks like for now, we might just have to live with a false positive for pointers to comptime-only types. Language support here seems like it would be ideal. |
isComptimeOnly took up too much eval branch quota to be practical, and some of my changes to auto_hash.zig were admittedly overzealous and out of scope for this branch.
…tut/zig into unique-representation-patch
While technically correct, it seems like this is causing issues with hash maps, so it has to be rolled back, at least for now. Goodnight sweet prince :(
|
I've determined that the build failures come from an incorrect usage of autoHash in |
|
Looks like the above bug actually stems from an alarming stale pointer reference where |
Squashed some bugs and cleaned things up in auto_hash and hasUniqueRepresentation. Additionally, fixed a stale reference bug in std.zig.resolveTargetQuery. This fix required adding an additional allocator parameter to resolveTargetQuery.
Would you mind splitting this into a separate PR? Sounds like a fix we should land ASAP. |
|
For the time being, I will be closing this PR, as the tests have proven more difficult than expected, and I will likely be busy for the foreseeable future |
|
Additionally, there is deeper surgery to be done on related components such as auto_hash and resolveTargetQuery which are out of scope for a single PR anyways |
Fix several errors in hasUniqueRepresentation, both false negatives and false positives. Additionally, improve documentation surrounding the function, and split its ever-growing unit test into multiple seperate tests.
Pointers to comptime-only typesWhile fixing this, I ran into several notable edge cases, so I would recommend any reviewers read the comments I left in hasUniqueRepresentation
One of the false positives in question was pointers to comptime-only types. Their representation is not defined, but to solve this, it was necessary to introduce a function to determine whether a type is comptime-only. Since this is a widely-applicable function, I made it public in meta.isComptimeOnly.^ This has been rolled back, as determining whether a type is comptime-known takes up a massive of eval branch quota such that building any sizable project, including the compiler or even the build system, is impossible when checking this in hasUniqueRepresentation
Some rules of thumb for how hasUniqueRepresentation works now:
Comptime-only types always return falsefalse positive
provably has a unique representation, but may also return a false
negative
PS: While working on this, I ran into some bugs in
std.hash.autoHashwhich ended up incorrectly hashing slices, optional slices, and other types which should not be able to be hashed without specifying a strategy. To resolve this, I ended up refactoringstd/hash/auto_hash.zigand fixing some uses of the API which should have been compile errors.PPS: Also while working on this, I ran into a stale reference bug in
std.zig.resolveTargetQuerythat was causing the CI and behavior tests to fail. To fix this, an additional allocator parameter had to be added to the function. Then, I had to correct every reference of the function within the codebase, hence the all-over-the-place changes.