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] - fix: prevent Int.ofNat being used instead of Nat.cast #2878

Closed
wants to merge 2 commits into from

Conversation

eric-wieser
Copy link
Member

Without this override, the coercion is found as instCoeNatInt : Coe ℕ ℤ which unfolds to Int.ofNat. This only happened when searching for Coe ℕ ℤ; CoeHTCT ℕ ℤ would already find the correct instance.


Open in Gitpod

Without this override, the coercion is found as `instCoeNatInt : Coe ℕ ℤ` which unfolds to `Int.ofNat`.
This only happened when searching for `Coe ℕ ℤ`; `CoeHTCT ℕ ℤ` would already find the correct instance.
@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Mar 14, 2023
@eric-wieser eric-wieser requested a review from gebner March 14, 2023 18:24
@@ -45,6 +45,9 @@ instance [NatCast R] : CoeTail ℕ R where coe := Nat.cast
-- see note [coercion into rings]
instance [NatCast R] : CoeHTCT ℕ R where coe := Nat.cast
Copy link
Member

Choose a reason for hiding this comment

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

These two are in std4 now. ^^

@gebner
Copy link
Member

gebner commented Mar 14, 2023

Right instance, but it should go into std4.

@eric-wieser
Copy link
Member Author

Are you happy to add the instance in both places, and remove this one next time we bump std4?

@gebner
Copy link
Member

gebner commented Mar 14, 2023

We already have two duplicated instances above it, so it doesn't change much. Please make a PR that adds the instance to std4 and a PR to remove the 2 (3) instances from mathlib4. In the meantime, feel free to
bors d+
to unblock things here.

@bors
Copy link

bors bot commented Mar 14, 2023

✌️ 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.

@github-actions github-actions bot added delegated and removed awaiting-review The author would like community review of the PR labels Mar 14, 2023
@eric-wieser
Copy link
Member Author

bors merge

I'll make the Std4 PR shortly

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Mar 14, 2023
bors bot pushed a commit that referenced this pull request Mar 14, 2023
Without this override, the coercion is found as `instCoeNatInt : Coe ℕ ℤ` which unfolds to `Int.ofNat`. This only happened when searching for `Coe ℕ ℤ`; `CoeHTCT ℕ ℤ` would already find the correct instance.
@eric-wieser
Copy link
Member Author

std4 PR is leanprover-community/batteries#107

@bors
Copy link

bors bot commented Mar 14, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix: prevent Int.ofNat being used instead of Nat.cast [Merged by Bors] - fix: prevent Int.ofNat being used instead of Nat.cast Mar 14, 2023
@bors bors bot closed this Mar 14, 2023
@bors bors bot deleted the instCoeNatInt-override branch March 14, 2023 23:21
bors bot pushed a commit that referenced this pull request Mar 17, 2023
We bump std4 and take care to delete the now-upstreamed (leanprover-community/batteries#107) instances mentioned in #2878.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants