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.

Sign up for GitHub

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

Jump to bottom

split method is ambiguous on its own #60

Open
mx00s opened this issue Nov 13, 2024 · 1 comment · May be fixed by #61
Open

split method is ambiguous on its own #60

mx00s opened this issue Nov 13, 2024 · 1 comment · May be fixed by #61

Comments

@mx00s
Copy link

mx00s commented Nov 13, 2024 •
edited
Loading

Calling split on a single-element NonEmpty returns the same thing as a two-element NonEmpty when all the elements are the same.

    #[test]
    fn non_empty_split() {
        let one_element: NonEmpty<i32> = NonEmpty {
            head: 0,
            tail: vec![],
        };
        assert_eq!(one_element.len(), 1);

        let two_elements: NonEmpty<i32> = NonEmpty {
            head: 0,
            tail: vec![0],
        };
        assert_eq!(two_elements.len(), 2);

        // This assertion fails because they return the same thing, namely `(0, &[], 0)`!
        assert_ne!(one_element.split(), two_elements.split());
    }

Although this situation can be disambiguated, e.g. by checking the length, it's a potential source of confusion and bugs.

Changing the return type to (&T, &[T], Option<&T>) and returning (0, &[], None) for the one-element case and (0, &[], Some(0)) for the two-element case would resolve the ambiguity.
@FintanH
Copy link
Collaborator

FintanH commented Nov 14, 2024

That makes sense 👍 I'll add a patch to change it to return Option for the last element

FintanH added a commit that referenced this issue Nov 14, 2024
@FintanH
Improve return type of NonEmpty::split
645cb0c 
Fixes #60[^issue-60].

As pointed out in #60, splitting a `NonEmpty` that contains a single
element can be ambiguous to a `NonEmpty` equivalent to `(x, [x])`. A
better representation for this return type is `(&T, &[T],
Option<&T>)`.

[^issue-60]: #60
Signed-off-by: Fintan Halpenny <[email protected]>
X-Clacks-Overhead: GNU Terry Pratchett
@FintanH FintanH linked a pull request Nov 14, 2024 that will close this issue
Open
FintanH added a commit that referenced this issue Nov 14, 2024
@FintanH
Improve return type of NonEmpty::split
df0381c 
Fixes #60[^issue-60].

As pointed out in #60, splitting a `NonEmpty` that contains a single
element can be ambiguous to a `NonEmpty` equivalent to `(x, [x])`. A
better representation for this return type is `(&T, &[T],
Option<&T>)`.

[^issue-60]: #60
Signed-off-by: Fintan Halpenny <[email protected]>
X-Clacks-Overhead: GNU Terry Pratchett
FintanH added a commit that referenced this issue Nov 15, 2024
@FintanH
Improve return type of NonEmpty::split
b4083ff 
Fixes #60[^issue-60].

As pointed out in #60, splitting a `NonEmpty` that contains a single
element can be ambiguous to a `NonEmpty` equivalent to `(x, [x])`. A
better representation for this return type is `(&T, &[T],
Option<&T>)`.

[^issue-60]: #60
Signed-off-by: Fintan Halpenny <[email protected]>
X-Clacks-Overhead: GNU Terry Pratchett
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve return type of NonEmpty::split cloudhead/nonempty
2 participants
@FintanH @mx00s