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

feat: add Bitvec.ofInt_ofNat #5081

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Conversation

tobiasgrosser
Copy link
Contributor

@tobiasgrosser tobiasgrosser commented Aug 18, 2024

We use no_index to work around special-handling of OfNat.ofNat in DiscrTree, which has been reported as an issue in #2867 and is currently in the process of being fixed in #3684. As the potential fix seems non-trivial and might need some time to arrive in-tree, we meanwhile add the no_index keyword to the problematic subterm.

@tobiasgrosser tobiasgrosser requested a review from kim-em as a code owner August 18, 2024 06:57
We use no_index to annotate a problematic subterm. With the no_index annotation
this `ofInt_ofNat` fires as expected.
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Aug 18, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Batteries CI can not be attempted yet, as the nightly-testing-2024-08-17 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Batteries CI should run now. (2024-08-18 07:18:25)

@tobiasgrosser tobiasgrosser marked this pull request as draft August 18, 2024 11:46
@tobiasgrosser
Copy link
Contributor Author

I am currently debugging why no_index is needed.

@tobiasgrosser
Copy link
Contributor Author

tobiasgrosser commented Aug 18, 2024

I have two versions: one fails, and the other one works. Advice on what is going on would be appreciated. I would be OK with the no_index version but would prefer to add one sentence explaining why it is needed.

@[simp] theorem ofInt_ofNat (w n : Nat) :
    BitVec.ofInt w (no_index (OfNat.ofNat n)) = BitVec.ofNat w n :=
  rfl

@[simp] theorem ofInt_ofNat_no_noindex (w n : Nat) :
    BitVec.ofInt w (OfNat.ofNat n) = BitVec.ofNat w n :=
  rfl
example : BitVec.ofInt w 1 = 1#w + 0#w := by
  simp only [ofInt_ofNat] -- this one works
  simp only [BitVec.add_zero]

example : BitVec.ofInt w 1 = 1#w + 0#w := by
  simp only [ofInt_ofNat_no_noindex] -- this one fails
  simp only [BitVec.add_zero]

Corresponding zulip discussion: https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/no_index.20required.20on.20ofNat.20instance/near/463300187

@tobiasgrosser tobiasgrosser marked this pull request as ready for review August 19, 2024 05:22
@tobiasgrosser
Copy link
Contributor Author

tobiasgrosser commented Aug 19, 2024

@urkud kindly provide the relevant context to understand the no_index issue in the zulip thread. I am now convinced that no_index is needed at the moment and put the relevant information into the PR summary at the top. This PR is now ready to review.

@tobiasgrosser
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added the awaiting-review Waiting for someone to review the PR label Aug 20, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 20, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 20, 2024
@leanprover-bot leanprover-bot added the P-high We will work on this issue label Aug 23, 2024
@kim-em kim-em enabled auto-merge August 26, 2024 12:12
auto-merge was automatically disabled August 26, 2024 13:00

Head branch was pushed to by a user without write access

@hargoniX hargoniX enabled auto-merge August 26, 2024 13:07
@hargoniX hargoniX added this pull request to the merge queue Aug 26, 2024
Merged via the queue into leanprover:master with commit c6feffa Aug 26, 2024
13 checks passed
@kim-em kim-em deleted the bitvec_ofInt_ofNat branch August 27, 2024 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR P-high We will work on this issue toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants