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

refactor: drop most CoeFun instances #1219

Closed
wants to merge 71 commits into from
Closed

refactor: drop most CoeFun instances #1219

wants to merge 71 commits into from

Conversation

urkud
Copy link
Member

@urkud urkud commented Dec 26, 2022

If we define a CoeFun instance instead of a FunLike instance, then Lean unfolds the definition. We don't want this, at least until we find a way to make it work with *HomClasses or find an equally convenient machinery.

@kbuzzard
Copy link
Member

My guess is that stuff like this will make simp work more like it works in Lean 3. Obviously we don't want to start refactoring mathlib4 before we've ported mathlib3 but these changes look pretty innocuous to me.

@digama0
Copy link
Member

digama0 commented Dec 26, 2022

Ping @gebner . At first glance this seems incorrect - we just need to make sure that any CoeFun instances are wrapped in functions with the @[coe] attribute. (PS: I'm on holiday this week and I'm probably not going to be spending much time on lean.)

@urkud
Copy link
Member Author

urkud commented Dec 27, 2022

@digama0 Lean 4 does not unfold FunLike.coe, so ((f : M →* N) : M → N) unfolds to FunLike.coe f, not to f.toFun. This allows us to use generic lemmas like map_add, map_sum etc.
In master, ((o : OrderIso α β) : α → β) unfolds to something ugly (AFAIR, it's FunLike.coe o.toOrderEmbedding.toEmbedding but I'm not sure). Of course, we can drop FunLike.coe and unfold it to o.toFun but this will be o.toEquiv.toFun, not just OrderIso.toFun o. This means that we'll need to replicate, e.g., lemmas about OrderEmbedding.
Similarly, without FunLike, ((f : M →* N) : M → N) is f.toOneHom.toFun and there is no clear way to apply MulHom.map_mul here.

An alternative approach is to have something like (non-dependent version for simplicity):

structure BundledHom (α β : Type _) (p : (α → β) → Sort _) [∀ f, Subsingleton (p f)] :=
(toFun : α → β)
(prop : p toFun)

with generic BundledHom.ext etc. Here p takes values in Sort _ to allow finsupp, equiv etc.

@semorrison semorrison added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 3, 2023
Comment on lines 619 to 629
@[simp]
theorem MonoidWithZeroHom.coe_mk [MulZeroOneClass M] [MulZeroOneClass N] (f h1 hmul) :
(MonoidWithZeroHom.mk f h1 hmul : M → N) = (f : M → N) := rfl
#align monoid_with_zero_hom.coe_mk MonoidWithZeroHom.coe_mk

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 this lemma moved? Does this order make it more or less similar to lean3?

Comment on lines 237 to 232
-- see Note [function coercion]
instance : CoeFun (r ↪r s) fun _ => α → β :=
⟨fun o => o.toEmbedding⟩

Copy link
Member

Choose a reason for hiding this comment

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

Should this have a #noalign or porting comment?

@gebner
Copy link
Member

gebner commented Jan 9, 2023

At first glance this seems incorrect - we just need to make sure that any CoeFun instances are wrapped in functions with the @[coe] attribute. (PS: I'm on holiday this week and I'm probably not going to be spending much time on lean.)

I think this PR is the right choice. If we want to have FunLike (and now is probably not the right time to explore other designs) then FunLike.coe should be the unique function symbol we use for hom-to-fun coercion.

@digama0
Copy link
Member

digama0 commented Jan 10, 2023

Okay, I retract my concern then.

@urkud
Copy link
Member Author

urkud commented Jan 10, 2023

I'll merge master and go over minor concerns tomorrow.

@urkud
Copy link
Member Author

urkud commented Jan 11, 2023

For some reason, the coercion of a MulEquiv gets unfolded. I don't know why.

@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 11, 2023
@semorrison semorrison added the blocked-by-other-PR This PR depends on another PR to Mathlib label Jan 12, 2023
@semorrison semorrison removed the blocked-by-other-PR This PR depends on another PR to Mathlib label Jan 25, 2023
@semorrison
Copy link
Contributor

This PR/issue depends on:

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Nov 8, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Dec 9, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Dec 21, 2023
@urkud
Copy link
Member Author

urkud commented Dec 29, 2023

All the good parts were moved to other PRs and merged, so I'm closing this one.

@urkud urkud closed this Dec 29, 2023
@urkud urkud deleted the YK-coe-fn branch December 29, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflict The PR has a merge conflict with master, and needs manual merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants