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

Moving general-purpose rapply from Programs to Init #17209

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Feb 3, 2023

Who has experience with rapply? Shouldn't it be at some time the default apply (or, at least, unifall would lead to identify apply and rapply, right?)?

There is an alternative definition of rapply in TLC (and Software Foundations) which seems currently broken. Since there is a version of rapply already in Coq, maybe should we just rely on this one??

  • Added changelog (?)
  • Updated documentation.

@herbelin herbelin added kind: cleanup Code removal, deprecation, refactorings, etc. part: tactics labels Feb 3, 2023
@herbelin herbelin added this to the 8.18+rc1 milestone Feb 3, 2023
@herbelin herbelin requested review from a team as code owners February 3, 2023 22:15
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Feb 3, 2023
@andres-erbsen
Copy link
Contributor

I believe rapply and apply also differ in elaboration order and handling of /\ in hypotheses (and maybe lemmas). @JasonGross probably knows

@andres-erbsen andres-erbsen self-assigned this Feb 4, 2023
Copy link
Member

@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

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

I'm in favor.

I agree that the implementation of apply should be moved to the refine engine (ideally while keeping the current behavior of decomposing single-constructor inductives). However, the distinction between apply and eapply should be kept, and the move should be done in OCaml, not Ltac, I think.

@herbelin
Copy link
Member Author

herbelin commented Feb 4, 2023

I realize that Program's rapply tries maximal application first, so that it may encounters limits of unification and fail on non atomic goals with flexible head variables, as in:

Require Import Program.
Axiom A : forall R:nat -> Prop,  forall x, R x.
Goal forall R:nat -> Prop,  forall x, R x.
Fail rapply A. (* Unable to unify "?Goal ?Goal0" with "forall (R : nat -> Prop) (x : nat), R x" *)

I guess that the motivation to start with maximal application is efficiency, so I don't know what to think.

This can be addressed if moved to OCaml though.

@Alizter
Copy link
Contributor

Alizter commented Feb 4, 2023

I would prefer an OCaml implementation for rapply since we are paying attention to it. I'm also of the non-blocking opinion that the prelude should be reduced not increased. i.e. If a tactic is considered built-in, then it should be built-in.

@herbelin
Copy link
Member Author

herbelin commented Feb 4, 2023

I would prefer an OCaml implementation for rapply

I made an experimental branch at https://github.com/herbelin/github-coq/pull/new/master+native-rapply so that we can discuss what to do with it, based on concrete code.

It provides rapply and erapply and supports destructing single-constructor. The rapply in and erapply in are still to be done though.

@andres-erbsen andres-erbsen removed their assignment Feb 4, 2023
@gares
Copy link
Member

gares commented Jun 30, 2023

@coqbot run full CI

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jun 30, 2023
@gares gares modified the milestones: 8.18+rc1, 8.19+rc1 Jul 5, 2023
@SkySkimmer SkySkimmer removed this from the 8.19+rc1 milestone Nov 22, 2023
@SkySkimmer
Copy link
Contributor

@coq/stdlib-maintainers this has been stalled for a while, please take a decision (eg merge / close / say that something specific needs to be done before merge)

If you merge before the branch you can put back the milestone.

@SkySkimmer
Copy link
Contributor

ping @coq/stdlib-maintainers

@andres-erbsen
Copy link
Contributor

andres-erbsen commented Feb 27, 2024

I am in favor of merging one implementation or the other. The Ltac one looks good to me despite limitations; if we want the OCaml one somebody else should review in detail. I don't have a particular preference for it being in prelude, so if those opposing that placement could offer an alternative that might be better indeed.

Copy link
Member

@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

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

I am a fan of moving this to Init. I think probably an OCaml variant might be better, but I'm happy to have this merged and defer the decision on the OCaml implementation to later.

@andres-erbsen
Copy link
Contributor

Hearing no suggested changes, I intend to merge this PR next time I look at it.

@andres-erbsen andres-erbsen self-assigned this Mar 20, 2024
@andres-erbsen
Copy link
Contributor

@coqbot run full ci

it's been almost a year

@andres-erbsen andres-erbsen added this to the 8.20+rc1 milestone Apr 9, 2024
@andres-erbsen
Copy link
Contributor

@coqbot merge now

@coqbot-app coqbot-app bot merged commit eccca3a into coq:master Apr 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: cleanup Code removal, deprecation, refactorings, etc. part: tactics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants