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

Document invariant on Ranges #277

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Document invariant on Ranges #277

merged 2 commits into from
Nov 11, 2024

Conversation

konstin
Copy link
Member

@konstin konstin commented Nov 8, 2024

Ranges keeps up certain invariants internally, we document which they are and why they exist. Currently, they can only be observed by iterating over segments.

Split out from #273.

///
/// These ensure that equivalent instances have an identical representation, which is important
/// for `Eq` and `Hash`. Given that this type doesn't know the lower bound, upper bound, granularity
/// or actual instances of `V`, this applies only to equality under pubgrub's model.
Copy link
Member

Choose a reason for hiding this comment

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

this applies only to equality under pubgrub's model.

Is there a word missing here? I'm not sure what this was trying to say.

@Eh2406
Copy link
Member

Eh2406 commented Nov 8, 2024

@mpizenberg is our resident wordsmith, so I will leave it up to him.

@mpizenberg
Copy link
Member

Another way to describe: "list of non-overlapping, non-consecutive, intervals with strictly increasing values".

The described invariants are good to me.

I don’t understand what the last sentence means though. Could you elaborate?

Given that this type doesn't know the lower bound, upper bound, granularity or actual instances of V, this applies only to equality under pubgrub's model.

@mpizenberg
Copy link
Member

Though maybe rule number 1: the sorting doesn’t make much sense until rule 2 and 3 are given. Once we have rules 2 and 3, we know that interval bounds can be sorted. So maybe it should be the last invariant?

@mpizenberg
Copy link
Member

Oh maybe I get it.

Given that this type doesn't know the lower bound, upper bound, granularity or actual instances of V, this applies only to equality under pubgrub's model.

Do you mean that the provided invariants cannot guaranty that sets described by these invariants are actually equal if and only if their representation is equal. This is not an equivalence, just an implication. Because for example, In the set of natural numbers, both these sets representations are the valid under the above rule, yet they are different representation of the same:

  1. [3, 7[
  2. [3, 6]

@mpizenberg
Copy link
Member

That’s why originally, we forced the rule that left bound is inclusive, and right bound is exclusive, and the set of values must have a strict ordering and every "next" value must be known. Which is annoying when you don’t really know what "next" can be.

@mpizenberg
Copy link
Member

mpizenberg commented Nov 9, 2024

What do you think of replacing the last sentence with something like:

Remark: This representation cannot strictly guaranty equality of Ranges with equality of its representation without also knowing the nature of the underlying versions. In particular, if the version space is discrete, different representations, using different types of bounds (exclusive/inclusive) may correspond to the same set of existing versions. It is a tradeoff we acknowledge, but which makes representations of continuous version sets more accessible, to better handle features like pre-releases and other types of version modifiers.

@konstin
Copy link
Member Author

konstin commented Nov 10, 2024

I added an example too so it's more accessible for non-math-background people.

@konstin konstin added this pull request to the merge queue Nov 11, 2024
Merged via the queue into dev with commit 36c2e41 Nov 11, 2024
6 checks passed
@konstin konstin deleted the konsti/dev/document-invariants branch November 11, 2024 14:11
@konstin konstin added the Ranges label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants