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: make RBNode.mem_insert more robust #342

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: make RBNode.mem_insert more robust #342

wants to merge 1 commit into from

Conversation

kim-em
Copy link
Collaborator

@kim-em kim-em commented Nov 1, 2023

This proof has three different hypotheses named h, and in a upcoming version of leanprover/lean4#2793 something is going to change in how they shadow each other.

This is a fix that works before and after. For now I'll be pointing leanprover-community/mathlib4#8076 at this branch.

@digama0 this proof has some other scary features (e.g. nonterminal simp) which we should fix at some point.

@digama0
Copy link
Member

digama0 commented Nov 1, 2023

Can you say more about how the shadowing will change? That sounds a bit scary. (Can you point me to a branch that fails without this change?)

Regarding nonterminal simp, I don't avoid it here as heavily as in mathlib, because the simp set is small(er) and stable(r), and proofs tend to involve more domain-specific notions (like the RBSet functions in this case) for which the simp set is known exactly. I think the jury is still out regarding whether this is a good idea or whether we need to be as defensive as mathlib wrt the simp set.

@kim-em
Copy link
Collaborator Author

kim-em commented Nov 1, 2023

@digama0, try the Std branch lean-pr-testing-2793. It has no changes except to use the toolchain from leanprover/lean4#2793.

@digama0
Copy link
Member

digama0 commented Nov 1, 2023

It seems the issue is that the rw [find?_eq_zoom] at h line does not delete the original h, and the reason for this is that it results in h: root? (zoom (cmp v) t ?inr.p).fst = some v' where ?inr.p could possibly depend on the original h. The mvar is unified away in the next rewrite, but it should have been inferred from the optParam in find?_eq_zoom, and rw [find?_eq_zoom (p := .root)] at h fixes the issue. In other words, this is another manifestation of the issue you reported: leanprover/lean4#2793 (comment)

@kim-em
Copy link
Collaborator Author

kim-em commented Nov 1, 2023

Ah, great, thank you.

I'm happy in that case to either close this or merge, as you see fit!

@digama0
Copy link
Member

digama0 commented Nov 1, 2023

Let's leave it open until we find out how leanprover/lean4#2793 (comment) will be resolved.

@digama0 digama0 added the depends on core changes This PR need only be reviewed after changes land in Lean core. label Nov 3, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label May 7, 2024
@fgdorais
Copy link
Collaborator

The original upstream PR was subsumed by lean4#4596. Does this mean this PR is obsolete?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on core changes This PR need only be reviewed after changes land in Lean core. merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants