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

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

Closed
arybczak opened this issue Apr 16, 2023 · 5 comments
Closed

Comments

@arybczak
Copy link
Collaborator

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 😞

What do we do?

@adamgundry
Copy link
Member

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
Copy link
Member

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

@adamgundry
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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

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

No branches or pull requests

3 participants