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

Ltac call trace does not include tactics run by ML tactics #6404

Closed
JasonGross opened this issue Dec 12, 2017 · 0 comments · Fixed by #6407
Closed

Ltac call trace does not include tactics run by ML tactics #6404

JasonGross opened this issue Dec 12, 2017 · 0 comments · Fixed by #6407
Labels
kind: user messages Improvement of error messages, new warnings, etc. part: ltac Issues and PRs related to the Ltac tactic language.
Milestone

Comments

@JasonGross
Copy link
Member

JasonGross commented Dec 12, 2017

Version

8.7.0

Description of the problem

Ltac a _ := pose (I : I).
Ltac b _ := a ().
Ltac abs _ := transparent_abstract b ().
Ltac c _ := abs ().
Goal True.
  Fail c ().
  (* The command has indeed failed with message:
In nested Ltac calls to "c", "abs" and "transparent_abstract (tactic3)", last call
failed.
The term "I" has type "True" which should be Set, Prop or Type. *)
Abort.

I expect to see a and b in the trace.

This seems to be semi-intentional, as there is a function specifically dedicated to doing this:

let skip_extensions trace =
let rec aux = function
| (_,Tacexpr.LtacNameCall f as tac) :: _
when Tacenv.is_ltac_for_ml_tactic f -> [tac]
| (_,Tacexpr.LtacNotationCall _ as tac) :: (_,Tacexpr.LtacMLCall _) :: _ ->
(* Case of an ML defined tactic with entry of the form <<"foo" args>> *)
(* see tacextend.mlp *)
[tac]
| (_,Tacexpr.LtacMLCall _ as tac) :: _ -> [tac]
| t :: tail -> t :: aux tail
| [] -> [] in
List.rev (aux (List.rev trace))

I think it is right to coalesce sequenced Tacexpr.LtacNotationCall and Tacexpr.LtacMLCall nodes, but wrong to skip all subsequent calls.

@JasonGross JasonGross added kind: user messages Improvement of error messages, new warnings, etc. part: ltac Issues and PRs related to the Ltac tactic language. labels Dec 12, 2017
JasonGross added a commit to JasonGross/coq that referenced this issue Dec 12, 2017
This closes coq#5082 and coq#5778, but makes coq#6404 apply to `abstract` as well
as `transparent_abstract`.  This is unfortunate, but I think it is worth
it to get `abstract` in the profile (and therefore not misassign the
time spent sending the subproof to the kernel).  Another alternative
would have been to add a dedicated entry to `ltac_call_kind` for
`TacAbstract`, but I think it's better to just deal with coq#6404 all at
once.

The "better" solution here would have been to move `abstract` out of the
Ltac syntax tree and define it via `TACTIC EXTEND` like
`transparent_abstract`.  However, the STM relies on its ability to
recognize `abstract` and `solve [ abstract ... ]` syntactically so that
it can handle `par: abstract`.

Note that had to add locations to `TacAbstract` nodes, as I could not
figure out how to call `push_trace` otherwise.
JasonGross added a commit to JasonGross/coq that referenced this issue Dec 12, 2017
This closes coq#5082 and coq#5778, but makes coq#6404 apply to `abstract` as well
as `transparent_abstract`.  This is unfortunate, but I think it is worth
it to get `abstract` in the profile (and therefore not misassign the
time spent sending the subproof to the kernel).  Another alternative
would have been to add a dedicated entry to `ltac_call_kind` for
`TacAbstract`, but I think it's better to just deal with coq#6404 all at
once.

The "better" solution here would have been to move `abstract` out of the
Ltac syntax tree and define it via `TACTIC EXTEND` like
`transparent_abstract`.  However, the STM relies on its ability to
recognize `abstract` and `solve [ abstract ... ]` syntactically so that
it can handle `par: abstract`.

Note that had to add locations to `TacAbstract` nodes, as I could not
figure out how to call `push_trace` otherwise.
JasonGross added a commit to JasonGross/coq that referenced this issue Dec 12, 2017
This closes coq#5082 and closes coq#5778, but makes coq#6404 apply to `abstract`
as well as `transparent_abstract`.  This is unfortunate, but I think it
is worth it to get `abstract` in the profile (and therefore not
misassign the time spent sending the subproof to the kernel).  Another
alternative would have been to add a dedicated entry to `ltac_call_kind`
for `TacAbstract`, but I think it's better to just deal with coq#6404 all
at once.

The "better" solution here would have been to move `abstract` out of the
Ltac syntax tree and define it via `TACTIC EXTEND` like
`transparent_abstract`.  However, the STM relies on its ability to
recognize `abstract` and `solve [ abstract ... ]` syntactically so that
it can handle `par: abstract`.

Note that had to add locations to `TacAbstract` nodes, as I could not
figure out how to call `push_trace` otherwise.
JasonGross added a commit to JasonGross/coq that referenced this issue Dec 12, 2017
JasonGross added a commit to JasonGross/coq that referenced this issue Dec 12, 2017
JasonGross added a commit to JasonGross/coq that referenced this issue Dec 14, 2017
This closes coq#5082 and closes coq#5778, but makes coq#6404 apply to `abstract`
as well as `transparent_abstract`.  This is unfortunate, but I think it
is worth it to get `abstract` in the profile (and therefore not
misassign the time spent sending the subproof to the kernel).  Another
alternative would have been to add a dedicated entry to `ltac_call_kind`
for `TacAbstract`, but I think it's better to just deal with coq#6404 all
at once.

The "better" solution here would have been to move `abstract` out of the
Ltac syntax tree and define it via `TACTIC EXTEND` like
`transparent_abstract`.  However, the STM relies on its ability to
recognize `abstract` and `solve [ abstract ... ]` syntactically so that
it can handle `par: abstract`.

Note that had to add locations to `TacAbstract` nodes, as I could not
figure out how to call `push_trace` otherwise.
jfehrle pushed a commit to jfehrle/coq that referenced this issue Feb 11, 2018
This closes coq#5082 and closes coq#5778, but makes coq#6404 apply to `abstract`
as well as `transparent_abstract`.  This is unfortunate, but I think it
is worth it to get `abstract` in the profile (and therefore not
misassign the time spent sending the subproof to the kernel).  Another
alternative would have been to add a dedicated entry to `ltac_call_kind`
for `TacAbstract`, but I think it's better to just deal with coq#6404 all
at once.

The "better" solution here would have been to move `abstract` out of the
Ltac syntax tree and define it via `TACTIC EXTEND` like
`transparent_abstract`.  However, the STM relies on its ability to
recognize `abstract` and `solve [ abstract ... ]` syntactically so that
it can handle `par: abstract`.

Note that had to add locations to `TacAbstract` nodes, as I could not
figure out how to call `push_trace` otherwise.
JasonGross added a commit to JasonGross/coq that referenced this issue Mar 24, 2018
JasonGross added a commit to JasonGross/coq that referenced this issue Apr 2, 2018
@Zimmi48 Zimmi48 added this to the 8.9+beta1 milestone Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: user messages Improvement of error messages, new warnings, etc. part: ltac Issues and PRs related to the Ltac tactic language.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants