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

Ensure that names of constants, inductive types or constructors have distinct names and that binders are named #18394

Closed
wants to merge 5 commits into from

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Dec 9, 2023

The PR does the following:

  • ensure that Coq objects have distinct names, e.g.:

     Definition f (a a:nat) := a.
     About f.
     (* Before: Arguments f (a a)%nat_scope *)
     (* With the PR: Arguments f (a a0)%nat_scope *)

    Possible alternative: failing instead of renaming silently but this would require adaptations (see note below).

  • ensure that binders are named:

    Definition g (_:nat) (P:nat -> Prop) (x : P ltac:(auto)) := x.
    About g.
    (* Before: Arguments g _%nat_scope P%function_scope x *)
    (* With the PR: Arguments g n%nat_scope P%function_scope x *)

    Note: The alternative to force naming user-side would require several changes in the CI.

The last point will be useful for #14597 which requires named binders but for which CI showed that this is sometimes not the case.

See also issue #6786 about non-distinct names in inductive types.

Depends on #18393.

  • Added / updated test-suite
  • Added changelog.

Note: Here are typical cases which indirectly generate definitions with several times the same name:

  • Instantiated definitions
    Definition d1 {y} x (h : x = y :>nat) := h. 
    Definition d2 x := d1 (y:=x).
    About d2.  (* With the PR: "Arguments d2 (x x0)%nat_scope h"; before "(x x)%nat_scope h" *)
  • Auxiliary definitions
    Definition idnat (x:nat) := x.
    Definition f (x:nat) := idnat.
    About f. (* With the PR: "Arguments f (x x0)%nat_scope"; before "(x x)%nat_scope" *)

Related issue: avoid the time spent at renaming the types of objects in Typing.type_of_constant & cie.

@herbelin herbelin added needs: merge of dependency This PR depends on another PR being merged first. part: implicit arguments The implicit arguments mechanism, generalizable variables, etc. labels Dec 9, 2023
@herbelin herbelin added this to the 8.20+rc1 milestone Dec 9, 2023
@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 Dec 9, 2023
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Dec 9, 2023
@herbelin herbelin force-pushed the master+distinct-argument-names branch from 5168ed4 to f54d0d0 Compare December 10, 2023 08:21
@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 Dec 10, 2023
@herbelin herbelin marked this pull request as ready for review December 10, 2023 09:58
@herbelin herbelin requested review from a team as code owners December 10, 2023 09:58
@ppedrot
Copy link
Member

ppedrot commented Dec 15, 2023

I think that autogenerated names in general are really bad. It's extremely annoying for tactics, but here it's affecting vernacular definitions. The fix goes in the wrong direction for #14597. If one wants to name bound variables, it's the responsibility of the user to name the corresponding definition correctly.

@herbelin
Copy link
Member Author

it's the responsibility of the user to name the corresponding definition correctly.

Yes.

It might be quite a lot of overlays though. We could make an evaluation of the number of changes to do and if too much have a warning as compromise? (Or ask upstreams to do the changes?)

@ppedrot
Copy link
Member

ppedrot commented Dec 15, 2023

I think a warning is a good idea, regardless of the chosen solution.

@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 Mar 22, 2024
Copy link
Contributor

coqbot-app bot commented Apr 22, 2024

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 Apr 22, 2024
@herbelin herbelin force-pushed the master+distinct-argument-names branch from f54d0d0 to f9e405b Compare April 30, 2024 07:05
@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. stale This PR will be closed unless it is rebased. labels Apr 30, 2024
Copy link
Member

@ppedrot ppedrot left a comment

Choose a reason for hiding this comment

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

This PR is doing the wrong change. Never ever should we rename binders explicitly given by the user using an freshening algorithm, that would drive people crazy and make the code very much not modular (e.g. when metaprogramming). Similarly, anonymous binders should similarly remain anonymous. Instead, Coq should warn when there are two variables names that conflict and let the user fix the issue, not try to be clever.

@ppedrot
Copy link
Member

ppedrot commented May 23, 2024

This is going the wrong way and there is no point in keeping zombie PRs, we should close.

@ppedrot ppedrot closed this May 23, 2024
@coqbot-app coqbot-app bot removed this from the 8.20+rc1 milestone May 23, 2024
@herbelin
Copy link
Member Author

"zombie PR" is your terminology that I don't endorse! Personally, I would name it an experience which suggests that another approach should be done (in particular in relation to #14597). But that will require some concentration on the topic that I don't have now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. needs: merge of dependency This PR depends on another PR being merged first. part: implicit arguments The implicit arguments mechanism, generalizable variables, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants