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

fix missing type-level equivalence in contra #1183

Merged
merged 3 commits into from
Mar 28, 2024

Conversation

Tragicus
Copy link
Collaborator

@Tragicus Tragicus commented Mar 8, 2024

Motivation for this change

The type-level equivalence was missing from the inference algorithm for contra.

Checklist
  • added corresponding entries in CHANGELOG_UNRELEASED.md
  • added corresponding documentation in the headers

Reference: How to document

Reminder to reviewers

classical/contra.v Outdated Show resolved Hide resolved
@affeldt-aist affeldt-aist added this to the 1.1.0 milestone Mar 14, 2024
@affeldt-aist affeldt-aist self-requested a review March 24, 2024 13:25
Copy link
Collaborator

@proux01 proux01 left a comment

Choose a reason for hiding this comment

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

Could the commit / PR titles and descriptions be more precise? It reads "fix contra" but then it seems to be mostly adding a type level equivalence, so I'm a bit lost.

Comment on lines 84 to 85
Definition equivT_LR {S T} '(EquivT S_T _) : S -> T := S_T.
Definition equivT_RL {S T} '(EquivT _ T_S) : T -> S := T_S.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The usual style in mathcomp rather seems to be something like

Suggested change
Definition equivT_LR {S T} '(EquivT S_T _) : S -> T := S_T.
Definition equivT_RL {S T} '(EquivT _ T_S) : T -> S := T_S.
Definition equivT_LR {S T} (eq : equivT S T) : S -> T := let: EquivT S_T _ := eq in S_T.
Definition equivT_RL {S T} (eq : equivT S T) : T -> S := let: EquivT _ T_S := eq in T_S.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I also change l.75?

(******************************************************************************)

Variant equivT S T := EquivT of S -> T & T -> S.
Notation "S <--> T" := (equivT S T) (at level 95, no associativity).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this notation is used only on the three lines below (it is not exported), I'd rather remove it to avoid modifying the parser just for that.

@affeldt-aist
Copy link
Member

the following line
"The ForallI implementation has to work around several Coq 8.12 issues"
suggest that some workarounds could be removed from the code

@Tragicus
Copy link
Collaborator Author

I think I remember asking [at]ggonthier, who said that the issues have not been fixed yet.

@Tragicus Tragicus changed the title fix contra fix missing type-equivalence in contra Mar 25, 2024
@Tragicus Tragicus changed the title fix missing type-equivalence in contra fix missing type-level equivalence in contra Mar 25, 2024
@Tragicus
Copy link
Collaborator Author

Could the commit / PR titles and descriptions be more precise? It reads "fix contra" but then it seems to be mostly adding a type level equivalence, so I'm a bit lost.

I changed the PR title and description, I will do the commit messages when I squash. IS that better?

@proux01
Copy link
Collaborator

proux01 commented Mar 26, 2024

Not really, this still doesn't tell what this type level equivalence is useful for, what problem it fixes (why it was "missing in the inference algorithm", could you give an example) and why it is defined here.

@Tragicus
Copy link
Collaborator Author

Tragicus commented Mar 27, 2024

It was part of the code from [at]ggonthier I ported to define the contra tactic and I pruned a little too much. I am afraid I can not tell you why it is needed since I do not know. As for an example, #1173 does not compile without it (if you put its commit on top of master, wochoice.v fails at l.167).

@affeldt-aist affeldt-aist self-requested a review March 27, 2024 10:07
@proux01
Copy link
Collaborator

proux01 commented Mar 28, 2024

Ok, thanks for the explanation! It would probably have made more sense to put it in #1173 then, but let's merge.

@proux01 proux01 mentioned this pull request Mar 28, 2024
2 tasks
Copy link
Member

@affeldt-aist affeldt-aist left a comment

Choose a reason for hiding this comment

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

We merge it because otherwise contra will not work as expected but we'll issue about the lack of examples in the doc.

@affeldt-aist affeldt-aist merged commit 7f31359 into math-comp:master Mar 28, 2024
0 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants