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

Transparent abstract #201

Merged
merged 7 commits into from
May 11, 2017
Merged

Transparent abstract #201

merged 7 commits into from
May 11, 2017

Conversation

JasonGross
Copy link
Member

This is a small change that allows a transparent version of tclABSTRACT.
Additionally, it factors the machinery of abstract through a
plugin-accessible function which allows alternate continuations (other
than exact_no_check), and allows alternate types (other than the type of the current goal).

It might be nice to have a better interface for caching terms. The use-case of cache_term_by_tactic_then is something like

  Tactic Notation "cache_term" constr(term) "run" tactic(tacK) :=
    let T := type of term in
    cache_term_by_tactic_then id gk ~opaque:false ~tac_type_from_gl:(fun gl -> T) ltac:(exact_no_check term) (fun (lem, args) -> ltac_apply tacK (applist (lem, args))).

I've separated this out into three commits: one which gives tclABSTRACT an option for transparent lemmas, one which makes a transparent_abstract tactic, and one which allows Fiat's plugin to not need typeclass hackery to cache terms. I'd like this to get merged into 8.6.

@JasonGross JasonGross force-pushed the transparent-abstract branch 3 times, most recently from c655892 to 8b6f53d Compare June 25, 2016 17:37
@ppedrot ppedrot added the 8.6 label Sep 9, 2016
@ppedrot
Copy link
Member

ppedrot commented Sep 9, 2016

The code looks OK to me, and I find the feature useful. Nonetheless, I though that the general trend promoted by @gares was to get rid of abstract rather than using it more...

@gares
Copy link
Member

gares commented Sep 9, 2016

Why 8.6? It was not in the road map was it?

@ppedrot
Copy link
Member

ppedrot commented Sep 9, 2016

That is right, I assumed it was and categorized it carelessly. Changing the tag then.

@ppedrot ppedrot added 8.7 and removed 8.6 labels Sep 9, 2016
@maximedenes maximedenes added this to the 8.7 milestone Oct 4, 2016
@maximedenes maximedenes removed the 8.7 label Oct 4, 2016
@maximedenes maximedenes removed this from the 8.7 milestone Nov 21, 2016
@maximedenes maximedenes added the kind: feature New user-facing feature request or implementation. label Nov 21, 2016
@JasonGross JasonGross force-pushed the transparent-abstract branch 2 times, most recently from 913d886 to 97e6990 Compare February 22, 2017 01:31
@maximedenes maximedenes requested a review from gares March 15, 2017 10:41
Copy link
Member

@gares gares left a comment

Choose a reason for hiding this comment

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

The code works if using ... is not given. Otherwise I get an interpretation error. @maximedenes and @ppedrot may clarify the ident v.s. preident mess.

The feature should be marked as "use at your own risk" for the following reasons:

  • building relevant (as in computationally relevant) terms with tactics is fragile and should not be recommended (e.g. recent regression in unimath in the econstr branch)
  • abstract cannot play well with asynchronous proofs, so scripts full of abstract .. using ... are doomed to be processed slower.

It also seems that there was a blacklist for .*_subproof and this patch introduces a .*_subterm class of definitions. Not sure we want to see these as results of Search.

@ejgallego
Copy link
Member

I was thinking of proposing some modifications on how abstract is handled, so maybe we could wait a bit on the decision for this PR?

@JasonGross
Copy link
Member Author

The feature should be marked as "use at your own risk" for the following reasons:

Done

It also seems that there was a blacklist for ._subproof and this patch introduces a ._subterm class of definitions. Not sure we want to see these as results of Search.

Added _subterm to the blacklist.

I was thinking of proposing some modifications on how abstract is handled, so maybe we could wait a bit on the decision for this PR?

@ejgallego Can you say more?

The code works if using ... is not given. Otherwise I get an interpretation error.

I can't reproduce. The test-suite file includes a test case for transparent_abstract tac using foo, and, other than misc/deps-order, the test-suite builds fine for me.

@ejgallego
Copy link
Member

@ejgallego Can you say more?

The idea won't be concrete at least until the next WG, but I was thinking of replacing side-effects with proof-local environments, such that:

  • the environment is guaranteed to be the same before and after Qed;
  • term cooking is removed;
  • pairs of (constr,env) are stored at Qed time, proofs are checked against their own env;
  • thus proof-private constants are not visible anymore (removing the need for the blacklist, etc);
  • Definitions and Proofs inside Proofs would be supported and reasonably clean;
  • and abstract shouldn't differ too much from the above.

this builds on the functional state handling patch for the STM. However I lack the necessary knowledge to see it the above proposed approach is a) workable and b) desirable.

@JasonGross
Copy link
Member Author

What about abstract and the transparent version here when used in Defined proofs?

Regardless, transparent_abstract is something I'd like to have regardless of such a change, and I expect that the bulk of abstract would not be changed, and that making such a change to abstract wouldn't be significantly complicated by this patch. Do you see a place where they'd conflict?

@ejgallego
Copy link
Member

In defined proofs you indeed must propagate the full modified env. Right now the mechanism relies on static analysis, but I'd like to experiment with futures. Thus, for the case of Qed, the env future is resolved immediately, and processing of the next sentence can continue, whereas for Defined they will block on the evaluation of the proof.

I think indeed the ideas I outlined above are orthogonal to this patch, but I wanted to be sure that there is no conflict.

@ejgallego
Copy link
Member

@JasonGross travis says that doc building is broken .

@maximedenes maximedenes added the needs: fixing The proposed code change is broken. label Apr 6, 2017
@JasonGross
Copy link
Member Author

Sorry for the delay. I've updated the docs to replace _ with \_ when outside of math mode, and they now build for me.

@JasonGross
Copy link
Member Author

Rebased on top of newly-merged econstr. Any update on the status of merging this?

JasonGross added a commit to JasonGross/coq that referenced this pull request Apr 11, 2017
Also move named arguments to the beginning of the functions.  As per
coq#201 (comment)
JasonGross added a commit to JasonGross/coq that referenced this pull request Apr 11, 2017
JasonGross added a commit to JasonGross/coq that referenced this pull request Apr 11, 2017
This is a small change that allows a transparent version of tclABSTRACT.
Additionally, it factors the machinery of [abstract] through a
plugin-accessible function which allows alternate continuations (other
than exact_no_check.

It might be nice to factor it further, into a cache_term function that
caches a term, and a separate bit that calls cache_term with the result
of running the tactic.
This will allow a cache_term tactic that doesn't suffer from the
Not_found anomalies of abstract in typeclass resolution.
Also move named arguments to the beginning of the functions.  As per
coq#201 (comment)
@JasonGross
Copy link
Member Author

Bump. Any update on this?

@JasonGross
Copy link
Member Author

@maximedenes please remove the fix_needed tag

@maximedenes maximedenes removed the needs: fixing The proposed code change is broken. label May 9, 2017
@maximedenes
Copy link
Member

@ejgallego @gares do you confirm this is ok for you? If yes, I'll merge it.

@ejgallego
Copy link
Member

Based on my understanding of the current situation and roadmap, this patch is fine for me. However, as @gares said, I would mark this tactic "use at your own risk/experimental".

People relying on this should be willing to deal with a possibly unstable feature.

@JasonGross
Copy link
Member Author

@ejgallego is the current warning in the refman insufficient?

@ejgallego
Copy link
Member

Personally that's OK for me, but I wonder what is the general policy here wrt experimental features. Maybe we should keep a list ?

@coqbot coqbot merged commit e4262a8 into coq:trunk May 11, 2017
@JasonGross
Copy link
Member Author

Thanks!

@JasonGross JasonGross deleted the transparent-abstract branch May 11, 2017 12:28
Matafou pushed a commit to Matafou/coq that referenced this pull request May 31, 2017
Also move named arguments to the beginning of the functions.  As per
coq#201 (comment)
Matafou pushed a commit to Matafou/coq that referenced this pull request May 31, 2017
Matafou pushed a commit to Matafou/coq that referenced this pull request May 31, 2017
@Zimmi48 Zimmi48 modified the milestone: 8.6beta1 Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New user-facing feature request or implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants