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

Add head reduction flags (lazy head beta etc) #17503

Merged
merged 4 commits into from Oct 25, 2023
Merged

Conversation

SkySkimmer
Copy link
Contributor

@SkySkimmer SkySkimmer commented Apr 18, 2023

NB: the cbv code doesn't seem to have a weak mode (I guess it would
just mean not reducing under binders).

Overlays:

@SkySkimmer SkySkimmer requested review from a team as code owners April 18, 2023 14:02
@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 Apr 18, 2023
@SkySkimmer
Copy link
Contributor Author

@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 Apr 18, 2023
Copy link
Member

@jfehrle jfehrle left a comment

Choose a reason for hiding this comment

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

Some suggestions. Would be good to add the index entries.

doc/tools/docgram/orderedGrammar Outdated Show resolved Hide resolved
doc/sphinx/proofs/writing-proofs/equality.rst Outdated Show resolved Hide resolved
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.

This is great, thank you!

I would also be quite interested in modifiers that meant "don't go underneath lambdas". Should I open a separate issue for that?

@@ -11,3 +11,6 @@ Eval simpl in (fix plus (n m : nat) {struct n} : nat :=

Eval hnf in match (plus (S n) O) with S n => n | _ => O end.

Eval weak simpl in 2 + 2.
Copy link
Member

Choose a reason for hiding this comment

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

Please also test 2 + (2 + 2) (behavior on terms with non-normal arguments) and hold (fun x => 1 + x)forAxiom hold : forall {T}, T.` (when the arguments to the whnf have lambdas)

@SkySkimmer
Copy link
Contributor Author

If we want to add modifiers we should probably not go with this syntax and instead put the modifiers among the flags (beta etc)
The situation for simpl with flags is kinda strange btw, it ignores the flags unless simpliscbn AFAICT

@JasonGross
Copy link
Member

If we want to add modifiers we should probably not go with this syntax and instead put the modifiers among the flags (beta etc)

I'd be fine with weak meaning the thing in this PR for lazy, cbn, and simpl, and meaning "don't go under binders" for cbv, vm_compute and native_compute.

@jfehrle
Copy link
Member

jfehrle commented Apr 19, 2023

I'd be fine with weak meaning the thing in this PR for lazy, cbn, and simpl, and meaning "don't go under binders" for cbv, vm_compute and native_compute.

Only if we document the behavior/differences!

@SkySkimmer
Copy link
Contributor Author

I'd be fine with weak meaning the thing in this PR for lazy, cbn, and simpl, and meaning "don't go under binders" for cbv, vm_compute and native_compute.

But "go in subterms but not under binders" is also meaningful for cbn & co.

@SkySkimmer SkySkimmer added needs: progress Work in progress: awaiting action from the author. needs: test-suite update Test case should be added to / updated in the test-suite. labels Apr 19, 2023
@JasonGross
Copy link
Member

I'd be fine with weak meaning the thing in this PR for lazy, cbn, and simpl, and meaning "don't go under binders" for cbv, vm_compute and native_compute.

But "go in subterms but not under binders" is also meaningful for cbn & co.

That's true. But it's also the natural meaning of "weak" for cbv ("stop once you're guaranteed that there can be no more redexs in the head position"). But in any case, I care more about having the functionality at all than about what the syntax is. And I support merging this PR with the current syntax. (Though I do think it'd be cleaner overall to have two flags instead of prefixes, one for "stop when there are guaranteed to be no redexs in the head position", and another for "do not traverse into the bodies of lambdas/Pis".)

@ppedrot
Copy link
Member

ppedrot commented Apr 24, 2023

Related to the flag discussion, I'm not a huge fan of adding non-identifiers in the list of reductions, since this easily causes mayhem with parsing (think about future parsing extensions). If not a flag, this should rather be called weak_cbn or something similar as a single ident.

@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Apr 25, 2023
@coqbot-app coqbot-app bot added needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. and removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. labels Apr 27, 2023
@SkySkimmer SkySkimmer changed the title Add weak reductions: weak lazy weak cbn weak simpl Add head reduction flags (lazy head beta etc) Apr 27, 2023
@SkySkimmer
Copy link
Contributor Author

weak was the wrong term, it should have been head. Now changed.
Also changed to use the regular postfix flag system instead of a special prefix.

@SkySkimmer SkySkimmer removed needs: progress Work in progress: awaiting action from the author. needs: test-suite update Test case should be added to / updated in the test-suite. labels Apr 27, 2023
@herbelin
Copy link
Member

OK if you insist. Then how to proceed? You fix the ll of contextualize and update the serapi orverlay, then I merge?

Copy link
Member

@herbelin herbelin left a comment

Choose a reason for hiding this comment

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

OK to me.

Note that the following will later have to be done:

Regarding terminology: head looks good because more intuitive than weak-head (e.g. using the word whd would have looked a bit jargon).

@herbelin
Copy link
Member

herbelin commented Oct 22, 2023

Also: will it be "acceptable practice" that the weak cbv PR modifies the change log of this PR?

Added: I guess so.

@herbelin herbelin self-assigned this Oct 22, 2023
@SkySkimmer SkySkimmer added the request: full CI Use this label when you want your next push to trigger a full CI. label Oct 24, 2023
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Oct 24, 2023
@SkySkimmer
Copy link
Contributor Author

@coqbot ci minimize ci-analysis

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Oct 24, 2023

I am now running minimization at commit 6bbecf5 on requested target ci-analysis. I'll come back to you with the results once it's done.

@SkySkimmer SkySkimmer added the needs: fixing The proposed code change is broken. label Oct 24, 2023
@coqbot-app

This comment was marked as outdated.

@coqbot-app

This comment was marked as outdated.

@JasonGross
Copy link
Member

I expect the minimization will not go anywhere without help; the file takes 10s to run and is 3700loc...

@coqbot-app

This comment was marked as outdated.

@coqbot-app

This comment was marked as outdated.

@SkySkimmer
Copy link
Contributor Author

Looks like it was a temporary problem anyway, probably some incompatible mathcomp change.

@SkySkimmer SkySkimmer removed the needs: fixing The proposed code change is broken. label Oct 25, 2023
@SkySkimmer
Copy link
Contributor Author

@herbelin this should be ready to merge

@herbelin
Copy link
Member

@coqbot: merge now (finally!)

@coqbot-app coqbot-app bot merged commit f2e9235 into coq:master Oct 25, 2023
9 of 11 checks passed
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Oct 25, 2023

@herbelin: Please take care of the following overlays:

  • 17503-SkySkimmer-redexpr.sh

@herbelin herbelin added the part: reduction strategies The Strategy command for defining reduction straegies. label Oct 25, 2023
ejgallego added a commit to ejgallego/coq-serapi that referenced this pull request Oct 25, 2023
Adapt to coq/coq#17503 (raw/glob_red_expr moved)
@SkySkimmer SkySkimmer deleted the redexpr branch October 26, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. part: reduction strategies The Strategy command for defining reduction straegies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet