Ticket #5858 (closed bug: fixed)

Opened 16 months ago

Last modified 15 months ago

type inference of an OverloadedString for a class instance with type parameters

Reported by: GregWeber Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.4.1
Keywords: Cc: greg@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: typecheck/should_fail/T5858 Blocked By:
Blocking: Related Tickets:

Description (last modified by simonpj) (diff)

We have some code in Yesod:

class RedirectUrl master a where
    -- | Converts the value to the URL and a list of query-string parameters.
    toTextUrl :: a -> GHandler sub master Text

instance t ~ Text => RedirectUrl master (Route master, [(t, t)]) where
    toTextUrl (u, ps) = do
        r <- getUrlRenderParams
        return $ r u ps

When I use it in my application, I am required to give an annotation to the overloaded strings. If I don't:

redirect $ (SearchR, [("foo", "bar")])

I end up with this error message:

    No instance for (RedirectUrl Search (Route Search, [(t0, t1)]))
      arising from a use of `redirect'
    Possible fix:
      add an instance declaration for
      (RedirectUrl Search (Route Search, [(t0, t1)]))
    In the expression: redirect
    In the expression: redirect $ (SearchR, [("foo", "bar")])
    In an equation for `getFoodsr23R':
        getFoodsr23R foodId = redirect $ (SearchR, [("foo", "bar")])

I would be ok with having to type annotate if instead of the compiler suggesting I declare an entire new instance the compiler instead suggested that I annotate my overloaded strings.

However, in trying to reproduce this program in a simpler setting, it seems to normally perform the OverloadedStrings inference without any issue. This works just fine:

{-# LANGUAGE OverloadedStrings, NoMonomorphismRestriction, FlexibleInstances, GADTs #-}
module InferOverloaded where
import Data.Text

class InferOverloaded a where
  infer :: a -> a

data Data = Data String

instance t ~ Text => InferOverloaded (Data, [(t,t)]) where
  infer = id

foo = infer (Data "data", [("overloaded", "strings")])

Change History

Changed 16 months ago by GregWeber

  • cc greg@… added

I am using 7.4 Haven't tried 7.0 yet.

Changed 16 months ago by GregWeber

I see the same problem on 7.0

Changed 16 months ago by simonpj

  • difficulty set to Unknown
  • description modified (diff)

Here's why it your last example works: GHC infers this type for foo:

    foo :: forall t t1.
           (Data.String.IsString t1, Data.String.IsString t,
            InferOverloaded (Data, [(t, t1)])) =>
           (Data, [(t, t1)])

Notice that

  • There is nothing to force "overloaded" and "strings" to have the same type, so they get types t, t1 respectively.
  • Hence the instance does not get used
  • GHC instead defers solving the constraint to the call site, in the hope that it may by then be clearer what t, t1 are.

One way to get the un-annotated behaviour you want might be this:

instance (t1 ~ Text, t2 ~ Text) => InferOverloaded (Data, [(t1,t2)] where
  infer = id

About your suggestion about error messages, I suppose that in the situation where giving more type information at the call site would pick a valid instance, we could suggest that. I can see where to do this. Could you supply a small test case that exhibits the behaviour? (Not depending on Data.Text.)

Thanks

Simon

Changed 16 months ago by GregWeber

Thanks, your suggestion did fix the inference. Here is a small test program

{-# LANGUAGE FlexibleInstances, GADTs #-}
module InferOverloaded where

class InferOverloaded a where
  infer :: a -> String

-- instance (t1 ~ String, t2 ~ String) => InferOverloaded (t1,t2) where
instance (t1 ~ String) => InferOverloaded (t1,t1) where
  infer = show . fst
  
foo = infer ([],[])

Changed 15 months ago by simonpj

How's this? Instead of the old message

T5858.hs:11:7:
    No instance for (InferOverloaded ([a0], [a1]))
      arising from a use of `infer'
    The type variables `a0', `a1' are ambiguous
    Possible fix: add a type signature that fixes these type variable(s)
    Possible fix:
      add an instance declaration for (InferOverloaded ([a0], [a1]))
    In the expression: infer ([], [])
    In an equation for `foo': foo = infer ([], [])

we now get this:

T5858.hs:11:7:
    Cannot resolve the instance for InferOverloaded ([a0], [a1])
      arising from a use of `infer'
    There is a potential instance available:
      instance t1 ~ String => InferOverloaded (t1, t1)
        -- Defined at T5858.hs:8:10
    Possible fix: add a type signature to instantiate `a0, a1'
    In the expression: infer ([], [])
    In an equation for `foo': foo = infer ([], [])

Is that better?

Simon

Changed 15 months ago by GregWeber

T5858.hs:11:7:
    Cannot resolve the instance for InferOverloaded ([a0], [a1])
      arising from a use of `infer'
    Possible fix:
      add an instance declaration for (InferOverloaded ([a0], [a1]))
    Possible fix: add a type signature to instantiate `a0, a1'
      There is a potential instance available:
        instance t1 ~ String => InferOverloaded (t1, t1)
          Defined at T5858.hs:8:10
   
    In the expression: infer ([], [])
    In an equation for `foo': foo = infer ([], [])

I think the above would be idea: mention both fixes as before.

The expansion of the suggestion here is great. My favorite aspect of 7.4 is how it suggests alternatives functions when you mis-spell one. These help me, but I think better messages are perhaps the biggest improvement for new users. If we wanted to go for the gold on this we would actually spell out the fixed code in the possible fix:

infer ([]::String, []::String)

But I could definitely imagine that this might be problematic in certain circumstances.

Changed 15 months ago by simonpj@…

commit 7b0f95cfe4ee97a7aafee769e5150b00bd6806ad

Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Fri Mar 9 13:27:52 2012 +0000

    Improve reporting of type-class errors
    
    Inspired by suggestions on Trac #5858, the errors now
    mention "potential instances".  Lots of refactoring
    as usual, but localised.

 compiler/typecheck/TcErrors.lhs |  209 +++++++++++++++++++++------------------
 compiler/types/InstEnv.lhs      |   17 ++--
 2 files changed, 124 insertions(+), 102 deletions(-)

Changed 15 months ago by simonpj

  • status changed from new to closed
  • testcase set to typecheck/should_fail/T5858
  • resolution set to fixed

I've added a test too. Thanks for the idea.

Note: See TracTickets for help on using tickets.