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: fix the MonadStore type classes, with semiOutParam #4205

Merged
merged 16 commits into from
May 24, 2024

Conversation

JovanGerb
Copy link
Contributor

@JovanGerb JovanGerb commented May 17, 2024

The type class MonadStore1 and friends have an outParam, which should not be an outParam, because there are multiple possible values for this parameter. At this function fetchOrCreate, there are multiple stacked StateT monad transformers that each give a different instance to MonadStore1. It is an implementation detail of type class synthesis which instance is found. This particular type class synthesis fails when the unused instance Lake.instMonadStore1OfMonadDStoreOfFamilyOut is set to a lower priority, because then the synthesis order happens to go differently, so the wrong instance is found.

Replacing the outParam with a semiOutParam solves this issue. Thus, we make a new type class MonadStore1Of, which is the same, but with a semiOutParam. This follows the design of MonadState and MonadStateOf.

However, then it turns out that the instance cannot anymore be synthesised.

There are two instances for MonadStore1:

instance [MonadDStore κ β m] : MonadStore1 k (β k) m
instance [MonadDStore κ β m] [FamilyOut β k α] : MonadStore1 k α m 

The first one is problematic during unification, especially when β should be instantiated as a constant function. We make the second one sufficient by adding an instance for the general type family:

/-- The general type family -/
instance (priority := low) : FamilyDef Fam a (Fam a) where
  family_key_eq_type := rfl

So then we can get rid of the first instance.

@JovanGerb JovanGerb requested a review from tydeu as a code owner May 17, 2024 20:10
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented May 17, 2024

Mathlib CI status (docs):

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label May 18, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 18, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 18, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 19, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 19, 2024
@JovanGerb
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added the awaiting-review Waiting for someone to review the PR label May 21, 2024
Copy link
Member

@tydeu tydeu left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! ❤️ The MonadStore(1)/MonadStore(1)Of split looks great! However, I still have some outstanding questions on the design of the instances, along with a few minor organizational notes.

src/lake/Lake/Util/Store.lean Outdated Show resolved Hide resolved
src/lake/Lake/Util/Store.lean Outdated Show resolved Hide resolved
tests/lean/MonadStore.lean Outdated Show resolved Hide resolved
src/lake/Lake/Util/Store.lean Show resolved Hide resolved
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 21, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 21, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 22, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 22, 2024
@tydeu
Copy link
Member

tydeu commented May 24, 2024

Okay, everything looks good to me! Thank you for the time you spent on this PR and explaining the situation to me! ❤️

@tydeu tydeu added this pull request to the merge queue May 24, 2024
Merged via the queue into leanprover:master with commit e5e5a4d May 24, 2024
12 checks passed
@JovanGerb JovanGerb deleted the fixMonadStore branch June 15, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants