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

fix: use reducible transparency when producing no_confusion_type and injectivity lemmas #812

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jun 14, 2023

The relevant test case is

def wrapped_nat (n : nat) := nat

structure with_wrapped : Type :=
(n : nat)
(m : wrapped_nat n)

#check with_wrapped.mk.inj_eq

The diff due to this PR is

 ∀ {n : ℕ} {m : wrapped_nat n} {n' : ℕ} {m' : wrapped_nat n'},
-   with_wrapped.mk n m = with_wrapped.mk n' m' = (n = n' ∧ m = m'))
+   with_wrapped.mk n m = with_wrapped.mk n' m' = (n = n' ∧ m == m'))

which is an improvement since m and m' are not intended to be equal types.

It's not immediately clear to me whether this will stop the particular defeq check that is causing trouble in this motivating Zulip thread.

@eric-wieser eric-wieser requested a review from gebner June 14, 2023 12:49
@kim-em
Copy link
Contributor

kim-em commented Jun 15, 2023

I've build mathlib3 against this locally, no problems to report.

@ericrbg
Copy link
Contributor

ericrbg commented Jun 15, 2023

Scott, does it prevent the crash?

@eric-wieser
Copy link
Member Author

I tried to test this in leanprover-community/mathlib3#19192 but I can't work out how to make a release to build against.

@kim-em
Copy link
Contributor

kim-em commented Jun 15, 2023

But Eric, I don't think that helps: as I said I've already compiled mathlib3 master against this branch.

What I want is a mathlib3 commit that would fail against current lean3 master, that I can run against this.

@eric-wieser
Copy link
Member Author

@semorrison, the point is that would give us a cache that we can try all the mwes from the thread in.

@eric-wieser
Copy link
Member Author

Either way, I think the fact that mathlib succeeds with this change (combined with agreeing with the intended effect, whether or not it helps performance) is sufficient justification to merge it

@collares
Copy link

Doesn't an analogue of this have to land in Lean 4 first? What happens if this PR is merged and Leo doesn't like it for Lean 4?

@eric-wieser
Copy link
Member Author

We can use the option to disable the generation of these lemmas in Lean 4.

@digama0
Copy link
Member

digama0 commented Jun 15, 2023

fix: use semireducible transparency when producing no_confusion_type and injectivity lemmas #812

You mean reducible transparency, right?

@eric-wieser eric-wieser changed the title fix: use semireducible transparency when producing no_confusion_type and injectivity lemmas fix: use reducible transparency when producing no_confusion_type and injectivity lemmas Jun 15, 2023
@eric-wieser
Copy link
Member Author

Yes, I can never remember if "X transparency" includes X or not...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants