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

Replace record directive "eta-equality" by "no-eta-equality;pattern" #2476

Draft
wants to merge 2 commits into
base: experimental
Choose a base branch
from

Conversation

andreasabel
Copy link
Member

@andreasabel andreasabel commented Sep 6, 2024

Replace record directive eta-equality by no-eta-equality; pattern.

Future Agda might not allow unguarded record types with eta-equality in safe mode anymore.
So we switch eta off here, but make the constructor a pattern so that it can still be matched on.

Re: agda/agda#7470

@andreasabel andreasabel marked this pull request as draft September 6, 2024 10:19
@andreasabel andreasabel force-pushed the pattern-instead-of-eta-equality branch from 4d24502 to 8ef8f9a Compare September 6, 2024 20:34
@andreasabel andreasabel changed the base branch from master to merge-v2.1.1 September 7, 2024 06:49
@andreasabel andreasabel force-pushed the pattern-instead-of-eta-equality branch from 8ef8f9a to 1a5ce5f Compare September 7, 2024 06:56
@andreasabel andreasabel added the upstream Changes induced by Agda upstream label Sep 7, 2024
@andreasabel
Copy link
Member Author

These two commits passed CI when based onto master.
Now I rebased them onto merge-v2.1.1 so I can use them with Agda master (2.8.0).

@andreasabel andreasabel closed this Sep 7, 2024
@andreasabel andreasabel reopened this Sep 7, 2024
@andreasabel
Copy link
Member Author

Unfortunately, CI does not run anymore since the merge base is not master anymore.

I will have to change it back to master once #2473 has landed:

Future Agda versions might no longer support eta for unguarded records in `--safe` mode.

Dropping eta-equality for constructor _⊐_ only requires changing the order of clauses in ⊞-inj.

Re: agda/agda#7470
Future Agda might not allow unguarded record types with eta-equality
in `--safe` mode anymore.
Re: agda/agda#7470

Switching of eta for `rec` is slightly inconvenient, as was observed
already in this issue: agda/agda#840

Allowing it would need improvements to the Agda positivity checker,
so that it can recognize `Record` as guarded record that can safely
support eta.
@andreasabel andreasabel force-pushed the pattern-instead-of-eta-equality branch from 1a5ce5f to 8caaeae Compare September 8, 2024 09:31
@andreasabel andreasabel changed the base branch from merge-v2.1.1 to experimental September 8, 2024 09:32
@andreasabel andreasabel closed this Sep 8, 2024
@andreasabel andreasabel reopened this Sep 8, 2024
@andreasabel
Copy link
Member Author

I reset experimental to current master and rebased this PR onto experimental. So we are back in the main stream now.

@MatthewDaggitt
Copy link
Contributor

Great, thank you!

@MatthewDaggitt MatthewDaggitt added this to the Agda v2.8.0 milestone Sep 8, 2024
@MatthewDaggitt
Copy link
Contributor

As this is a breaking change, is it possible to wait until we know for sure that Agda will also make the breaking change?

@andreasabel
Copy link
Member Author

As this is a breaking change, is it possible to wait until we know for sure that Agda will also make the breaking change?

Yes, let's wait until the Agda side is settled.

Commit 91e2c01 seems benign as probably the affected module is used mostly internally in the ring solver tactic.

Commit 8caaeae is more invasive and might have larger repercussions on the user of the Record structure. A pretext is here:

@jamesmckinna
Copy link
Contributor

Re: being a breaking change, the common case I worry most about will be the ability, currently exploited systematically, to never mention, either as a pattern on the LHS, nor as a term on the RHS, of definitions, the value tt : ⊤ of the unit type...
... but I suppose, at least IIUC, that pattern will ensure that the LHS uses will be OK under this change, while no-eta will have a lot of trivial-but-annoying knock-on consequences on the RHS? Is this analysis/'understanding' broadly correct?

@MatthewDaggitt MatthewDaggitt added the status: blocked-by-issue Progress on this issue or PR is blocked by another issue. label Oct 7, 2024
@jamesmckinna
Copy link
Contributor

Further to my comment above: does the change mean (morally) that (inductive) records are now a parameter-only specialisation of (the now-more-general?) one-constructor data declarations (which have pattern, as it were, automatically, for their single constructor, and moreover no eta-expansion/contraction allowed)? or will some instances of eta still be permitted?
(Apologies if I haven't understood the internals of all this, but I remember at AIM in Delft being recommended to use records mutually with inductive data for a certain representation, without being entirely clear about the pragmatics/trade-offs involved, and these now seem to be changing?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking status: blocked-by-issue Progress on this issue or PR is blocked by another issue. upstream Changes induced by Agda upstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants