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: make norm_num erasable (and scoped) #1364

Closed
wants to merge 30 commits into from

Conversation

thorimur
Copy link
Collaborator

@thorimur thorimur commented Jan 6, 2023

As per #1308, we give norm_nums the capability to be erased. E.g.

attribute [-norm_num] Mathlib.Meta.NormNum.evalPow

This required us to switch the implementation from a PersistentEnvExtenstion to a ScopedEnvExtension so that the attribute could be erased in a section (or via attribute [-norm_num] ... in) in a way the user would expect.

This also easily lets us now make norm_num a localizable attribute. (I imagine that's a rare occurrence, but may as well—the functionality comes for free with ScopedEnvExtension, and all we have to do is not prevent it.)

The following explains in detail what changes I make, but the actual changes to the code are small. If you're reviewing this, it might make more sense just to read the code.


To handle erasure:

  • We turn the state of the attribute into a structure and add the erased field
  • We add the name field to the extension structure and give it an autoparam to capture the name of the norm_num extension at hand
  • In erasing the attribute, we remove it from the state's Entrys if present, and when adding the attribute, we remove it from erased if present—the latter shouldn't ever need to happen, but simp apparently does it sometimes (but I don't know why), so I left it in there just to avoid unanticipated edge cases.
  • We perform the check that the attribute can be erased by confirming that it's present somewhere in the DiscrTree and that it's not already erased. If it can't be erased, we throw an error saying that the identifier does not have the norm_num attribute, like simp does.

To switch to ScopedEnvExtension, we:

  • remove exportEntriesFn, since ScopedEnvExtension performs an equivalent .toArray.reverse operation automatically.
  • refactor the fields of the extension as necessary (e.g. addImportedFn to ofOLeanEntry plus addEntry, with the rest being taken care of by ScopedEnvExtension's built-in addImportedFn)
  • remove the entries field from the state, since that's now taken care of through ScopedEnvExtension directly

To make it localizable, we adjust the add field when registering the attribute to:

  • remove the now-unnecessary guard on the kind
  • change the setEnv and addEntry to ScopedEnvExtension's add (which calls modifyEnv and addEntry behind the scenes on appropriately-scoped values)

Misc:

  • norm_num traces now print the name of the extension used for the rewrite, since it's now available (whereas it wasn't before).

@thorimur thorimur added the WIP Work in progress label Jan 6, 2023
@thorimur thorimur linked an issue Jan 6, 2023 that may be closed by this pull request
@hrmacbeth hrmacbeth added the t-meta Tactics, attributes or user commands label Jan 6, 2023
@thorimur
Copy link
Collaborator Author

thorimur commented Jan 6, 2023

Q's:

  • In derive, should we check whether erased is empty before looping over all matched extensions, or is it fast enough to check whether the extension is erased every pass?
  • Should I add more tests? (There's only one.)
  • Is the inline of parts of std4#56 handled appropriately? Resolved via appropriate import.

@thorimur thorimur added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Jan 6, 2023
Comment on lines 226 to 232
/- should error:
type mismatch
HEq.rfl
has type
HEq ?m.38198 ?m.38198 : Prop
but is expected to have type
3 ^ 3 + 4 = 31 : Prop
Copy link
Member

Choose a reason for hiding this comment

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

@thorimur Can you please use successIfFail here, even though there's no good way to make the with_msg part work?

Copy link
Collaborator Author

@thorimur thorimur Jan 6, 2023

Choose a reason for hiding this comment

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

Hmm, I'm having a bit of trouble translating the monad operation successIfFail into this context. Would a syntactic guard_target be alright? Or is admit frowned upon?

attribute [-norm_num] Mathlib.Meta.NormNum.evalPow
example : 3 ^ 3 + 4 = 31 := by
  norm_num1
  guard_target =ₛ 3 ^ 3 + 4 = 31
  admit

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a good approach! (Also you can probably change the admit to rfl, if you want to get rid of the warning.)

@hrmacbeth
Copy link
Member

  • Is the inline of parts of std4#56 handled appropriately?

I've never personally tried this, but can you update your lake-manifest to use the Std commit associated to std4#56? (If that breaks because that PR is too old, you might need to ask the author of that PR to merge master.)

@thorimur
Copy link
Collaborator Author

thorimur commented Jan 6, 2023

  • Is the inline of parts of std4#56 handled appropriately?

I've never personally tried this, but can you update your lake-manifest to use the Std commit associated to std4#56? (If that breaks because that PR is too old, you might need to ask the author of that PR to merge master.)

I'll try! I am a little wary of making all of mathlib4 depend on a non-master branch of std4, though...is that what would be happening?

(Also I should link this conversation with @digama0, just so it's visible to everyone)

EDIT: I tried, but it doesn't seem to like it enough to build. Maybe someone more familiar with lake-manifests could get it to work, though.

EDIT 2: I removed the commits to make the rebase to master work well...but that seems to have changed the timestamps of all the commits. oh well.

/- should error: 'Mathlib.Meta.NormNum.evalPow' does not have [norm_num] attribute -/
/-
attribute [-norm_num] Mathlib.Meta.NormNum.evalPow
attribute [-norm_num] Mathlib.Meta.NormNum.evalPow in
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify what these comments refer to? Are they tests which you hoped would work but don't?

Copy link
Collaborator Author

@thorimur thorimur Jan 6, 2023

Choose a reason for hiding this comment

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

Writing attribute [-norm_num] ... a second time should give the error that the identifier does not have the norm_num attribute, as it has already been erased. This comment tries to say that if you uncomment those lines of code, it should (and does) give that error—I'll clarify it so that it actually says that :)

Or maybe there's a way to use successIfFail here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clarified! I couldn't find a way to make successIfFail work, though.

@hrmacbeth
Copy link
Member

@thorimur Another possible approach to the dependency on leanprover/std4#56: If I understand correctly, @JLimperg made this PR in order to move some DiscrTree functionality in Aesop upstream to Std. Since Mathlib already imports Aesop, maybe we already have access to it? Do the methods in
https://github.com/JLimperg/aesop/blob/master/Aesop/Util/Basic.lean
do what you need?

@thorimur
Copy link
Collaborator Author

thorimur commented Jan 6, 2023

@thorimur Another possible approach to the dependency on leanprover/std4#56: If I understand correctly, @JLimperg made this PR in order to move some DiscrTree functionality in Aesop upstream to Std. Since Mathlib already imports Aesop, maybe we already have access to it? Do the methods in https://github.com/JLimperg/aesop/blob/master/Aesop/Util/Basic.lean do what you need?

Ah, great find! Importing Aesop.Util.Basic worked like a charm. :)

@thorimur thorimur added awaiting-review The author would like community review of the PR and removed awaiting-review The author would like community review of the PR labels Jan 7, 2023
@thorimur thorimur changed the title feat: make norm_num erasable feat: make norm_num erasable, scoped, and localizable Jan 9, 2023
@thorimur thorimur changed the title feat: make norm_num erasable, scoped, and localizable feat: make norm_num erasable (and scoped) Jan 9, 2023
@JLimperg
Copy link
Collaborator

With #1447, the DiscrTree functionality will move to Std.Lean.Meta.DiscrTree.

@thorimur
Copy link
Collaborator Author

thorimur commented Jan 10, 2023

With #1447, the DiscrTree functionality will move to Std.Lean.Meta.DiscrTree.

Thanks for the heads up (and the functionality)! :) I've rebased and the import has been updated.

Note: I needed to delete some experimental lake-manifest commits in order for the rebase to go through smoothly; the rebase seems to have changed the timestamps of all the commits in the web interface. I should have merged master... 🙃

@digama0
Copy link
Member

digama0 commented Jan 17, 2023

bors r+

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Jan 17, 2023
bors bot pushed a commit that referenced this pull request Jan 17, 2023
As per #1308, we give `norm_num`s the capability to be erased. E.g.
```lean
attribute [-norm_num] Mathlib.Meta.NormNum.evalPow
```
This required us to switch the implementation from a `PersistentEnvExtenstion` to a `ScopedEnvExtension` so that the attribute could be erased in a section (or via `attribute [-norm_num] ... in`) in a way the user would expect.

This also easily lets us now make `norm_num` a `local`izable attribute. (I imagine that's a rare occurrence, but may as well—the functionality comes for free with `ScopedEnvExtension`, and all we have to do is *not* prevent it.)

The following explains in detail what changes I make, but the actual changes to the code are small. If you're reviewing this, it might make more sense just to read the code.
@bors
Copy link

bors bot commented Jan 17, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: make norm_num erasable (and scoped) [Merged by Bors] - feat: make norm_num erasable (and scoped) Jan 17, 2023
@bors bors bot closed this Jan 17, 2023
@bors bors bot deleted the erase-norm_num branch January 17, 2023 03:38
jcommelin pushed a commit that referenced this pull request Jan 23, 2023
As per #1308, we give `norm_num`s the capability to be erased. E.g.
```lean
attribute [-norm_num] Mathlib.Meta.NormNum.evalPow
```
This required us to switch the implementation from a `PersistentEnvExtenstion` to a `ScopedEnvExtension` so that the attribute could be erased in a section (or via `attribute [-norm_num] ... in`) in a way the user would expect.

This also easily lets us now make `norm_num` a `local`izable attribute. (I imagine that's a rare occurrence, but may as well—the functionality comes for free with `ScopedEnvExtension`, and all we have to do is *not* prevent it.)

The following explains in detail what changes I make, but the actual changes to the code are small. If you're reviewing this, it might make more sense just to read the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors. t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement erase method for norm_num attribute
4 participants