8000 Make interpretation of references with maximal implicit arguments in Ltac functions the same in strict and non-strict mode by herbelin · Pull Request #17084 · rocq-prover/rocq · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
8000

Make interpretation of references with maximal implicit arguments in Ltac functions the same in strict and non-strict mode #17084

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

Conversation

herbelin
Copy link
Member
@herbelin herbelin commented Jan 7, 2023

As shown in #16935, the interpretation of references in Ltac functions happens to be different in strict mode (i.e., in practice, non interactively) and in non-strict mode (i.e., in practice, interactively) regarding the insertion or not of maximal implicit arguments.

This comment at #16935 suggested to align the strict mode on the non-strict mode and this is what the PR does.

In particular, this ensures than in a configuration such as:

Class T.
Local Instance t : T := {}.
Parameter a : forall {t:T}, True.
Ltac f x := exact x.
Ltac g := f a.
Goal True.
g.

where g evaluates an Ltac function applied to a reference in strict mode and this reference has inferable maximal implicit arguments, the implicit arguments are indeed inserted, as if f a had been given directly.

I did not find other cases where the change of semantics is observable (i.e. the maximal implicit arguments need to be solvable by type class inference, otherwise it fails with an unresolved implicit arguments anyway). In the stdlib, the only occurrence of such pattern is class_apply (which plays the role of f above) and the compatibility can be preserved by turning class_apply into a tactic definition.

  • Added changelog.
  • Added / updated documentation.

@herbelin herbelin added part: ltac Issues and PRs related to the Ltac tactic language. kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. labels Jan 7, 2023
@herbelin herbelin requested review from a team as code owners January 7, 2023 13:46
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jan 7, 2023
@herbelin
Copy link
Member Author
herbelin commented Jan 7, 2023

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jan 7, 2023
@coqbot-app
Copy link
Contributor
coqbot-app bot commented Jan 7, 2023

The job library:ci-fiat_crypto_legacy has failed in allow failure mode
ping @JasonGross

@coqbot-app
Copy link
Contributor
coqbot-app bot commented Jan 7, 2023

🔴 CI failures at commit 3c99cec without any failure in the test-suite

✔️ Corresponding jobs for the base commit 64e93b1 succeeded

❔ Ask me to try to extract minimal test cases that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following targets: ci-fiat_crypto_legacy, ci-iris, ci-verdi_raft, ci-vst
  • You can also pass me a specific list of targets to minimize as arguments.
  • If you tag me saying @coqbot ci minimize all, I will additionally minimize the following target (which I do not suggest minimizing): ci-fiat_crypto (because base job at 64e93b1 failed)

@herbelin
Copy link
Member Author
herbelin commented Jan 7, 2023

For the record, here is a reduced form of the cause of failure in iris_example (whose primary cause is in autosubst):

Ltac is_succ s :=
  match s with
  | S ?n => constr:(Some n)
  | 0 => None
  end.

Goal True.
match is_succ 0 with
| Some _ => idtac "some"
| _ => idtac "none"
end.

In master, None is returned without is implicit argument and the branch _ is taken. With the PR, None is returned with its implicit argument, which is not inferable, so is_succ fails.

I would say that the PR behavior is what was implicitly expected, so that both branches of is_succ return an object of type option (for instance, in master, match is_succ 0 with Some _ => idtac "some" | None => idtac "none" end fails because it is not @None _ which is returned but @None).

@herbelin
Copy link
Member Author
herbelin commented Jan 8, 2023

The failure in vst and fiat-crypto are of the following form:

Ltac apply' f := apply f.
Ltac g := apply' eq_refl.
Goal 0=0.
g. (* fails with the PR due to the implicit argument of eq_refl being non inferable *)

I see two possible fixes at the level of the script:

  • enforce no implicit arguments in the script
    Ltac apply' f := apply f.
    Ltac g := apply' (@eq_refl).
    Goal 0=0.
    g. 
  • ensure that unresolved implicit arguments are not fatal in the script
    Ltac apply' f := apply f.
    Ltac g := apply' open_constr:(eq_refl).
    Goal 0=0.
    g. 

and two at the level of Coq, therefore needing no change in the scripts:

Note: For the record, verdi-raft now includes a backwards compatible patch.

@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label Mar 5, 2023
@coqbot-app
Copy link
Contributor
coqbot-app bot commented Apr 4, 2023

The "needs: rebase" label was set more than 30 days ago. If the PR is not rebased in 30 days, it will be automatically closed.

@coqbot-app coqbot-app bot added the stale This PR will be closed unless it is rebased. label Apr 4, 2023
This was the case in non-strict mode (via interpretation at runtime),
but, in strict mode, it was instead the reference without its maximal
implicit arguments.

The main impact is on "class_apply" which we reformulate as a tactic
notation to ensure that its argument is always interpreted as a term.
@herbelin herbelin force-pushed the master+align-tacintern-tacinterp-strict-non-strict-reference-interpretation branch from 3c99cec to b67680d Compare April 4, 2023 14:42
@coqbot-app coqbot-app bot added needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. and removed needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. stale This PR will be closed unless it is rebased. labels Apr 4, 2023
@ppedrot
Copy link
Member
ppedrot commented Apr 7, 2023

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Apr 7, 2023
@coqbot-app
Copy link
Contributor
coqbot-app bot commented Apr 7, 2023

The job library:ci-fiat_crypto_legacy has failed in allow failure mode
ping @JasonGross

@coqbot-app
Copy link
Contributor
coqbot-app bot commented Apr 7, 2023

🔴 CI failures at commit b67680d without any failure in the test-suite

✔️ Corresponding jobs for the base commit fc4ff97 succeeded

❔ Ask me to try to extract minimal test cases that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following targets: ci-fiat_crypto_legacy, ci-iris, ci-vst
  • You can also pass me a specific list of targets to minimize as arguments.
  • If you tag me saying @coqbot ci minimize all, I will additionally minimize the following target (which I do not suggest minimizing): ci-fiat_crypto (because base job at fc4ff97 failed)

@ppedrot ppedrot added the needs: progress Work in progress: awaiting action from the author. label Apr 25, 2023
@SkySkimmer
Copy link
Contributor

So what do we do with this? Write overlays and merge?

@SkySkimmer SkySkimmer added the needs: changelog entry This should be documented in doc/changelog. label Apr 17, 2024
@ppedrot
Copy link
Member
ppedrot commented Jul 23, 2024

I support this PR, so indeed in my opinion writing the overlays is the way to go.

SkySkimmer pushed a commit to SkySkimmer/rocq that referenced this pull request May 26, 2025
… impargs.

This was the case in non-strict mode (via interpretation at runtime),
but, in strict mode, it was instead the reference without its maximal
implicit arguments.

Also deprecate the old strict mode internalization.

For compat with enabling the option, the main impact is on
"class_apply" which we reformulate as a tactic notation to ensure that
its argument is always interpreted as a term.

Replaces rocq-prover#17084 in a more
backward compatible way.

Co-authored-by: Hugo Herbelin <herbelin@users.noreply.github.com>
SkySkimmer pushed a commit to SkySkimmer/rocq that referenced this pull request May 26, 2025
… impargs.

This was the case in non-strict mode (via interpretation at runtime),
but, in strict mode, it was instead the reference without its maximal
implicit arguments.

Also deprecate the old strict mode internalization.

For compat with enabling the option, the main impact is on
"class_apply" which we reformulate as a tactic notation to ensure that
its argument is always interpreted as a term.

Replaces rocq-prover#17084 in a more
backward compatible way.

Co-authored-by: Hugo Herbelin <herbelin@users.noreply.github.com>
@SkySkimmer
Copy link
Contributor

Replaced by #20682 which provides a deprecation phase

@SkySkimmer SkySkimmer closed this May 26, 2025
SkySkimmer pushed a commit to SkySkimmer/rocq that referenced this pull request May 28, 2025
… impargs.

This was the case in non-strict mode (via interpretation at runtime),
but, in strict mode, it was instead the reference without its maximal
implicit arguments.

Also deprecate the old strict mode internalization.

For compat with enabling the option, the main impact is on
"class_apply" which we reformulate as a tactic notation to ensure that
its argument is always interpreted as a term.

Replaces rocq-prover#17084 in a more
backward compatible way.

Co-authored-by: Hugo Herbelin <herbelin@users.noreply.github.com>
SkySkimmer pushed a commit to SkySkimmer/rocq that referenced this pull request Jun 3, 2025
… impargs.

This was the case in non-strict mode (via interpretation at runtime),
but, in strict mode, it was instead the reference without its maximal
implicit arguments.

Also deprecate the old strict mode internalization.

For compat with enabling the option, the main impact is on
"class_apply" which we reformulate as a tactic notation to ensure that
its argument is always interpreted as a term.

Replaces rocq-prover#17084 in a more
backward compatible way.

Co-authored-by: Hugo Herbelin <herbelin@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Enhancement to an existing user-facing feature, tactic, etc. needs: changelog entry This should be documented in doc/changelog. needs: progress Work in progress: awaiting action from the author. part: ltac Issues and PRs related to the Ltac tactic language.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0