well-typed / optics

Optics as an abstract interface
374 stars 24 forks source link

https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9411 seems to break us #488

Closed arybczak closed 11 months ago

arybczak commented 1 year ago

Specifically, there's an incoherent instance of AppendIndices and if it's not optimized away (and it looks like after this patch it's not), everything crumbles.

I checked the testsuite with GHC HEAD and 13 tests fail, all show presence of $dAppendIndices in Core that seem to inhibit further optimizations.

The MR in question can't be cleanly reverted anymore, but when I did this:

diff --git a/optics-core/src/Data/Maybe/Optics.hs b/optics-core/src/Data/Maybe/Optics.hs
index 47b5b7a..cc9ad42 100644
--- a/optics-core/src/Data/Maybe/Optics.hs
+++ b/optics-core/src/Data/Maybe/Optics.hs
@@ -44,7 +44,7 @@ _Just =
 -- @since 0.4.1
 infixl 9 %?
 (%?)
-  :: (AppendIndices is js ks, JoinKinds k A_Prism k', JoinKinds k' l m)
+  :: (AppendIndices is js ks, AppendIndices is NoIx is, JoinKinds k A_Prism k', JoinKinds k' l m)
   => Optic k is s t (Maybe u) (Maybe v)
   -> Optic l js u v a b
   -> Optic m ks s t a b
diff --git a/optics-core/src/Optics/Indexed/Core.hs b/optics-core/src/Optics/Indexed/Core.hs
index 7bc6409..42692e9 100644
--- a/optics-core/src/Optics/Indexed/Core.hs
+++ b/optics-core/src/Optics/Indexed/Core.hs
@@ -105,7 +105,7 @@ o %> o' = noIx o % o'
 --
 infixl 9 <%
 (<%)
-  :: (JoinKinds k l m, IxOptic l u v a b, NonEmptyIndices js)
+  :: (JoinKinds k l m, AppendIndices is NoIx is, IxOptic l u v a b, NonEmptyIndices js)
   => Optic k is s t u v
   -> Optic l js u v a b
   -> Optic m is s t a b
diff --git a/optics-core/src/Optics/Internal/Optic/TypeLevel.hs b/optics-core/src/Optics/Internal/Optic/TypeLevel.hs
index 94ed7c5..41dc6e7 100644
--- a/optics-core/src/Optics/Internal/Optic/TypeLevel.hs
+++ b/optics-core/src/Optics/Internal/Optic/TypeLevel.hs
@@ -113,8 +113,8 @@ class AppendIndices xs ys ks | xs ys -> ks where

 -- | If the second list is empty, we can pick the first list
 -- even if nothing is known about it.
-instance {-# INCOHERENT #-} xs ~ zs => AppendIndices xs '[] zs where
-  appendIndices = IxEq
+--instance {-# INCOHERENT #-} xs ~ zs => AppendIndices xs '[] zs where
+--  appendIndices = IxEq

 instance ys ~ zs => AppendIndices '[] ys zs where
   appendIndices = IxEq

All of these tests pass again, so I'm pretty sure it's the problem.

For the record, I'm not sure if the above diff is "safe" and I consider the GHC MR problematic.

GPlateInner also has an incoherent instance. This one can't be removed :disappointed:

What do we do?

adamgundry commented 1 year ago

Thanks for spotting this promptly! I brought up this worry speculatively at the time, but didn't get much traction. But if we have concrete evidence that the change regresses performance, let's file a GHC ticket asking for the change to be rethought (guarding it behind a flag would make sense to me).

adamgundry commented 1 year ago

I've raised this upstream as https://gitlab.haskell.org/ghc/ghc/-/issues/23287

adamgundry commented 1 year ago

I believe this should now be resolved upstream as the problematic change is now guarded by a flag, -fno-specialise-incoherents. But it would be good to double-check with 9.8.1.

ysangkok commented 11 months ago

it would be good to double-check with 9.8.1.

@adamgundry does the CI suite passing on #499 mean that this is confirmed to not be an issue?

adamgundry commented 11 months ago

Yes, with the GHC flag in place and CI passing I think there's nothing more to do here.