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

New (simpler but more robust) implementation of canonicity check #354

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

jespercockx
Copy link
Member

This is an attempt to resolve the issue with the canonicity check that I encountered in #350 by implementing it in a completely different way. Instead of re-inferring the instance and checking it for equality with the given one, we instead traverse the term and check that its head symbol is an instance (either a global instance Def or a local instance Var), and that all its recursive instance arguments are canonical too.

Due to the upstream issue agda/agda#7449, this has the annoying effect that instances need to be given in a form that does not reduce unless projected, or if they reduce then the reduct should be a valid instance itself. This should not be a problem for user-written instances, but for the instances in the Prelude it means we cannot make use of most of the tricks that were there to avoid boilerplate. Instead, I just bit the bullet and made the code in it quite a bit more verbose in order to make it pass the new canonicity check.

Happy to get feedback on the general idea as well as the implementation details.

Copy link
Contributor

@flupe flupe left a comment

Choose a reason for hiding this comment

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

With regards to instances being inlined, can this be prevented by annotating with {-# NO_INLINE #-}? Alternatively, since mkApplicative was marked as private, we could assume that all uses of mkApplicative in instance arguments must arise from instance search, so we could hardcode these helpers from the prelude to be fine (TM) though I get why this would be annoying.

This should not be a problem for user-written instances.

It would be if we wanted to use the same kind of tricks for a custom class, right?

Regardless, nice!

src/Agda2Hs/Compile/Utils.hs Show resolved Hide resolved
@jespercockx
Copy link
Member Author

With regards to instances being inlined, can this be prevented by annotating with {-# NO_INLINE #-}?

That doesn't help as the terms are actually reduced to whnf, on which NO_INLINE has no effect.

since mkApplicative was marked as private, we could assume that all uses of mkApplicative in instance arguments must arise from instance search, so we could hardcode these helpers from the prelude to be fine (TM)

Yeah in hindsight perhaps this would've been easier. But now the work is done anyway.

It would be if we wanted to use the same kind of tricks for a custom class, right?

These tricks would not be accepted for instances that are actually compiled to Haskell. However, it could be a problem if a user wants to mirror instances that already exist in Haskell on the Agda side.

@jespercockx
Copy link
Member Author

Since there are no objections I will merge this now, so we can continue to update the Agda base version to 2.7.0.

@jespercockx jespercockx merged commit 03c9aae into agda:master Sep 7, 2024
7 checks passed
jespercockx added a commit to jespercockx/agda2hs that referenced this pull request Sep 10, 2024
jespercockx added a commit to jespercockx/agda2hs that referenced this pull request Sep 11, 2024
jespercockx added a commit to jespercockx/agda2hs that referenced this pull request Sep 19, 2024
jespercockx added a commit to jespercockx/agda2hs that referenced this pull request Sep 19, 2024
@jespercockx jespercockx added this to the 1.3 milestone Sep 24, 2024
jespercockx added a commit to jespercockx/agda2hs that referenced this pull request Sep 24, 2024
jespercockx added a commit to jespercockx/agda2hs that referenced this pull request Sep 25, 2024
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.

2 participants