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

[Merged by Bors] - feat(tactic/protect_proj): protect_proj attribute for structures #2855

Closed
wants to merge 29 commits into from

Conversation

ChrisHughes24
Copy link
Member

This attribute protect the projections of a structure that is being defined.

There were some errors in Euclidean domain caused by rw using euclidean_domain.mul_assoc instead of mul_assoc because the euclidean_domain namespace was open. This fixes this problem, and makes the group and ring etc. namespaces more usable.

Copy link
Member

@robertylewis robertylewis left a comment

Choose a reason for hiding this comment

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

It's not needed now, but soon enough, someone will ask if they can protect only some of the fields of a structure. What about taking an optional list of identifiers? If it's not given, the behavior should be as it is now (protect all fields). If it's given, protect only those.

-/
import meta.expr
/-!
# protect_proj user attribute
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# protect_proj user attribute
# `protect_proj` user attribute

namespace tactic

/-- Tactic that is executed when a structure is marked with the `protect_proj` attribute -/
meta def protect_proj_tac (n : name) (env : environment) : tactic environment :=
Copy link
Member

Choose a reason for hiding this comment

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

I feel like I've noticed this before, do we still not have tactic.mk_protected? This seems like a big oversight.

I'd add this function to tactic.core and implement protect_proj_tac : name -> tactic unit that fails (with your current error message) if the name is not a structure. (There's environment.is_structure to test this.)

all of the projections become projected, meaning they must always be referred to by
their full name `foo.bar`, even when the `foo` namespace is open.",
after_set := some (λ n _ _, get_env >>= protect_proj_tac n >>= set_env) }

Copy link
Member

Choose a reason for hiding this comment

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

protect_proj should have an add_tactic_doc.

@ChrisHughes24
Copy link
Member Author

It's not needed now, but soon enough, someone will ask if they can protect only some of the fields of a structure. What about taking an optional list of identifiers? If it's not given, the behavior should be as it is now (protect all fields). If it's given, protect only those.

I can't work out how to do this. I give it the type user_attribute unit (list name) right? and then set parser := lean.parser.list_include_var_names. This didn't seem to work. The trouble is there are several meta constants of type lean.parser (list name), none of which are documented.

@ChrisHughes24
Copy link
Member Author

It's not needed now, but soon enough, someone will ask if they can protect only some of the fields of a structure. What about taking an optional list of identifiers? If it's not given, the behavior should be as it is now (protect all fields). If it's given, protect only those.

I can't work out how to do this. I give it the type user_attribute unit (list name) right? and then set parser := lean.parser.list_include_var_names. This didn't seem to work. The trouble is there are several meta constants of type lean.parser (list name), none of which are documented.

I worked this out. It takes a list of identifiers that shouldn't be protected, rather than a list that should, as this is usually a shorter list. I also added a protected user attribute, and the tactic you suggested, so if the protected list is shorter, you can just use this attribute.

@ChrisHughes24 ChrisHughes24 added the awaiting-review The author would like community review of the PR label May 29, 2020
@kckennylau
Copy link
Collaborator

src/tactic/protected.lean Outdated Show resolved Hide resolved
src/tactic/protected.lean Outdated Show resolved Hide resolved
src/tactic/protected.lean Outdated Show resolved Hide resolved
src/tactic/protected.lean Outdated Show resolved Hide resolved
src/tactic/protected.lean Outdated Show resolved Hide resolved
@robertylewis
Copy link
Member

It would also be nice to add a few tests for these functions!

ChrisHughes24 and others added 4 commits May 29, 2020 12:32
Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
@robertylewis
Copy link
Member

Thanks Chris, this looks great.

bors merge

@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels May 29, 2020
bors bot pushed a commit that referenced this pull request May 29, 2020
This attribute protect the projections of a structure that is being defined. 

There were some errors in Euclidean domain caused by `rw` using `euclidean_domain.mul_assoc` instead of `mul_assoc` because the `euclidean_domain` namespace was open. This fixes this problem, and makes the `group` and `ring` etc. namespaces more usable.

Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
@bors
Copy link

bors bot commented May 29, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(tactic/protect_proj): protect_proj attribute for structures [Merged by Bors] - feat(tactic/protect_proj): protect_proj attribute for structures May 29, 2020
@bors bors bot closed this May 29, 2020
@bors bors bot deleted the protect_proj branch May 29, 2020 18:08
rwbarton added a commit that referenced this pull request May 30, 2020
This uses the recent `protect_proj` attribute (#2855).
bors bot pushed a commit that referenced this pull request May 30, 2020
This uses the recent `protect_proj` attribute (#2855).
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
…nprover-community#2855)

This attribute protect the projections of a structure that is being defined. 

There were some errors in Euclidean domain caused by `rw` using `euclidean_domain.mul_assoc` instead of `mul_assoc` because the `euclidean_domain` namespace was open. This fixes this problem, and makes the `group` and `ring` etc. namespaces more usable.

Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants