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

[declare] Consolidate Proof Using handling. #18890

Merged
merged 1 commit into from
May 8, 2024

Conversation

ejgallego
Copy link
Member

@ejgallego ejgallego commented Apr 3, 2024

This is an old to-do from the times of declare.ml refactoring;
thanks to Hugo Herbelin for trying first with #18742 , which this PR
could replace.

We adopt the following convention:

  • we remove the per-constant using field from CInfo.t (as in [Subsumed] Towards a single code path for the "using" clause + small improvement of "using Type" for co/fixpoints #18742)

  • interactive commands pass their using attribute down to the proof
    initialization, where the parameter is interpreted properly in declare.ml

  • ̶f̶o̶r̶ ̶n̶o̶w̶ ̶w̶e̶ ̶c̶h̶o̶o̶s̶e̶ ̶t̶o̶ ̶w̶a̶r̶n̶ ̶o̶n̶ ̶p̶l̶a̶c̶e̶s̶ ̶w̶h̶e̶r̶e̶ ̶u̶s̶i̶n̶g̶ ̶a̶t̶t̶r̶i̶b̶u̶t̶e̶s̶ ̶a̶r̶e̶ ̶n̶o̶t̶
    ̶ ̶ ̶u̶s̶e̶d̶ ̶(̶t̶r̶a̶n̶s̶p̶a̶r̶e̶n̶t̶ ̶d̶e̶f̶i̶n̶i̶t̶i̶o̶n̶ ̶n̶o̶t̶ ̶i̶n̶s̶i̶d̶e̶ ̶a̶ ̶s̶e̶c̶t̶i̶o̶n̶)̶

We add tests from #18742, written by Hugo.

cc: @herbelin

Overlays:

@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 3, 2024
@ejgallego ejgallego added this to TODO in Proof Handling via automation Apr 3, 2024
@herbelin
Copy link
Member

herbelin commented Apr 4, 2024

non-interactive commands ignore the using attribute (that could be resurrected maybe if we add an "Opaque" form)

add warning when attributes are not supported

Both the design choice of rejecting (as in the PR once the corresponding XXX are filled) or accepting (as in #13903) are ok to me. If it is e.g. easier for UI to reject, let's go this way.

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.

Good to me. There is the pending design decision of rejecting or accepting using on non-interative declarations.

vernac/declare.ml Outdated Show resolved Hide resolved
@ejgallego ejgallego marked this pull request as ready for review April 11, 2024 14:00
@ejgallego ejgallego requested a review from a team as a code owner April 11, 2024 14:00
@ejgallego
Copy link
Member Author

Hi @herbelin , I tried to fix all the problems, this should be good to go now, modulo CI / unintended bugs.

[I was careful to review all the places that matter, and to preserve the behavior, but still there is margin for errors]

@ejgallego ejgallego added part: vernac High level command interpretation. kind: cleanup Code removal, deprecation, refactorings, etc. kind: user messages Improvement of error messages, new warnings, etc. labels Apr 11, 2024
@ejgallego
Copy link
Member Author

@herbelin actually there are some tests that fail now due to this:

Section T.
Variables (T : Type) (x : T).
#[using="x"] Definition test : unit := tt.
End test.
Check test : forall T, T -> unit.

So indeed maybe we want to restore the use of the attribute in this case, right?

That would be easy to do, I guess we could also warn when the attribute is passed but no sections are opened.

@herbelin
Copy link
Member

So indeed maybe we want to restore the use of the attribute in this case, right?

I don't have a strong opinion, but this kinds of feature for immediate definitions is not a problem for UIs, I would preserve the ability to do it.

vernac/declare.ml Outdated Show resolved Hide resolved
@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 15, 2024
@coqbot-app coqbot-app bot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Apr 25, 2024
@ejgallego
Copy link
Member Author

Hi @herbelin , sorry for the wait, I rebased this PR, and I hope I can do the remaining changes before the weekend.

@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 30, 2024
@ejgallego ejgallego added request: full CI Use this label when you want your next push to trigger a full CI. and removed request: full CI Use this label when you want your next push to trigger a full CI. labels May 2, 2024
@coqbot-app coqbot-app bot removed the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label May 2, 2024
@ejgallego ejgallego added the request: full CI Use this label when you want your next push to trigger a full CI. label May 2, 2024
ejgallego added a commit to ejgallego/coq-elpi that referenced this pull request May 6, 2024
@ejgallego ejgallego added request: full CI Use this label when you want your next push to trigger a full CI. and removed needs: overlay This is breaking external developments we track in CI. labels May 6, 2024
@ejgallego ejgallego added this to the 8.20+rc1 milestone May 6, 2024
@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 May 6, 2024
This is an old to-do from the times of `declare.ml` refactoring;
thanks to Hugo Herbelin for trying first with coq#18742 , which this PR
could replace.

We adopt the following convention:

- we remove the per-constant `using` field from `CInfo.t` (as in coq#18742)

- interactive commands pass their using attribute down to the proof
  initialization, where the parameter is interpreted properly in declare.ml

- for now we choose to warn on places where using attributes are not
  used (transparent definition not inside a section)

We add tests from coq#18742, written by Hugo.

Co-authored-by: Hugo Herbelin <Hugo.Herbelin@inria.fr>
@ejgallego ejgallego added request: full CI Use this label when you want your next push to trigger a full CI. and removed kind: user messages Improvement of error messages, new warnings, etc. labels May 6, 2024
@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 May 6, 2024
@ejgallego
Copy link
Member Author

@herbelin this should be ready, a few comments:

  • I've removed the warning entirely, as it was quite confusing together with the error when using is used outside sections. I suggest improvements on user display to happen in a different PR
  • There are some possible minor improvements still to do in the code, but I think this is a good checkpoint

@herbelin
Copy link
Member

herbelin commented May 6, 2024

  • warning entirely, as it was quite confusing together with the error when using is used outside sections. I suggest improvements on user display to happen in a different PR

  • There are some possible minor improvements still to do in the code, but I think this is a good checkpoint

OK to me. (I just added a comment about solve_obligations but it can be in another PR too).

So, I'm assigning and, unless told differently, I'm going to merge this awaited old to-do when the CI is checked.

@herbelin herbelin self-assigned this May 6, 2024
@ejgallego
Copy link
Member Author

OK to me. (I just added a comment about solve_obligations but it can be in another PR too).

Thanks a lot @herbelin ; I think indeed we need to handle what is going on there, but I suggest I do so in a different PR as to allow depending PRs to move independently.

ejgallego added a commit to ejgallego/coq that referenced this pull request May 6, 2024
@ejgallego ejgallego moved this from TODO to Ready in Proof Handling May 6, 2024
@herbelin
Copy link
Member

herbelin commented May 8, 2024

@coqbot merge now

@coqbot-app coqbot-app bot merged commit a32d367 into coq:master May 8, 2024
6 checks passed
Copy link
Contributor

coqbot-app bot commented May 8, 2024

@herbelin: Please take care of the following overlays:

  • 18890-ejgallego-no_using_cinfo.sh

Proof Handling automation moved this from Ready to Done May 8, 2024
SkySkimmer added a commit to coq-community/vscoq that referenced this pull request May 8, 2024
@ejgallego ejgallego deleted the no_using_cinfo branch May 8, 2024 11:29
ppedrot added a commit to LPCIC/coq-elpi that referenced this pull request May 10, 2024
ejgallego added a commit to ejgallego/coq that referenced this pull request May 13, 2024
Follow-up to coq#18890

Not sure what the effect on this will be, doing as suggested by the PR
discussion.
ejgallego added a commit to ejgallego/coq that referenced this pull request May 13, 2024
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. kind: fix This fixes a bug or incorrect documentation. part: vernac High level command interpretation.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants