Skip to content

Conversation

@pi8027
Copy link
Member

@pi8027 pi8027 commented Dec 5, 2023

Motivation for this change

This PR makes the hierarchy-builder branch (#951) compatible with math-comp/math-comp#1131. The issue stems from the fact that the parsing of (_ \; _) _ uses the \; notation defined in the classical_set_scope.

We probably should close the classical_set_scope and localize its use, e.g.,

Arguments measurable {d}%measure_display_scope {s} _%classical_set_scope.
Things done/to do
  • [ ] added corresponding entries in CHANGELOG_UNRELEASED.md
  • [ ] added corresponding documentation in the headers
Compatibility with MathComp 2.0
  • [ ] I added the label TODO: HB port to make sure someone ports this PR to
    the hierarchy-builder branch or I already opened an issue or PR (please cross reference).
Automatic note to reviewers

Read this Checklist and put a milestone if possible.

@pi8027 pi8027 changed the title Adapt to math-comp/mathcomp#1131 Adapt to math-comp/math-comp#1131 Dec 5, 2023
@pi8027 pi8027 force-pushed the hierarchy-builder-function_scope branch from fe937d0 to 590c973 Compare December 5, 2023 14:34
@pi8027 pi8027 requested a review from affeldt-aist December 5, 2023 14:40
@proux01
Copy link
Collaborator

proux01 commented Dec 5, 2023

Why not putting the _ \; _ notation in function_scope?

@pi8027
Copy link
Member Author

pi8027 commented Dec 6, 2023

@proux01 That's because that does not fix the issue. I thought function_scope should be locally opened for the left-hand side of a function application, but it is not?

@pi8027
Copy link
Member Author

pi8027 commented Dec 6, 2023

Reported: rocq-prover/rocq#18372

@proux01
Copy link
Collaborator

proux01 commented Dec 6, 2023

Apparently not indeed. In master / future 8.19, an explicit cast t : _ -> _ opens function_scope in t but t x does not. Maybe something that could be tried in Coq.

@pi8027
Copy link
Member Author

pi8027 commented Dec 6, 2023

CI green.

@proux01 proux01 merged commit 480ca05 into hierarchy-builder Dec 6, 2023
@proux01 proux01 deleted the hierarchy-builder-function_scope branch December 6, 2023 13:00
@affeldt-aist
Copy link
Member

Sorry for the lag, I was super busy. So I guess there was a conflict of notation and you judged that since this notation was used seldom used in this file it was right to remove it, is that it?

@proux01
Copy link
Collaborator

proux01 commented Dec 7, 2023

Yes

@affeldt-aist affeldt-aist mentioned this pull request Dec 7, 2023
3 tasks
@affeldt-aist
Copy link
Member

affeldt-aist commented Dec 7, 2023

@pi8027 can you explain the logic? it looks like you removed the notation at some places but not at others?

@pi8027
Copy link
Member Author

pi8027 commented Dec 7, 2023

@affeldt-aist It seems that _ \; _ declared in the classical_set_scope has precedence over the _ \; _ local notation which I removed. I have no explanation for how math-comp/math-comp#1131 triggered this change, but MathComp analysis does not seem to use notation scopes correctly anyway.

@affeldt-aist
Copy link
Member

MathComp analysis does not seem to use notation scopes correctly anyway.

You mean for example that we should be more careful when setting that arguments of definitions such that:

Arguments measurable {d}%measure_display_scope {s} _%classical_set_scope.

?

@pi8027
Copy link
Member Author

pi8027 commented Dec 7, 2023

@affeldt-aist Yes, and we should first understand how Coq interprets notations: https://coq.inria.fr/refman/user-extensions/syntax-extensions.html#notation-scopes

@affeldt-aist
Copy link
Member

Thanks for the tip!

@pi8027 pi8027 changed the title Adapt to math-comp/math-comp#1131 [MC2] Adapt to math-comp/math-comp#1131 Dec 7, 2023
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.

4 participants