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: inconsistent/incorrect implementations of Ord, PartialOrd, PartialEq and Eq leading to panics on sort #28

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

davidsemakula
Copy link
Contributor

Description

Since Rust >= 1.81, sort algorithms panic when they detect inconsistent/incorrect implementations of Ord, PartialOrd, PartialEq and Eq traits.

This PR:

  • Updates the AbstractValue, FunctionReference, Path and PathEnum types to ensure that their implementations of the above four traits are consistent.
  • Ensures that the Hash implementation (where applicable) is also consistent with Eq (and hence PartialEq) for the aforementioned types.
  • Fixes panicking implementations of Ord for the FunctionReference and PathEnum types.

References:
https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html#new-sort-implementations
https://doc.rust-lang.org/nightly/std/cmp/trait.Ord.html#how-can-i-implement-ord
https://github.com/rust-lang/rust/blob/1.81.0/library/core/src/slice/sort/shared/smallsort.rs#L847-L863
https://doc.rust-lang.org/nightly/std/hash/trait.Hash.html#hash-and-eq

Fixes # (issue)
N/A

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • API change with a documentation update
  • Additional test coverage
  • Code cleanup or just keeping up with the latest Rustc nightly

How Has This Been Tested?

A contrived unit test for AbstractValue has been added.

Panics typically happen at these sort operations in the summarize function.

Suggestions for how to implement more and/or better tests are welcome.
In general, it's quite complicated (for me anyway) to create minimal tests that cause MIRAI to visit summarize in these inconsistent states. However, this repo from this recent issue is also affected.

Checklist:

  • Fork the repo and create your branch from main.
  • If you've added code that should be tested, add tests.
  • If you've changed APIs, update the documentation.
  • Ensure the test suite passes.
  • Make sure your code lints.

…PartialEq` and `Eq` traits leading to panics on sort
@davidsemakula davidsemakula changed the title Fix: inconsistent/incorrect implementations of Ord, PartialOrd, PartialEq and Eq traits leading to panics on sort Fix: inconsistent/incorrect implementations of Ord, PartialOrd, PartialEq and Eq leading to panics on sort Jan 3, 2025
Copy link
Collaborator

@hermanventer hermanventer left a comment

Choose a reason for hiding this comment

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

Looks good.

@hermanventer hermanventer merged commit e756a1e into endorlabs:main Jan 3, 2025
6 checks passed
@davidsemakula davidsemakula deleted the consistent-ord-impls branch January 4, 2025 05:46
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