-
Notifications
You must be signed in to change notification settings - Fork 36
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
singletons generates ill-kinded type signature for class method #358
Comments
Ugh, this turns out to be very tricky to fix. First, an explanation of why this bug occurs in the first place. When singling a class instance, we look up the type signature of each singled class method. For sMethod :: forall k (f :: k -> Type). SC f => forall (a :: k). Sing (MethodSym0 :: f a) Next, we need to figure out which types to substitute from the class head: instance SC [] where ... Here, there is (seemingly) only one type from the instance head. We this form a substitution roughly like so:
It's step (3) where things go awry. Normally, the lengths of instance SC [] where
-- Before substitution
sMethod :: forall (a :: k). Sing (MethodSym0 :: f a)
-- After substitution
sMethod :: forall (a :: []). Sing (MethodSym0 :: f a) And that's how we end up with the nonsensical results in the original post. How do we fix this problem? It's not clear at all to me. The problem is that it's not easy in general to know what |
Instead of slurping the |
I'm not sure what you mean by "look at the arguments". Can you give a more detailed example of how this would work? |
We have
Currently, you're describing that we consider (wrongly) Or have I missed something? |
Even if we actually mapped sMethod :: forall (a :: k). Sing (MethodSym0 :: [a]) Here, the kind of |
Ah, yes. Perhaps @mynguyenbmc's work on visible kind application (which includes an update to TH) will solve the problem. I believe she'll be posting her work for review soon. |
Now that I think about it, I have to wonder: why are we giving singled instance methods type signatures in the first place? As far as I can tell, they're completely unnecessary, since GHC is more than capable of inferring these types by itself. Plus, it's not like we're using them to bring type variables into scope, since that's a role that's already served by pattern signatures. As evidence for this claim, I commented out singled instance method type signatures in |
Well, it sounds like you've solved this then. I think we needed the signatures before TH could deal with pattern signatures that bound variables. |
This is indeed encouraging. However, even after removing these instance method type signatures, it turns out that we're still using the aforementioned erroneous substitution in other parts of the code, which makes me worry that there's still a bug lurking. Let me see if it's possible to flush it out. |
Welp, time for disappointing news: my plan of just ditching the instance method type signatures won't work. Here's a counterexample: {-# LANGUAGE GADTs #-}
{-# LANGUAGE InstanceSigs #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeInType #-}
{-# LANGUAGE UndecidableInstances #-}
module Bug where
import Data.Kind
import Data.Singletons.TH
$(singletons [d|
class C (f :: Type -> Type) where
method :: forall a. (f :: Type -> Type) a -> f a
instance C [] where
method x =
case x of
_ -> x
|]) Normally, this generated the following instance SC [] where
sMethod ::
forall a_a6Ev (t_a6EW :: ([] :: Type -> Type) a_a6Ev).
Sing t_a6EW -> Sing (Apply MethodSym0 t_a6EW :: [a_a6Ev])
sMethod (sX :: Sing x_a6EL)
= case sX of { _ -> sX } ::
Sing (Case_6989586621679035384_a6EN x_a6EL x_a6EL :: [a_a6Ev]) Notice that the kind variable
|
Ugh. It looks like we need to be able to bind type variables explicitly.... |
Indeed. My next thought was "well what if we just use kinds in the pattern signatures"? Something like: instance SC [] where
sMethod (sX :: Sing (x :: a))
= case sX of { _ -> sX } ::
Sing (Case x x :: [a]) Would work for that example, I think, but that only works because I agree that in order to fix this robustly, we'll likely need visible type variable patterns. I'll go ahead and park this ticket for now pending further developments on GHC's side. |
Agreed on all fronts. |
To answer my earlier question about whether the erroneous substitution was causing other bugs, the answer is currently no (but with an emphasis on "currently"). There are only two other places that use this substitution:
|
Hold on, I just thought of another approach which might bypass this issue entirely. I was so caught up in trying to remove $(singletons [d|
instance C [] where
method :: [a]
method = []
|]) Then instead of trying to look up the type of Of course, this wouldn't be a fix for the underlying bogus substitution. But it is a workaround, and a pretty solid one too. Plus, we'd be able to support another GHC language feature, which is a nice bonus. Does that sound like a reasonable approach? I have a working prototype of this feature at the moment (that I'm currently testing on my upstream code which originally triggered this issue), so if you like what you see here, then we can get it in before the final 2.5 release. |
Could I summarize your proposal as "Every instance of a poly-kinded type class must have signatures on all methods" (if we are to single it)? That's a cheap-and-cheerful way to make quick progress, and it's easy to specify and conform to. |
Previously, `singletons-th` would always attempt to generate instance signatures for singled instance methods, even if the original instance code lacks instance signatures. To do so, `singletons-th` will infer an instance signature by reifying the type of the method (or, if that cannot be found, the singled version of the method) and manually applying a substitution from class variables to instance types. This process is quite involved, and what's more, it doesn't work in all cases: * As noted in #358, inferred instance signatures can sometimes be ill-kinded. * In order to support singling examples like the ones from #581, we need type variables from class method defaults and instance methods to scope over their bodies. However, the approach that `singletons-th` used to reify the method type for the singled code will sometimes reify _different_ type variables than the ones used in the promoted code, leading to disaster. This convention of inferring the instance signature dates all the way back to commit c9beec5, and it's unclear why this choice was made. At the time of writing #358, I was convinced that inferred instance signatures were necessary to support examples like the one in #358 (comment). However, this example is only problematic due to the use of an explicit kind annotation on a promoted `case` expression, and these explicit kind annotations were removed in the fix for #547. As such, this convention no longer serves a useful purpose. This patch removes the instance signature inference code, greatly simplifying the overall process of singling instance declarations. Fixes #590.
Previously, `singletons-th` would always attempt to generate instance signatures for singled instance methods, even if the original instance code lacks instance signatures. To do so, `singletons-th` will infer an instance signature by reifying the type of the method (or, if that cannot be found, the singled version of the method) and manually applying a substitution from class variables to instance types. This process is quite involved, and what's more, it doesn't work in all cases: * As noted in #358, inferred instance signatures can sometimes be ill-kinded. * In order to support singling examples like the ones from #581, we need type variables from class method defaults and instance methods to scope over their bodies. However, the approach that `singletons-th` used to reify the method type for the singled code will sometimes reify _different_ type variables than the ones used in the promoted code, leading to disaster. This convention of inferring the instance signature dates all the way back to commit c9beec5, and it's unclear why this choice was made. At the time of writing #358, I was convinced that inferred instance signatures were necessary to support examples like the one in #358 (comment). However, this example is only problematic due to the use of an explicit kind annotation on a promoted `case` expression, and these explicit kind annotations were removed in the fix for #547. As such, this convention no longer serves a useful purpose. This patch removes the instance signature inference code, greatly simplifying the overall process of singling instance declarations. Fixes #590.
Previously, `singletons-th` would always attempt to generate instance signatures for singled instance methods, even if the original instance code lacks instance signatures. To do so, `singletons-th` will infer an instance signature by reifying the type of the method (or, if that cannot be found, the singled version of the method) and manually applying a substitution from class variables to instance types. This process is quite involved, and what's more, it doesn't work in all cases: * As noted in #358, inferred instance signatures can sometimes be ill-kinded. * In order to support singling examples like the ones from #581, we need type variables from class method defaults and instance methods to scope over their bodies. However, the approach that `singletons-th` used to reify the method type for the singled code will sometimes reify _different_ type variables than the ones used in the promoted code, leading to disaster. This convention of inferring the instance signature dates all the way back to commit c9beec5, and it's unclear why this choice was made. At the time of writing #358, I was convinced that inferred instance signatures were necessary to support examples like the one in #358 (comment). However, this example is only problematic due to the use of an explicit kind annotation on a promoted `case` expression, and these explicit kind annotations were removed in the fix for #547. As such, this convention no longer serves a useful purpose. This patch removes the instance signature inference code, greatly simplifying the overall process of singling instance declarations. Fixes #590.
The following code fails to single with
singletons-2.4.1
and HEAD:Two important things to note:
instance C []
instance being in a separate splice from the declaration forclass C f
. In other words, the bug appears to have something to do with the way thatsingletons
is reifyingSC
.f
having kindk -> Type
is also important, because changing it toType -> Type
makes the bug go away.The text was updated successfully, but these errors were encountered: