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

refactor Uninhabited implementation for Elem types #3412

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ihor-rud
Copy link

Description

Refactor Uninhabited implementation for Data.List.Elem, Data.List1.Elem, Data.SnocList.Elem and Data.Vect.Elem so it can be used for homogeneous and heterogeneous equality.

@@ -19,11 +19,11 @@ data Elem : a -> List a -> Type where
There : Elem x xs -> Elem x (y :: xs)

export
Uninhabited (Here = There e) where
Uninhabited (Here {x} {xs} = There {x = x'} {y} {xs} e) where
Copy link
Member

Choose a reason for hiding this comment

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

If you're going fully heterogeneous, why not have two different xs on each sides?

Copy link
Author

Choose a reason for hiding this comment

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

good point. xs are now different

Copy link
Author

Choose a reason for hiding this comment

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

Since xs are now different maybe we should add something like that?
Either (Not (x = x')) (Not xs = xs') => Uninhabited (Here {x} {xs} = Here {x = x'} {xs = xs'}) where
Basically, if x or xs are different, Here and There will be different as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either (Not (x = x')) (Not xs = xs') => Uninhabited (Here {x} {xs} = Here {x = x'} {xs = xs'})

I think this can be proven for any Biinjective, not only for this particular one

@ihor-rud ihor-rud requested a review from gallais November 18, 2024 18:46
Copy link
Collaborator

@CodingCellist CodingCellist left a comment

Choose a reason for hiding this comment

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

This might be more of a stylistic choice, I'm not sure, but I would personally prefer if at least one of the test code files used the explicit syntax for which equality was being used (i.e. === or ~=~), just so we have something clearly showing the different ones - since that was what caused the original confusion on the discord - as well as testing the magic elaboration of = to the hopefully correct equality : )

@@ -227,6 +227,8 @@ This CHANGELOG describes the merged but unreleased changes. Please see [CHANGELO

* Added `Data.IORef.atomically` for the chez backend.

* Refactor `Uninhabited` implementation for `Data.List.Elem`, `Data.List1.Elem`, `Data.SnocList.Elem` and `Data.Vect.Elem`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind adding a brief sentence explaining why / explaining what the refactor achieves?

Copy link
Author

Choose a reason for hiding this comment

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

done

@ihor-rud
Copy link
Author

This might be more of a stylistic choice, I'm not sure, but I would personally prefer if at least one of the test code files used the explicit syntax for which equality was being used (i.e. === or ~=~), just so we have something clearly showing the different ones - since that was what caused the original confusion on the discord - as well as testing the magic elaboration of = to the hopefully correct equality : )

I added tests for all combinations

Copy link
Collaborator

@CodingCellist CodingCellist left a comment

Choose a reason for hiding this comment

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

Thank you for making the minor changes, I'm happy with it as-is now. If nobody has any other comments before Friday at noon GMT (UTC+0), I'll merge things around then : )

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.

4 participants