Opened 5 years ago

Last modified 14 months ago

#10087 new feature request

DefaultSignatures: error message mentions internal name

Reported by: andreas.abel Owned by:
Priority: normal Milestone:
Component: Compiler (Type checker) Version: 7.8.4
Keywords: Generics Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Other Test Case:
Blocked By: Blocking:
Related Tickets: #12854, #13755 Differential Rev(s):
Wiki Page:

Description

{-# LANGUAGE DefaultSignatures #-}

class C a where
  reflexive :: a -> Bool
  default reflexive :: Eq a => a -> Bool
  reflexive x = x == x

data D

instance C D where

-- /home/abel/play/haskell/bugs/DefaultSig.hs:10:10:
--     No instance for (Eq D) arising from a use of ‘Main.$gdmreflexive’
--     In the expression: Main.$gdmreflexive
--     In an equation for ‘reflexive’: reflexive = Main.$gdmreflexive
--     In the instance declaration for ‘C D’

Error looks odd: The user has not written $gdmreflexive in his code.

TODO Better error message.

Maybe this should just trigger a warning that method reflexive is undefined for instance D of C. Like when I remove the default method.

/home/abel/play/haskell/bugs/DefaultSig.hs:10:10: Warning:
    No explicit implementation for
      ‘reflexive’
    In the instance declaration for ‘C D’

It seems the semantics of a default signature is that each instance *must* implement this method.

Change History (14)

comment:1 Changed 5 years ago by dreixel

I agree that it's not a good idea to mention $gdmreflexive in the error message.

But I don't think the error should be replaced by a warning. If there are no default signatures we don't get any errors or warnings:

class C a where
  reflexive :: Eq a => a -> Bool
  reflexive x = x == x

data D

instance C D

So perhaps that's what we should do in this case, too.

comment:2 Changed 5 years ago by andreas.abel

I see. You get the error when using the default implementation.

test :: D -> Bool
test d = reflexive d
    No instance for (Eq D) arising from a use of ‘reflexive’
    In the expression: reflexive d
    In an equation for ‘test’: test d = reflexive d

I agree this would be the most consistent behavior.

comment:3 Changed 5 years ago by dreixel

Simon, is there a good reason for tc_default in TcInstDcls to treat DefMeth so differently from GenDefMeth?

comment:4 in reply to:  3 Changed 5 years ago by simonpj

Replying to dreixel:

Simon, is there a good reason for tc_default in TcInstDcls to treat DefMeth so differently from GenDefMeth?

No there isn't. See tc_default in tcMethods in TcInstDcls.

  • For polymorphic default methods, DefMeth, we generate typechecked HsSyn Id; see Note [Default methods in instances].
  • For generic default methods, GenDefMeth, we currently generate renamed HsSyn Name, and then feed it to the type checker.
  • For user-written methods, NoDefMeth, we obviously just typecheck what the user wrote.

I think it'd be pretty simple to instead treat GenDefMeth more like DefMeth. It might be a bit more code, but we'd get decent error messages.

Do you think you could do that? I can advise.

Simon

comment:5 Changed 4 years ago by simonpj

Keywords: Generics added

comment:6 Changed 3 years ago by RyanGlScott

Status: newinfoneeded

I'm a bit confused about the state of affairs for this ticket.

Pedro, I don't really understand what you mean when you say "So perhaps that's what we should do in this case, too." After all, this code:

{-# LANGUAGE DefaultSignatures #-}

class C a where
  reflexive :: a -> Bool
  default reflexive :: Eq a => a -> Bool
  reflexive x = x == x

data D

instance C D where

and this code:

class C a where
  reflexive :: Eq a => a -> Bool
  reflexive x = x == x

data D

instance C D

appear to be fundamentally different. In the former, reflexive defines a function that requires an Eq a constraint when the user doesn't implement it. In the latter, reflexive defines a function that presupposes that a is an instance of Eq. When viewed in this light, shouldn't the former code error and the latter code be OK?

Simon, what are GenDefMeth and NoDefMeth? I can't find anything in the source about them (save for one possibly outdated comment on NoDefMeth). Were they replaced by DefMethSpec (i.e, this) at some point? If so, are your comments about a proposed refactoring still relevant?

comment:7 Changed 3 years ago by simonpj

Clarifications. GenDefMeth an NoDefMeth are gone. Now we just have (in Class.hs):

type DefMethInfo = Maybe (Name, DefMethSpec Type)
   -- Nothing                    No default method
   -- Just ($dm, VanillaDM)      A polymorphic default method, name $dm
   -- Just ($gm, GenericDM ty)   A generic default method, name $gm, type ty
   --                              The generic dm type is *not* quantified
   --                              over the class variables; ie has the
   --                              class vaiables free

So there are still three cases, just as stated in comment:4. But the representation has changed.

comment:8 Changed 3 years ago by simonpj

The real issue is this: what error message do we *want* from the erroneous program in the Description?

There really is an error:

  • the generic default method is used in the instance because no explicit method for reflexive is given.
  • but the generic default requires (Eq a) and that is not available.

So we need something like

    No instance for (Eq D) arising from 
       the generic default method for `reflexive`
    In the instance declaration for ‘C D’

Would that be about right?

The difficulty is that for generic defaults, for the class decl we generate

$gdmreflexive :: (C a, Eq a) => a -> Bool
$gdmreflexive x = x==x

This part is fine.

For the missing method binding in the instance we generate we generate source code looking like

  reflexive = $gdmreflexive

Now we typecheck that, which gives the error message. And you can see it might be hard to generate the "right" error message.

Better perhaps to do what happens for non-generic default methods, which is to generate typechecked code directly (and emit some constraints). Compare what we do for the Nothing case of DefMethInfo in tc_default in tcMethods in TcInstDcls.

Does that make sense?

comment:9 in reply to:  8 Changed 3 years ago by RyanGlScott

Status: infoneedednew

Thanks, Simon! This comment helps tremendously in figuring out how things currently work.

Replying to simonpj:

So we need something like

    No instance for (Eq D) arising from 
       the generic default method for `reflexive`
    In the instance declaration for ‘C D’

Would that be about right?

I agree completely! I was confused because after reading Pedros' comment, I was under the impression he was implying that code should be legal. I probably misinterpreted it wildly and came to a very wrong conclusion.

Better perhaps to do what happens for non-generic default methods, which is to generate typechecked code directly (and emit some constraints). Compare what we do for the Nothing case of DefMethInfo in tc_default in tcMethods in TcInstDcls.

Does that make sense?

That seems sensible. Clearly, the mechanism we use for VanillaDM works well, because I've never seen an error message mention a name that begins with $dm :) If we modify the code that handles GenericDM to use the same tricks, that would probably make the error message much more palatable. We could also insert the phrase "the generic default method for" to make the origin of the issue clearer.

comment:10 Changed 3 years ago by dreixel

I'm not entirely sure I understand my own comment, to be honest. The current proposal for handling certainly seems fine to me.

comment:11 Changed 3 years ago by simonpj

OK, Ryan if you are prepared to have a go at this, feel free to consult me. You will have to emit some constraints, with a suitable CtOrigin.

Simon

comment:12 Changed 2 years ago by RyanGlScott

See also #13755, which deals with default implementations instead of generic default implementations.

comment:13 Changed 16 months ago by RyanGlScott

comment:14 Changed 14 months ago by Richard Eisenberg <rae@…>

In c955a51/ghc:

Remove decideKindGeneralisationPlan

TypeInType came with a new function: decideKindGeneralisationPlan.
This type-level counterpart to the term-level decideGeneralisationPlan
chose whether or not a kind should be generalized. The thinking was
that if `let` should not be generalized, then kinds shouldn't either
(under the same circumstances around -XMonoLocalBinds).

However, this is too conservative -- the situation described in the
motivation for "let should be be generalized" does not occur in types.

This commit thus removes decideKindGeneralisationPlan, always
generalizing.

One consequence is that tc_hs_sig_type_and_gen no longer calls
solveEqualities, which reports all unsolved constraints, instead
relying on the solveLocalEqualities in tcImplicitTKBndrs. An effect
of this is that reporing kind errors gets delayed more frequently.
This seems to be a net benefit in error reporting; often, alongside
a kind error, the type error is now reported (and users might find
type errors easier to understand).

Some of these errors ended up at the top level, where it was
discovered that the GlobalRdrEnv containing the definitions in the
local module was not in the TcGblEnv, and thus errors were reported
with qualified names unnecessarily. This commit rejiggers some of
the logic around captureTopConstraints accordingly.

One error message (typecheck/should_fail/T1633)
is a regression, mentioning the name of a default method. However,
that problem is already reported as #10087, its solution is far from
clear, and so I'm not addressing it here.

This commit fixes #15141. As it's an internal refactor, there is
no concrete test case for it.

Along the way, we no longer need the hsib_closed field of
HsImplicitBndrs (it was used only in decideKindGeneralisationPlan)
and so it's been removed, simplifying the datatype structure.

Along the way, I removed code in the validity checker that looks
at coercions. This isn't related to this patch, really (though
it was, at one point), but it's an improvement, so I kept it.

This updates the haddock submodule.
Note: See TracTickets for help on using tickets.