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

New hook for early prompt/output analyzis. #495

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

Matafou
Copy link
Contributor

@Matafou Matafou commented May 27, 2020

proof-state-change-pre-hook happens earlier than
proof-state-change-hook, i.e. before proof-done-advancing. This should
be used to register information in the currently processed span before
proof-done-advancing classifies it.

Historically PG design was to gather these information during
proof-done-advancing (or in its hook called at the end) by just
looking at the command statement. But it is often useful to look at
the output (messages and/or prompt) to gather more accurate
information. Some of this information may be needed DURING
proof-done-advancing. Hence this early hook.

@Matafou
Copy link
Contributor Author

Matafou commented May 27, 2020

Trying to be a good guy proceeding with PR instead of wild (and buggy commit). Waiting for tests and comments.

When trying to correct PR #467 I detected that there was a bit of dependency loop between proof-done-advancing and its end-hook. Some of the information gathered in the span by this hook (e.g. is this command opening a goal?) was actually needed by proof-done-advancing (e.g.should we increment proof-nesting-depth?).

Notably this does actually not help to correct PR #467 since the information (are we inside a proof?) is needed even earlier. This is the subject of another commit.

@erikmd
Copy link
Member

erikmd commented May 27, 2020

Hi @Matafou, thanks, the idea looks good; two questions:

@Matafou
Copy link
Contributor Author

Matafou commented May 28, 2020

Hi @Matafou, thanks, the idea looks good; two questions:

  • which kind of "typical tests" would you have in mind? (BTW it might be possible to automate part of these tests in the CI if you'd like)

Difficult to test this currently without writing some lisp testing. Maybe once the proof-nesting generic code works (although it is more accurate now, currently the proof-nesting-depth variable is incorrect when backtracking).

No. The proof-done-advancing-hook remains. It is only that some of its content were triggered to late, hence the need for another hook.

Well spotted. I will add it there as the information about state number is typically done by the pre-hook now. It would probably break proof-tree otherwise.

This will have no effect but it is more consistent to change this to pre-hook too.

Thanks!

proof-state-change-pre-hook happens earlier than
proof-state-change-hook, i.e. before proof-done-advancing. This should
be used to register information in the currently processed span before
proof-done-advancing classifies it.

Historically PG design was to gather these information during
proof-done-advancing (or in its hook called at the end) by just
looking at the command statement. But it is often useful to look at
the output (messages and/or prompt) to gather more accurate
information. Some of this information may be needed DURING
proof-done-advancing. Hence this early hook.
@Matafou
Copy link
Contributor Author

Matafou commented May 28, 2020

I just had a look at the test infrastructure. Can we easily add adhoc lisp code to a test?

@erikmd
Copy link
Member

erikmd commented May 28, 2020

I just had a look at the test infrastructure. Can we easily add adhoc lisp code to a test?

Yes, it suffices to add an ERT form in coq-tests.el such as :

(ert-deftest 090_coq-test-pr-495 ()
  "Test for PR #495 about …" 
  (coq-fixture-on-file (coq-test-full-path "test_pr495.v") 
  (lambda ()
  ;elisp code, e.g.:
  (proof-process-buffer)
  (proof-shell-wait)
  ;optional assertion: (should…)
  ;otherwise: t)

assuming you need an accompanying file test_pr495.v

Then you can run the tests in two alternative ways:

  1. ./ci/test.sh or:
  2. C-x C-f coq-tests.el RET
    M-: (progn (load-file "coq-tests.el") (call-interactively #'ert)) RET

@Matafou
Copy link
Contributor Author

Matafou commented May 28, 2020

Cool. I will try to adopt this systematically.

@erikmd
Copy link
Member

erikmd commented May 28, 2020

If you don't need an ad-hoc file, you can just replace (coq-test-full-path "test_pr495.v") with nil and the coq-fixture-on-file function will take care of starting a fresh Coq instance on a temporary Coq file

@erikmd
Copy link
Member

erikmd commented May 28, 2020

Then you can run the tests in two alternative ways:

1. `./ci/test.sh` or:

2. C-x C-f coq-tests.el RET
   M-: (progn (load-file "coq-tests.el") (call-interactively #'ert)) RET

or more simply, just evaluating Line 8 in a buffer:

PG/ci/coq-tests.el

Lines 6 to 8 in d20ad3d

;;; Eval this to run the tests interactively <C-x C-e>
;;
;; (progn (load-file "coq-tests.el") (call-interactively #'ert))

@erikmd
Copy link
Member

erikmd commented May 28, 2020

Finally in the case of a failure in ERT interactive mode, you can click on a failing test and press b to get a backtrace, see also:
https://www.gnu.org/software/emacs/manual/html_node/ert/Running-Tests-Interactively.html#Running-Tests-Interactively

@Matafou
Copy link
Contributor Author

Matafou commented Jun 4, 2020

I merge this and will add tests later.

@Matafou Matafou merged commit 5c3ebac into ProofGeneral:master Jun 4, 2020
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.

2 participants