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(to_additive): unfold lemmas generated by simp #14628

Closed
wants to merge 5 commits into from

Conversation

fpvandoorn
Copy link
Member

  • This removes a bunch of hacky code
  • Thanks to @kmill for the idea
  • Also fix a bug in the implementation of @[to_additive (attr := to_additive, x)]
  • The removed test tested for an implementation detail. The rest of the test file (and Mathlib) still passes, which shows that it still works as desired.

Open in Gitpod

Copy link

PR summary 360d18f00a

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Lean.Name 3 4 +1 (+33.33%)
Mathlib.Tactic.ToAdditive 34 39 +5 (+14.71%)
Import changes for all files
Files Import difference
Too many changes (4249)!

Declarations diff

+ Lean.Meta.unfoldAuxLemmas
+ Lean.Name.isAuxLemma
- addSimpAttr
- addSimpAttrFromSyntax
- addSimpTheorem'
- mkSimpTheoremsFromConst'

You can run this locally as follows
## summary with just the declaration names:
./scripts/no_lost_declarations.sh short <optional_commit>

## more verbose report:
./scripts/no_lost_declarations.sh <optional_commit>

Copy link
Contributor

@kmill kmill left a comment

Choose a reason for hiding this comment

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

Looks good to me. The best kind of feature is one that's implemented by mostly deleting code!

For due diligence, I'll mention that I think the whole auxLemma business is about trying to avoid re-typechecking proof terms, so unfolding auxLemmas defeats this optimization, however I don't believe that simp's auxLemmas tend to be very complicated.

@fpvandoorn fpvandoorn added the t-meta Tactics, attributes or user commands label Jul 10, 2024
@eric-wieser
Copy link
Member

What's the "feat" here? Is there any visible behavior change, or is this just internal cleanup?

@eric-wieser
Copy link
Member

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 360d18f.
There were no significant changes against commit 5b4d457.

@fpvandoorn
Copy link
Member Author

See the issues that reference this: one feature request there is now fixed, another bug might be fixed (haven't tested)

@kim-em
Copy link
Contributor

kim-em commented Jul 15, 2024

bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Jul 15, 2024
mathlib-bors bot pushed a commit that referenced this pull request Jul 15, 2024
* This removes a bunch of hacky code
* Thanks to @kmill for the idea
* Also fix a bug in the implementation of `@[to_additive (attr := to_additive, x)]`
* The removed test tested for an implementation detail. The rest of the test file (and Mathlib) still passes, which shows that it still works as desired.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jul 16, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat(to_additive): unfold lemmas generated by simp [Merged by Bors] - feat(to_additive): unfold lemmas generated by simp Jul 16, 2024
@mathlib-bors mathlib-bors bot closed this Jul 16, 2024
@mathlib-bors mathlib-bors bot deleted the fvd/to_additive_unfold_aux branch July 16, 2024 00:30
@adomani adomani mentioned this pull request Aug 1, 2024
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.

5 participants