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 recRecursive when record isn't recursive #7042

Closed
wants to merge 1 commit into from
Closed

Conversation

flupe
Copy link
Contributor

@flupe flupe commented Dec 24, 2023

A bug in agda/agda2hs#257 is caused by it relying on TypeChecking/Monad/Base:recRecursive which may fail when recMutual is set to Nothing.

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Not sure what you are after here, but I can't see how this makes sense.

If you want to query recRecursive before the mutuality computation has run, maybe you want to change the type to Defn -> Maybe Bool.

Currently, the invariant is that recRecursive is never queried before it has been set to Just _.

@@ -2960,7 +2960,7 @@ instance Pretty ProjLams where

-- | Is the record type recursive?
recRecursive :: Defn -> Bool
recRecursive (Record { recMutual = Just qs }) = not $ null qs
recRecursive (Record { recMutual = mqs }) = any (not . null) mqs
Copy link
Member

Choose a reason for hiding this comment

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

Well, the semantics of recMutual = Nothing is that nothing is known about mutualilty yet.
If you want not mutual you have to make sure this gets set to Just [].

@flupe
Copy link
Contributor Author

flupe commented Dec 25, 2023

Are you saying that all record definitions should have recMutual set to Just once typechecking is done?

Because the bug from agda2hs I'm refering to comes from a record definition compiled with the backend. We call recRecursive (presumably after typechecking is completed, since it's a backend compile hook) on the given record which definitely still has recMutual set to False Nothing when it reaches the backend.

@andreasabel
Copy link
Member

We call recRecursive (presumably after typechecking is completed, since it's a backend compile hook) on the given record which definitely still has recMutual set to False when it reaches the backend.

(I think you meant to write Nothing, not False.)
Yes, recRecursive should be set latest once all checking phases are completed (type/coverage/termination... checking).

@flupe
Copy link
Contributor Author

flupe commented Dec 26, 2023

Thank you for the clarification. So I guess there is still an issue with the record from agda/agda2hs#257 never getting recMutual populated by Agda before reaching the backend. I will investigate this later, closing this PR.

@flupe flupe closed this Dec 26, 2023
@andreasabel
Copy link
Member

@flupe Maybe this PR fixes your problem in agda2hs:

@flupe
Copy link
Contributor Author

flupe commented Feb 27, 2024

Thank you for the heads up!

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.

None yet

2 participants