Skip to content

Warn on Qed Let declaration - #17205

Merged
coqbot-app[bot] merged 1 commit into
rocq-prover:masterfrom
SkySkimmer:warn-qeq-let
Apr 3, 2023
Merged

Warn on Qed Let declaration#17205
coqbot-app[bot] merged 1 commit into
rocq-prover:masterfrom
SkySkimmer:warn-qeq-let

Conversation

@SkySkimmer

Copy link
Copy Markdown
Contributor

cf #10459

@SkySkimmer
SkySkimmer requested review from a team as code owners February 3, 2023 13:58
@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
silene
silene previously requested changes Feb 3, 2023

@silene silene left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is bad. Turning the Let Qed tests into Let Defined ones makes it impossible to interactively navigate the files. For the sake of your sanity, I won't even copy-paste what Coqide looks like at that point.

It is better to disable the warning for these files and keep Let Qed. Hopefully, one day we have an actual command or attribute that make it possible to discard definitions from sections.

@SkySkimmer

Copy link
Copy Markdown
Contributor Author

Hopefully, one day we have an actual command or attribute that make it possible to discard definitions from sections.

I don't see a way to have it make sense.

@silene

silene commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

I don't see a way to have it make sense.

Why? We already have a Reset command in Coq. Having the section's End do a reset to the start of the section does not seem so farfetched to me.

@ejgallego

ejgallego commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

Having the section's End do a reset to the start of the section does not seem so farfetched to me.

Actually this what the actual implementation does, then replays some stuff, IIRC, no?

@SkySkimmer

Copy link
Copy Markdown
Contributor Author

This is bad. Turning the Let Qed tests into Let Defined ones makes it impossible to interactively navigate the files. For the sake of your sanity, I won't even copy-paste what Coqide looks like at that point.

We could do something like the current Qed hack for Opaque variables, or only omit the bodies when printing the goal. It makes more sense for Opaque as that's not supposed to be as strong as Qed.

Why? We already have a Reset command in Coq. Having the section's End do a reset to the start of the section does not
seem so farfetched to me.

How is that related? I don't think I understand what you mean by "discard definitions from sections".

@silene

silene commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

I mean this:

Section Foo.
  Example foo : True.
  Proof. exact I. Qed.
End Foo.
Reset Foo.
About foo.
(* foo not a defined object. *)

@Alizter

Alizter commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

That's great but what about when the user does:

Section Foo.
  Example foo : True.
  Proof. exact I. Qed.
  Definition bar : True.
  Proof. apply foo. Defined.
End Foo.

What is the definition of bar?

@silene

silene commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

There is no definition of bar. The whole point is to discard a section, e.g., the Section Examples from Qfield.v.

@Alizter

Alizter commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

So what you really want is a way to abort a section?

@silene

silene commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

That is another way to phrase it, yes. (Was "discard" really that strange a concept?)

@SkySkimmer

Copy link
Copy Markdown
Contributor Author

For me it was the phrasing as "discard definitions from sections" instead of "discard sections", the first one sounds like it is about discarding a subset of the section at least in the context of this PR.

@Alizter

Alizter commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

But to let @SkySkimmer make progress on this PR, you are suggesting to implement that feature before this one so that the behavior of the examples in the file are adequate?

@silene

silene commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

Not at all. I am suggesting that the changes to Qfield.v and similar ones should be reverted and replaced by a local disabling of the new warning.

@SkySkimmer SkySkimmer added the request: full CI Use this label when you want your next push to trigger a full CI. label Feb 24, 2023
@coqbot-app coqbot-app Bot removed request: full CI Use this label when you want your next push to trigger a full CI. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. labels Feb 24, 2023
@SkySkimmer

Copy link
Copy Markdown
Contributor Author

Not at all. I am suggesting that the changes to Qfield.v and similar ones should be reverted and replaced by a local disabling of the new warning.

I made a section per example instead, that way they don't interfere with each other.
We should not rely on the LetQed behaviour if at all possible.

@SkySkimmer
SkySkimmer requested a review from silene February 24, 2023 13:12
@SkySkimmer SkySkimmer added the request: full CI Use this label when you want your next push to trigger a full CI. label Feb 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 Feb 24, 2023
@ppedrot

ppedrot commented Mar 22, 2023

Copy link
Copy Markdown
Member

ping @silene about the new state of the PR

@silene

silene commented Mar 22, 2023

Copy link
Copy Markdown
Contributor

I am fine with the way the examples are written. (I suppose that, in the long run, we should stop storing Goal statements into modules, which would solve this mess in a more intuitive way.)

@silene
silene dismissed their stale review March 22, 2023 10:15

No longer relevant.

@SkySkimmer

Copy link
Copy Markdown
Contributor Author

changed doc

@ppedrot ppedrot 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 3, 2023
@ppedrot

ppedrot commented Apr 3, 2023

Copy link
Copy Markdown
Member

@coqbot merge now

@coqbot-app
coqbot-app Bot merged commit da829d8 into rocq-prover:master Apr 3, 2023

@jfehrle jfehrle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few more wording suggestions


.. warn:: @ident is declared opaque but this is not fully respected inside the section and not at all outside the section.

Terminating the proof for a :cmd:`Let` with :cmd:`Qed` with `Let` is not recommended.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a :cmd:`Let` with :cmd:`Qed` with `Let` -> a :cmd:`Let` with :cmd:`Qed`

Comment on lines +73 to +74
the body when checking, and it will be treated the same as a
:cmd:`Defined` `Let` when the section is closed.

@jfehrle jfehrle Apr 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

    the body when checking, and it will be treated the same as a
   :cmd:`Defined` `Let` when the section is closed.

->

   the body when checking.  When the section is closed,
   it will be treated as if :cmd:`Defined` had been specified.

(the same as a :cmd:`Defined` `Let` seemed awkward.)

@SkySkimmer
SkySkimmer deleted the warn-qeq-let branch April 4, 2023 11:19
proux01 added a commit to math-comp/math-comp that referenced this pull request Apr 12, 2023
proux01 added a commit to math-comp/math-comp that referenced this pull request Apr 24, 2023
Introduced in rocq-prover/rocq#17205
Using `Local Lemma` is not satisfying (exposes the lemma and require
manual introduction of hints). Replacing `Qed` with `Defined` doesn't
seem to incur a noticable slowdown but clutters the proof context with
bodies of the lemmas. So silencing the warning for now until Coq
offers a way to automatically do the `clearbody` at beginning of each
proof.
@proux01

proux01 commented Apr 24, 2023

Copy link
Copy Markdown
Contributor

User report: the feature is indeed used in MathComp. Replacing Qed with Defined there incurs an acceptable slowdown but clutters the environment of subsequent proofs and doing a bunch of clearbody manually there is not very practical. So we currently resorted to silence the warning.

What about having Qed in Let trigger an abstract?

Otherwise, would it make sense to add an attribute clearbody to Let that would automatically perform the clearbody in every subsequent proof in the section?

@SkySkimmer

Copy link
Copy Markdown
Contributor Author

What about having Qed in Let trigger an abstract?

Wouldn't that be the same as using Lemma with Qed? If that's what you want why not use Lemma?

Otherwise, would it make sense to add an attribute clearbody to Let that would automatically perform the clearbody in every subsequent proof in the section?

That would be OK IMO
I guess it would just use the current fake-Qed mechanism under the hood.

@ppedrot

ppedrot commented Apr 24, 2023

Copy link
Copy Markdown
Member

While we're at it, I think it would be great to have attributes for Let declarations that allow to control their inlining at section closure. Such a mechanism seems to have a cross-section with the topic discussed here.

@proux01

proux01 commented Apr 24, 2023

Copy link
Copy Markdown
Contributor

What about having Qed in Let trigger an abstract?

Wouldn't that be the same as using Lemma with Qed? If that's what you want why not use Lemma?

No, Let puts the lemma in the section context (so that it can be picked up by assumption for instance), that's what it is used for in MathComp. (We did try to use Lemma but that required adding a lot of Hint)

proux01 added a commit to math-comp/math-comp that referenced this pull request May 12, 2023
Introduced in rocq-prover/rocq#17205
Using `Local Lemma` is not satisfying (exposes the lemma and require
manual introduction of hints). Replacing `Qed` with `Defined` doesn't
seem to incur a noticable slowdown but clutters the proof context with
bodies of the lemmas. So silencing the warning for now until Coq
offers a way to automatically do the `clearbody` at beginning of each
proof.
proux01 added a commit to proux01/mathcomp that referenced this pull request May 12, 2023
Introduced in rocq-prover/rocq#17205
Using `Local Lemma` is not satisfying (exposes the lemma and require
manual introduction of hints). Replacing `Qed` with `Defined` doesn't
seem to incur a noticable slowdown but clutters the proof context with
bodies of the lemmas. So silencing the warning for now until Coq
offers a way to automatically do the `clearbody` at beginning of each
proof.
@herbelin

Copy link
Copy Markdown
Member

Maybe an idle talk, but would it make sense to add an opaque flag also to local definitions, that is to have the additional typing rule:

Γ, x : A ⊢ t : B       Γ ⊢ u : A
----------------------------------------
Γ ⊢ let opaque x := u : A in t : B[x:=u]

which, in other words, means to internalize the usual substitution as an explicit constructor of the syntax?

@SkySkimmer

Copy link
Copy Markdown
Contributor Author

How would it reduce?

@herbelin

Copy link
Copy Markdown
Member

I guess we can consider that it reduces to t[x:=u] (that is that the opacity is only about not adding the definitional equality x ≣ u when typing t).

Or we may also imagine that it is blocked (as it would be for a global definition), providing some proof-irrelevance, that is identifying let opaque x := u in t and let opaque x := v in t (it looks a bit strange though).

@herbelin

Copy link
Copy Markdown
Member

BTW, what is the attribute to make opaque a Definition (if there is one)? If there isn't, could #[clearbody] work also for Definition?

@SkySkimmer

Copy link
Copy Markdown
Contributor Author

I guess we can consider that it reduces to t[x:=u] (that is that the opacity is only about not adding the definitional equality x ≣ u when typing t).

But then isn't there no difference between let opaque x := v in w and (fun x => w) v?

BTW, what is the attribute to make opaque a Definition (if there is one)? If there isn't, could #[clearbody] work also for Definition?

The point of clearbody is that it is not opaque, it is some adhoc thing for section letins.

@herbelin

Copy link
Copy Markdown
Member

But then isn't there no difference between let opaque x := v in w and (fun x => w) v?

Yes, no difference except that one is a beta-redex and the other an explicit substitution. With an explicit substitution, one can directly simulate a context Section S. Let x : t. exact u. Qed.. With a fun, it would be more far-stretched to do so.

@herbelin

Copy link
Copy Markdown
Member

Is this the "ad hoc" thing you are mentioning?

Section S.
#[clearbody]
Let a:=0.
Lemma e : a = 0. Fail exact eq_refl. Abort.
Definition e : a = 0 := eq_refl. (* works *)

@herbelin

Copy link
Copy Markdown
Member

I still have my question: is there an attribute to tell that Definition a := t. should be considered opaque? And if not, couldn't we provide one?

@SkySkimmer

Copy link
Copy Markdown
Contributor Author

Isn't that basically rocq-prover/rfcs#42 ?

@herbelin

Copy link
Copy Markdown
Member

Wouldn't it be more consistent to grant variable opacity for all declarations and not only for interactively proved declarations? Alternatively. would there be compatibility issues at making a Qed-ed or clearbody-ed local definition opaque for all the rest of the section?

@herbelin

Copy link
Copy Markdown
Member

Isn't that basically rocq-prover/rfcs#42 ?

If only a decision is needed to at least already implement sealed/unsealed from rocq-prover/rfcs#42, I propose we take it!

@herbelin

herbelin commented Nov 3, 2023

Copy link
Copy Markdown
Member

For the record, that is not a problem, but clearbody fails on Context

Section S.
#[clearbody] Context (b:=0).
(* This command does not support this attribute: clearbody. *)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: user messages Error messages, warnings, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants