Instance diamond in `OreLocalization`

Stream: mathlib4

[Aaron Liu]

While trying to make a term of type ℝ →+*o Surreal:

import Mathlib

example :
    -- comes from `OreLocalization.hsmul`, depends on `SMul ℤ ℤ`
    @instHSMul ℤ (Localization (Submonoid.powers (2 : ℤ))) OreLocalization.instSMulOfIsScalarTower =
    -- comes from `OreLocalization.zsmul`, depends on `AddGroup ℤ`
    @instHSMul ℤ (Localization.Away (2 : ℤ)) SubNegMonoid.toZSMul := by
  -- helpme
  sorry

[Eric Wieser]

I'm not sure if this is a reasonable statement; are the types of the LHS and RHS reducibly defeq?

[Eric Wieser]

(that is, can you do the same test again on the second arguments to instHSMul, and test if with_reducible rfl is happy?)

[Aaron Liu]

import Mathlib

example : (Localization (Submonoid.powers (2 : ℤ))) = (Localization.Away (2 : ℤ)) := by
  with_reducible rfl

[Aaron Liu]

It works

[Kevin Buzzard]

I don't think this has got anything to do with the difference between Localization.Away and Localization: both docs#OreLocalization.instSMulOfIsScalarTower and docs#OreLocalization.instAddGroupOreLocalization work for general localization by submonoids. There's a warning in the code that the former instance might cause diamonds in an obscure situation but ths seems far less obscure.

[Yaël Dillies]

This diamond is known, yes. @Andrew Yang and I hit it in #21021

[Kevin Buzzard]

Maybe one could try weakening the definition of docs#OreLocalization.zsmul, adding an assumption [SMul ℤ R] and defining it not as zsmulRec but as OreLocalization.hsmul. This would reduce the scope of AddCommGroup X[S⁻¹] a little (requiring that S is a submonoid of basically a ring rather than just a monoid) but do we care in practice?

#21344 let's see if it compiles at least.

[Kevin Buzzard]

Hmm, it compiles (locally) but doesn't seem fix the diamond so I must have misanalysed something :-/

[Yaël Dillies]

Sorry, was interrupted. Let me explain.

[Kevin Buzzard]

Oh OK on that branch this works:

import Mathlib

example :
    -- comes from `OreLocalization.hsmul`, depends on `SMul ℤ ℤ`
    @instHSMul ℤ (Localization (Submonoid.powers (2 : ℤ))) OreLocalization.instSMulOfIsScalarTower =
    -- comes from `OreLocalization.zsmul`, depends on `AddGroup ℤ`
    @instHSMul ℤ (Localization.Away (2 : ℤ)) SubNegMonoid.toZSMul := by
  unfold instHSMul SMul.smul SubNegMonoid.toZSMul SubNegMonoid.zsmul
  unfold OreLocalization.instSMulOfIsScalarTower
  unfold AddGroup.toSubNegMonoid OreLocalization.instAddGroupOreLocalization
  dsimp only
  unfold OreLocalization.zsmul
  rfl

so it's now a reducibility issue (but I've now run out of time to think about this, unfortunately)

[Andrew Yang]

There is docs#OreLocalization.zsmul_eq_zsmul to show that they are at least equal.

[Andrew Yang]

And I’m not sure if we don’t need localization of modules of monoids.

[Kevin Buzzard]

We might need it in the future but apparently we don't need it yet in the non-commutative case.

[Kevin Buzzard]

Ok so #21344 now has

import Mathlib.RingTheory.OreLocalization.Ring
import Mathlib.GroupTheory.MonoidLocalization.Basic

example :
    -- comes from `OreLocalization.hsmul`, depends on `SMul ℤ ℤ`
    @instHSMul ℤ (Localization (Submonoid.powers (2 : ℤ))) OreLocalization.instSMulOfIsScalarTower =
    -- comes from `OreLocalization.zsmul`, depends on `AddGroup ℤ`
    @instHSMul ℤ (Localization (Submonoid.powers (2 : ℤ))) SubNegMonoid.toZSMul := by
  with_reducible_and_instances rfl

so the diamond is gone (and I assume will remain gone after import Mathlib) but in order to deal with Andrew's concern I've also added back the SMul instance without assuming a Z-action on R (so it works for monoids) but with low priority, so it will be found only if R isn't a ring, which should hopefully be enough to deal with all the issues here.

There are some sorrys which I've left as an exercise :-)

[Andrew Yang]

I don't think this low-priority trick works, because lemmas about groups will still find the Group instance which contains the bad zsmul instance.

[Andrew Yang]

Maybe it will work if [SMul ℤ X[S⁻¹]] is an assumption to AddGroup X[S⁻¹], but I'm not sure if TC synthesis will like it.

[Xavier Roblot]

I am hitting this diamond while working with FractionRing. But, Kevin's PR #21344 does fix the issue for me. So even if it is not a perfect solution, it seems to be a step in the right direction. If I adopt the PR and complete it, is there a chance it will be merged (or do I have to look for another solution).

[Xavier Roblot]

I see that the conversation continued here and it seems that #21344 is not the right solution. I'll see if using attribute [-instance] AddCommGroup.toIntModule works for me...

[Kevin Buzzard]

@Andrew Yang always understood this issue much better than me.

[Aaron Liu]

what's the problem again

[Aaron Liu]

oh I get it now

[Kevin Buzzard]

You found the diamond IIRC!

[Aaron Liu]

it was a while ago I forgot

[Aaron Liu]

I might try thinking of a solution later

[Xavier Roblot]

attribute [-instance] AddCommGroup.toIntModule did not work so I ended up using convert and resolved the diamond by hand thanks to docs#OreLocalization.zsmul_eq_zsmul

[Eric Wieser]

I think I had a fix to this at Big Proof, but Andrew suggested we do a different refactor first, then I suggested doing a different refactor, then Andrew pushed back and I agreed, then I ran out of time to review his compromise

[Kevin Buzzard]

It would be nice to have some more details of these ideas available. Seems like at least three people have independently run into this diamond now.