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

Properly pass the Ltac2 notation level to the gramlib API. #14094

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

ppedrot
Copy link
Member

@ppedrot ppedrot commented Apr 8, 2021

For some reason I was confusing the position and the level in the previous version of the code.

I am leaving this as a draft for now since we may seize the opportunity to have a richer level language for Ltac2 notations.

Fixes #11866: Ltac2 Notations do not respect precedence.

@ppedrot ppedrot mentioned this pull request Apr 8, 2021
2 tasks
@ppedrot ppedrot marked this pull request as ready for review April 15, 2021 13:09
@ppedrot ppedrot requested a review from a team as a code owner April 15, 2021 13:09
@ppedrot
Copy link
Member Author

ppedrot commented Apr 15, 2021

I am leaving this as a draft for now since we may seize the opportunity to have a richer level language for Ltac2 notations.

I changed my mind since we can alway introduce a backwards-compatible syntax using numbers for the built-in levels and combinators for user-defined levels. The camlp5 API actually provides something similar with the AFTER / BEFORE / FIRST / LAST / LIKE combinators so it's only a matter of exposing those constructs.

@ppedrot ppedrot added kind: fix This fixes a bug or incorrect documentation. part: ltac2 Issues and PRs related to the (in development) Ltac2 tactic langauge. labels Apr 15, 2021
@ppedrot ppedrot added this to the 8.14+rc1 milestone Apr 15, 2021
@ppedrot
Copy link
Member Author

ppedrot commented Apr 16, 2021

@JasonGross would you assign?

| Some n ->
let () =
if n < 0 || n > 6 then
user_err (str "Notation levels must range between 0 and 6")
Copy link
Member

Choose a reason for hiding this comment

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

Should this error message be documented?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added an entry in the refman.

@JasonGross
Copy link
Member

I can assign if you'd like me to, but I don't feel competent to review the code changes and the current test in the test-suite is not adequate to convince me that the levels are working as intended.

If you can design some test that will fail if the levels are not correctly associated, such that I no longer need to care what the code is doing, I'd be happy to take this.

Alternatively, if you want to merge it under the premise that what's going on is not worse than what was happening before, I can merge it from that perspective too.

@ppedrot ppedrot requested a review from a team as a code owner April 16, 2021 17:36
@ppedrot
Copy link
Member Author

ppedrot commented Apr 16, 2021

If you can design some test that will fail if the levels are not correctly associated

Not sure how to do that. Now that we can print the Ltac2 grammar we might try to rely on an output test but it'd be very fragile.

@JasonGross
Copy link
Member

If you can design some test that will fail if the levels are not correctly associated

Not sure how to do that. Now that we can print the Ltac2 grammar we might try to rely on an output test but it'd be very fragile.

Add some Ltac2 notations that parse into an embedded AST and check for equality. e.g.,

Ltac2 Type tree := [ Leaf | Node (tree * tree) ]
Ltac2 Notation tactic4(a) "node456" tactic6(b) : 5 := node (a, b).

or something like that, and then do stuff like Leaf node456 Leaf node456 Leaf and make sure that the tree you get is the one that you expect. Granted, this seems pretty tedious, so maybe it's worth writing a small script to write the Ltac2 for you to test all of the expected outputs.

@SkySkimmer
Copy link
Contributor

You can do something with types. eg in ocaml

let f (+) x y z = (x + y) + z
(* f : ('a -> 'b -> 'a) -> 'a -> 'b -> 'b -> 'a *)
 
let f (+) x y z = x + (y + z)
(* f : ('a -> 'b -> 'b) -> 'a -> 'a -> 'b -> 'b *)

Copy link
Member

@jfehrle jfehrle left a comment

Choose a reason for hiding this comment

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

Wording suggestions

doc/sphinx/proof-engine/ltac2.rst Outdated Show resolved Hide resolved
@ppedrot
Copy link
Member Author

ppedrot commented Apr 16, 2021

@JasonGross I am too lazy to write this kind of test when 1. it's unlikely to break again because of this API and 2. there are other Ltac2 parsing issues lurking around that were not yet observed because parsing has an explosive combinatoric flavour to it. I have added the test you suggested in the bug report, that was failing before and is now properly working. If by a very unfortunate mistake we use the wrong API again it will be caught anyways, the rest seems like we would need a complete parsing test suite for the language which is kind of unrelated to the PR.

For some reason I was confusing the position and the level in the previous
version of the code.

Fixes coq#11866: Ltac2 Notations do not respect precedence.
@JasonGross
Copy link
Member

@coqbot merge now

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Apr 21, 2021

@JasonGross: You can't merge the PR because you're not among the assignees.

@JasonGross JasonGross self-assigned this Apr 21, 2021
@JasonGross
Copy link
Member

@coqbot merge now

@coqbot-app coqbot-app bot merged commit b8b8c73 into coq:master Apr 21, 2021
@ppedrot ppedrot deleted the ltac2-notation-level-fix branch April 21, 2021 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation. part: ltac2 Issues and PRs related to the (in development) Ltac2 tactic langauge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ltac2 Notations do not respect precedence
4 participants