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: False positive in type equality #75

Merged
merged 2 commits into from
Jan 14, 2025
Merged

fix: False positive in type equality #75

merged 2 commits into from
Jan 14, 2025

Conversation

croyzor
Copy link
Collaborator

@croyzor croyzor commented Jan 13, 2025

When comparing types, the easy case is not when the variables are the same, but rather when the haskell representations of the values are equal! I've added a test case that would have typechecked before to demonstrate.

@croyzor croyzor requested a review from mark-koch January 13, 2025 12:43
Copy link
Collaborator

@acl-cqc acl-cqc left a comment

Choose a reason for hiding this comment

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

Ooops, my bad, but I'm puzzled why this doesn't work.
ends are both

data End = InEnd InPort | ExEnd OutPort
 deriving (Eq, Ord)

where

data OutPort = Ex Name Int deriving (Eq, Ord, Show)
data InPort = In Name Int deriving (Eq, Ord, Show)

So I believed the previous code would be true only for variables being the same port (Int) of the same node (Name). Obviously that isn't what happens (I agree that without this fix your "golden test" compiles, when it shouldn't) but why?

@croyzor
Copy link
Collaborator Author

croyzor commented Jan 13, 2025

Both of the metavariables in the example are defined in terms of the X and n input variables, so they're both Ex. Note that in this PR I'm not concerned about the direction of the wire, but rather that Ends contained in two values being equal doesn't make the values equal.
I.e. X != 1 + X or f != f(42), where X, f are metavariables

brat/Brat/Syntax/Value.hs Outdated Show resolved Hide resolved
@@ -159,6 +160,31 @@ data Val :: N -> Type where
VApp :: VVar n -> Bwd (Val n) -> Val n
VSum :: MODEY m => Modey m -> [Some (Ro m n)] -> Val n -- (Hugr-like) Sum types

-- Define a naive version of equality, which only says whether the data
-- structures are on-the-node equal
instance Eq (Val n) where
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it not possible to derive Eq?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, because we have things like VFun with an existential mode, which means GHC has to test that the modes are equal on both sides before it can tell that the CTys actually have the same type!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also note there's no Eq for VSum because that one is very tricky, and it's only produced by the compiler so I doubt we'll need to test equality for it (I'll add a comment saying this)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we even use VSum? I thought we got rid of it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😮 I didn't realise we stopped using it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#76 saves me having to add a comment

@croyzor croyzor merged commit 30bdc9f into main Jan 14, 2025
2 checks passed
@croyzor croyzor deleted the fix/typeEq branch January 14, 2025 09:47
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.

3 participants