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

chore: fixes for leanprover/lean4#2724 (Fin USize.size) #7853

Closed
wants to merge 684 commits into from

Conversation

semorrison
Copy link
Contributor

@semorrison semorrison commented Oct 23, 2023

Patches required for leanprover/lean4#2724.

The only relevant diff is in Mathlib.Data.UInt. Any other changes are parts of nightly-testing required to make this build.


Open in Gitpod

@semorrison semorrison changed the title chore: fixes for leanprover/lean4#2724 (Fin USize.size) chore: fixes for leanprover/lean4#2724 (Fin USize.size) [merged into nightly-testing] Oct 25, 2023
@semorrison semorrison added v4.3.0 This PR needs to wait until we move to `v4.3.0-rc1`. It will be merged to `nightly-testing` first. merged-into-nightly-testing This has been merged into `nightly-testing`, and will join `master` when we update the toolchain. labels Oct 25, 2023
@semorrison semorrison changed the title chore: fixes for leanprover/lean4#2724 (Fin USize.size) [merged into nightly-testing] chore: fixes for leanprover/lean4#2724 (Fin USize.size) Oct 25, 2023
Comment on lines 8 to 16
lemma UInt8.val_eq_of_lt {a : Nat} : a < UInt8.size -> (ofNat a).val = a := Nat.mod_eq_of_lt
lemma UInt8.val_eq_coe {a : Nat} : (ofNat a).val = a := rfl

lemma UInt16.val_eq_of_lt {a : Nat} : a < UInt16.size -> (ofNat a).val = a := Nat.mod_eq_of_lt
lemma UInt16.val_eq_coe {a : Nat} : (ofNat a).val = a := rfl

lemma UInt32.val_eq_of_lt {a : Nat} : a < UInt32.size -> (ofNat a).val = a := Nat.mod_eq_of_lt
lemma UInt32.val_eq_coe {a : Nat} : (ofNat a).val = a := rfl

lemma UInt64.val_eq_of_lt {a : Nat} : a < UInt64.size -> (ofNat a).val = a := Nat.mod_eq_of_lt
lemma UInt64.val_eq_coe {a : Nat} : (ofNat a).val = a := rfl

lemma USize.val_eq_of_lt {a : Nat} : a < USize.size -> (ofNat a).val = a := Nat.mod_eq_of_lt
lemma USize.val_eq_coe {a : Nat} : (ofNat a).val = a := rfl
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you changed the statement rather than fixing the syntax here? What actually changed is that lean now puts the coercion on the RHS, but the lemma was hoping it would appear on the LHS.

Writing an explicit coercion seems like it would be the best approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I missed that the statement had changed. I wish our default for lemmas that are actually about coercions was that we always wrote the coercion explicitly in the statement, to make it obvious what the lemma is actually saying.

I've fixed this now, and this PR becomes trivial.

Copy link
Member

Choose a reason for hiding this comment

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

I wish our default for lemmas that are actually about coercions was that we always wrote the coercion explicitly in the statement, to make it obvious what the lemma is actually saying.

Unfortunately there are still cases where writing the coercion in the place where the pretty-printer shows it actually changes the meaning of the lemma! An obvious example is the coercion on morphisms, which in my experience fails to roundtrip at all every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but that is not a reason to not write the coercions wherever possible!

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added merge-conflict The PR has a merge conflict with master, and needs manual merging. and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. labels Oct 26, 2023
@eric-wieser
Copy link
Member

bors d+

Sorry for not doing so earlier, I wasn't confident in my understanding of the diff any more

(Though I understand this won't be merged)

@bors
Copy link

bors bot commented Oct 31, 2023

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

@github-actions github-actions bot added delegated and removed awaiting-review The author would like community review of the PR labels Oct 31, 2023
bors bot pushed a commit that referenced this pull request Oct 31, 2023
This incorporates changes from 

* #7845
* #7847
* #7853
* #7872 (was never actually made to work, but the diffs in `nightly-testing` are unexciting: we need to fully qualify a few names)

They can all be closed when this is merged.



Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@semorrison
Copy link
Contributor Author

This was rolled into #8051.

@semorrison semorrison closed this Oct 31, 2023
fgdorais pushed a commit that referenced this pull request Nov 1, 2023
This incorporates changes from 

* #7845
* #7847
* #7853
* #7872 (was never actually made to work, but the diffs in `nightly-testing` are unexciting: we need to fully qualify a few names)

They can all be closed when this is merged.



Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked-by-core-release Not relevant for the current Lean release candidate, but will be needed for the next. delegated merge-conflict The PR has a merge conflict with master, and needs manual merging. merged-into-nightly-testing This has been merged into `nightly-testing`, and will join `master` when we update the toolchain. v4.3.0 This PR needs to wait until we move to `v4.3.0-rc1`. It will be merged to `nightly-testing` first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants