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

Remove uses of "Proof with" from library #12065

Closed
wants to merge 1 commit into from

Conversation

jfehrle
Copy link
Member

@jfehrle jfehrle commented Apr 10, 2020

NEW DESCRIPTION:
Remove uses of "Proof with" from the library because it's not a great feature (see #12059) and I think not a best practice. The library should try to follow best practices since users may emulate thing they see in there.

ORIGINAL:
Experiment with removing "Proof with" and see what breaks

@Zimmi48
Copy link
Member

Zimmi48 commented Apr 10, 2020

It's amazing how much it breaks! Even Software Foundations, wow!

@jfehrle
Copy link
Member Author

jfehrle commented Apr 10, 2020

Funny, I was pleased that about half the libraries and plugins still compiled.

You think it's worth committing the .v changes now?

@Zimmi48
Copy link
Member

Zimmi48 commented Apr 11, 2020

Well, it's pretty rare that we commit changes with this level of breakage. This would have to be discussed with the users of the feature, but given that it is not a super problematic feature, I wouldn't push for this too hard.

Regarding the clean-up of the standard library, yes it would be interesting to commit this but it would be better to clean the diff as much as possible, i.e., remove the trailing tactics every time they do nothing.

@jfehrle jfehrle changed the title Experiment: Remove Proof with Remove uses of "Proof with" from library Apr 11, 2020
@jfehrle
Copy link
Member Author

jfehrle commented Apr 11, 2020

yes it would be interesting to commit this but it would be better to clean the diff as much as possible, i.e., remove the trailing tactics every time they do nothing.

I already did that for the 2 files in theories/Reals where it looked like it was overused. In the other files it was used far less and didn't look overused. Verifying whether each one is necessary is too tedious, not something I plan to do.

@jfehrle
Copy link
Member Author

jfehrle commented Apr 11, 2020

We could add language to the doc for "Print with" recommending the use of alternative constructs. Could be in this PR or elsewhere.

@jfehrle jfehrle marked this pull request as ready for review April 11, 2020 20:36
@jfehrle jfehrle requested review from a team as code owners April 11, 2020 20:36
@anton-trunov
Copy link
Member

If we stick to changing only Qeded proofs it should not break things, right?

@jfehrle
Copy link
Member Author

jfehrle commented Apr 12, 2020

If we stick to changing only Qeded proofs it should not break things, right?

@Zimmi48's comments about breakage refer to an initial experiment in which I made Proof with unavailable. All the jobs in GitLab are green now.

@anton-trunov
Copy link
Member

Thanks. I just remember several years ago Software Foundation had a bunch of Proof with, so I naturally thought CI was not green because of Defined.

@ejgallego
Copy link
Member

Thanks. I just remember several years ago Software Foundation had a bunch of Proof with, so I naturally thought CI was not green because of Defined.

Indeed the use of Proof with in SF explains why is so pervasive among other developments.

@Zimmi48
Copy link
Member

Zimmi48 commented Apr 13, 2020

Verifying whether each one is necessary is too tedious, not something I plan to do.

Yeah, I was wondering if some automated strategy (based on proof diffs) could help here, but I have nothing concrete to propose.

@Zimmi48
Copy link
Member

Zimmi48 commented Apr 13, 2020

We could add language to the doc for "Print with" recommending the use of alternative constructs. Could be in this PR or elsewhere.

Before discouraging the use of the feature, we should ask the opinion of @bcpierce00. It makes little sense to discourage a feature in the reference manual, if in the same time it is presented in the most read tutorial about Coq.

But we can already remove it from the standard library if we want.

theories/FSets/FMapList.v Outdated Show resolved Hide resolved
@Zimmi48 Zimmi48 added the kind: cleanup Code removal, deprecation, refactorings, etc. label Apr 27, 2020
@Zimmi48
Copy link
Member

Zimmi48 commented May 13, 2020

Since I'm still reluctant to introduce a change that puts a lot of ; foo in the code even though most of them might be useless, I thought of a new idea. What about introducing a new warning (off by default) that a user can use to detect useless ..., i.e., Coq would warn if the goal after applying the "Proof with tactic" is in the same state as before. Users could activate this warning and a script could use them to remove all useless ..., then the script could further transform the remaining ... into ; foo.

@bcpierce00
Copy link
Contributor

I like this last option a lot. I'd be happy to get rid of all these Proof...with...'s, but would like not to make the proofs a lot uglier in the process.

@ejgallego
Copy link
Member

I think that in general the idea of ... is a useful one and built-in in other development styles, for example ssr's // which is rebindable and was recently extended.

I have indeed trouble agreeing on the original premise of this PR which qualifies ... as not a great feature and not best practice, as in practice alternatives such as // are essential for some proof styles, and also implemented in advanced ltac setups such as the ones by MIT.

@Zimmi48
Copy link
Member

Zimmi48 commented May 14, 2020

I think that in general the idea of ... is a useful one and built-in in other development styles, for example ssr's // which is rebindable and was recently extended.

I have indeed trouble agreeing on the original premise of this PR which qualifies ... as not a great feature and not best practice, as in practice alternatives such as // are essential for some proof styles, and also implemented in advanced ltac setups such as the ones by MIT.

There's a major difference between // and ... which is that the former does not require built in support in Proof and, more importantly, in terminators.

It would be pretty reasonable to replace this feature with a //. that is locally defined.

@jfehrle
Copy link
Member Author

jfehrle commented May 14, 2020

IIUC // doesn't do anything that can't be done by replacing Proof with <tactic> with Proof. Ltac f := <tactic>. and ... with . f. f is certainly rebindable. Setting aside SSR, we don't use punctuation by itself to represent tactics--those are named with identifiers. It makes sense to clean up some of the odder features of Coq syntax that duplicate the functionality of other, more consistent features--there's no loss of functionality, it's easier to document, fewer constructs to learn and perhaps enables other code cleanup.

@Zimmi48

What about introducing a new warning (off by default) that a user can use to detect useless ..., i.e., Coq would warn if the goal after applying the "Proof with tactic" is in the same state as before. Users could activate this warning and a script could use them to remove all useless ..., then the script could further transform the remaining ... into ; foo.

Something along these lines would make it painless for users to update their proofs, though instead of printing a warning, coqc could just remember where the ... should be removed. After compilation is complete, that info could be used to update the file, the code for which could be entirely OCaml or a python script started by coqc.

@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 Aug 24, 2020
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jul 26, 2021

The "needs: rebase" label was set more than 30 days ago. If the PR is not rebased in 30 days, it will be automatically closed.

@coqbot-app coqbot-app bot added the stale This PR will be closed unless it is rebased. label Jul 26, 2021
@Zimmi48
Copy link
Member

Zimmi48 commented Jul 26, 2021

I suggest closing this PR until a tool is made available that would help replace ... by ; tac only where it is useful (based on a warning or something else).

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Aug 25, 2021

This PR was not rebased after 30 days despite the warning, it is now closed.

1 similar comment
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Sep 6, 2021

This PR was not rebased after 30 days despite the warning, it is now closed.

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. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. stale This PR will be closed unless it is rebased.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants