-
Notifications
You must be signed in to change notification settings - Fork 37
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 for #357 #358
Fix for #357 #358
Conversation
49dfa3d
to
088d4ef
Compare
constructor Erased | ||
field @0 get : a | ||
instance constructor Erased | ||
field @0 {{get}} : a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation for changing the interface of Erase
and making the field an instance? Seems like it forces explicit brackets {{}}
everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it was to make sure that instance search could actually find an element of type Erase A
when there is an erased instance of type A
in scope. But I found a better way to get the same effect, which does not break backwards compatibility (see newly pushed version).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a situation that arises often? Having instances of a
laying around? For equality proofs I get, but otherwise not so sure. But I trust you that the interface hasn't changed :). (I would have kept the old Erase
definition and defined iErased
separately but it probably doesn't matter much. And with yours one can decide to match and directly introduce an instance, which I don't know if you can do otherwise)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opposite way (defining Erased
as the constructor and iErased
as the pattern synonym) is not accepted by Agda:
.../agda2hs/lib/Haskell/Extra/Erase.agda:22,21-22: error: [IllegalInstanceVariableInPatternSynonym]
Variable is bound as instance in pattern synonym, but does not
resolve as instance in pattern: x
when scope checking the declaration
pattern iErased ⦃ x ⦄ = Erased x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! The errors are perhaps not ideal to get what's wrong but I understand it would be annoying to make them clearer.
088d4ef
to
bbf84b3
Compare
There's a problem with this PR, namely that it breaks the use of the open import Haskell.Prelude
test : Bool → Bool
test = not <$> not
{-# COMPILE AGDA2HS test #-} Error message:
The type argument to The only way I can think of to prevent the eta-expansion on the Agda side is to wrap the function type in a (transparent) record type. |
…fore dropping them
I tried to fix the problem with the record Function (a b : Set) : Set where
field
apply : a → b
open Function public
{-# COMPILE AGDA2HS Function unboxed #-} but unfortunately it seems we do not properly support partially applied unboxed record types. Trying to compile the
(the same problem happens for partially applied tuple types.) It is actually not so obvious how Second thought: if we are anyway hard-coding things into |
… x)` to `(->)`
bbf84b3
to
2c9ba29
Compare
I implemented this idea, replacing |
This fixes #357 by adding a check that type arguments are valid Haskell types before dropping them. It also makes some changes to the Prelude (in particular to
guard
) so it conforms to this new check.