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] - refactor: add an ofNat() elaborator #20169

Closed
wants to merge 21 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Dec 22, 2024

ofNat(n) is a shorthand for no_index (OfNat.ofNat n).

Its purpose is:

This is not exhaustive, but is a step in the right direction.
For now, I only target (some of) the declarations with a See note [no_index around OfNat.ofNat].

Some theorem statements have been corrected in passing.

This exposed two Lean bugs relating to not using Expr.consumeMData:

These are made more likely by this PR adding no_index to the RHS of equalities, but were already possible to trigger by using simp [<- _] on existing theorems with no_index on the LHS.


Open in Gitpod

This lemma is malformed; the `n` on the RHS must be a raw literal, but the one on the left is not.
Correcting the statement results in `Nat.cast_ofNat` which already exists.
`ofNat(n)` is a shorthand for `no_index (OfNat.ofNat n)`.

Its purpose is:
* To make it easier to remember to write the `no_index`
* To add a place to put a hoverable docstrings explaining the complexities
* To make it easier to remove the `no_index`es in future if `DiscrTree`s stops needing them around `ofNat`.

This is not exhaustive, but is a step in the right direction.
For now, I only target (some of) the declarations with a `See note [no_index around OfNat.ofNat]`.

Some theorem statements have been corrected in passing.
Copy link

github-actions bot commented Dec 22, 2024

PR summary 555c7f825c

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference
There are 5066 files with changed transitive imports taking up over 214557 characters: this is too many to display!
You can run scripts/import_trans_difference.sh all locally to see the whole output.

Declarations diff

-++- ceil_ofNat
-++- floor_ofNat

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

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

The doc-module for script/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot added the blocked-by-other-PR This PR depends on another PR to Mathlib (this label is automatically managed by a bot) label Dec 22, 2024
@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot removed the blocked-by-other-PR This PR depends on another PR to Mathlib (this label is automatically managed by a bot) label Dec 22, 2024
@mathlib4-dependent-issues-bot
Copy link
Collaborator

This PR/issue depends on:

@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Dec 22, 2024
Copy link
Collaborator

@grunweg grunweg left a comment

Choose a reason for hiding this comment

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

Thanks for doing this clean-up! The diff does what is says. Somebody who knows this corner of mathlib needs to judge if this change is good.

@[to_additive (attr := simp)]
theorem op_ofNat [NatCast α] (n : ℕ) [n.AtLeastTwo] :
op (no_index (OfNat.ofNat n : α)) = OfNat.ofNat n :=
op (ofNat(n) : α) = ofNat(n) :=
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this is one example of fixing a theorem statement, right (as a no_index is also put on the RHS), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Arguably this doesn't change the statement, only the indexing of the statement. The corrections I was referring to in the description are the ones where a missing OfNat.ofNat is added.

@[simp]
theorem sqrt_ofNat (n : ℕ) : Int.sqrt (no_index (OfNat.ofNat n) : ℤ) = Nat.sqrt (OfNat.ofNat n) :=
theorem sqrt_ofNat (n : ℕ) : Int.sqrt ofNat(n) = Nat.sqrt ofNat(n) :=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Flagging: this is a change of theorem statement.

@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Dec 23, 2024
@mattrobball
Copy link
Collaborator

bors delegate+

Consider whether you want to wait until the next version release to clean up some of the workarounds. If not, please make a cleanup PR after it lands and ping me.

Either way, please merge current master before adding to the queue. Thanks.

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jan 1, 2025

✌️ eric-wieser can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@eric-wieser
Copy link
Member Author

Consider whether you want to wait until the next version release to clean up some of the workarounds.

I think there is going to be a long tail of workarounds as we propagate this and discover more missing consumeMDatas, so I don't think we should wait.

@eric-wieser eric-wieser added the auto-merge-after-CI Please do not add manually. Requests for a bot to merge automatically once CI is done. label Jan 1, 2025
@leanprover-community-mathlib4-bot
Copy link
Collaborator

As this PR is labelled auto-merge-after-CI, we are now sending it to bors:

bors merge

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the ready-to-merge This PR has been sent to bors. label Jan 1, 2025
mathlib-bors bot pushed a commit that referenced this pull request Jan 1, 2025
`ofNat(n)` is a shorthand for `no_index (OfNat.ofNat n)`.

Its purpose is:
* To make it easier to remember to write the `no_index`
* To add a place to put a hoverable docstrings explaining the complexities
* To make it easier to remove the `no_index`es in future if `DiscrTree`s stops needing them around `ofNat` (leanprover/lean4#2867).

This is not exhaustive, but is a step in the right direction.
For now, I only target (some of) the declarations with a `See note [no_index around OfNat.ofNat]`.

Some theorem statements have been corrected in passing.

This exposed two Lean bugs relating to not using `Expr.consumeMData`:

* leanprover/lean4#6438
* leanprover/lean4#6467

These are made more likely by this PR adding `no_index` to the RHS of equalities, but were already possible to trigger by using `simp [<- _]` on existing theorems with `no_index` on the LHS.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Jan 1, 2025

Pull request successfully merged into master.

Build succeeded!

And happy new year! 🎉

@mathlib-bors mathlib-bors bot changed the title refactor: add an ofNat() elaborator [Merged by Bors] - refactor: add an ofNat() elaborator Jan 1, 2025
@mathlib-bors mathlib-bors bot closed this Jan 1, 2025
@mathlib-bors mathlib-bors bot deleted the eric-wieser/ofNat() branch January 1, 2025 21:51
Julian added a commit that referenced this pull request Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-after-CI Please do not add manually. Requests for a bot to merge automatically once CI is done. delegated ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants