Opened 5 years ago

Closed 3 years ago

Last modified 2 years ago

#7854 closed bug (fixed)

Constrained method type accepted in Haskell 98 mode

Reported by: refold Owned by: thomie
Priority: normal Milestone: 8.0.1
Component: Compiler (Type checker) Version: 7.6.3
Keywords: newcomer Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: module/mod39
Blocked By: Blocking:
Related Tickets: #10118, #10119 Differential Rev(s): Phab:D688
Wiki Page:

Description

If I understand this paragraph of the GHC manual correctly, the following code should be rejected in Haskell 98 mode:

{-# LANGUAGE Haskell98 #-}

module Main where

class Compare a where
  comp :: Eq a => a -> a -> Bool

However, it compiles fine for me both with 7.4.2 and 7.6.1.

Change History (25)

comment:1 Changed 5 years ago by refold

Also accepted by 7.0.4 and 7.2.1, but rejected by 6.12.1. However, it complains about -XFlexibleContexts, not -XConstrainedClassMethods:

[1 of 1] Compiling Main             ( Test.hs, interpreted )

Test.hs:3:0:
    All of the type variables in the constraint `Eq a'
    are already in scope (at least one must be universally quantified here)
        (Use -XFlexibleContexts to lift this restriction)
    When checking the class method: comp :: (Eq a) => a -> a -> Bool
    In the class declaration for `Compare'
Failed, modules loaded: none.

comment:2 Changed 5 years ago by refold

Interestingly, the example from the manual seems to be always excepted if MultiParamTypeClasses are enabled:

  class Seq s a where
    fromList :: [a] -> s a
    elem     :: Eq a => a -> s a -> Bool

comment:3 in reply to:  2 Changed 5 years ago by refold

Replying to refold:

seems to be always excepted

s/excepted/accepted/

comment:4 Changed 5 years ago by simonpj

difficulty: Unknown

You are absolutely right. See the H98 report http://www.haskell.org/onlinereport/decls.html. In 4.3.1. it says "the cxi may not constrain u", where u is the class variable, and cx is the context of a class method signature.

But (a) it's a bit fiddly to fix, (b) it's not clear what exactly it means for multi-parameter type classes, and more seriously it might break some existing programs which are inadvertently straying from H98 definition.

So I'm rather inclined to let sleeping dogs lie.

Simon

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

Resolution: wontfix
Status: newclosed

... it might break some existing programs which are inadvertently straying from H98 definition.

So I'm rather inclined to let sleeping dogs lie.

Closing as wontfix.

comment:6 Changed 3 years ago by monoidal

Resolution: wontfix
Status: closednew

Sorry for reopening, but I think the situation can be improved, currently -XConstrainedClassMethods seems to do nothing and the documentation is misleading. I would support deprecating and eventually removing this pragma, and adding a notice to "Known bugs and infelicities" section.

comment:7 Changed 3 years ago by simonpj

I agree with monoidal. Would someone like to do that?

Simon

comment:8 Changed 3 years ago by simonpj

Keywords: newcomer added

comment:9 Changed 3 years ago by thomie

Component: CompilerCompiler (Type checker)
Milestone: 7.12.1
Owner: set to thomie

More bugs. GHC only rejects the following program when -XConstrainedClassMethods is turned on. It should however always reject it, because op doesn't mention any type variables of Foo.

module ShouldFail where
class Foo a where
  op :: Eq a => Int

I'm working on this.

comment:10 Changed 3 years ago by thomie

Differential Rev(s): Phab:D688
Status: newpatch
Test Case: typechecker/should_fail/T7854

Patches are ready for review in Phab:D686, Phab:D687 and Phab:D688.

Instead of deprecating ConstrainedClassMethods, I have tried fixing it instead. Phab:D688 turns on ConstrainedClassMethods for all modes (Haskell98, Haskell2010 and default), to not break any programs. I added a section to "Known bugs and infelicities".

Existing tests tcfail149 and tcfail150 have been reenabled, and I added T7854 for the MultiParamTypeClasses case.

comment:11 Changed 3 years ago by simonpj

Thomie, thanks for working on this. Inspired by your patches I can see a simpler way to achieve the same goal, so I'll do that.

Specifically I plan to do this:

  • Fix the original bug simply by always doing a checkValidType on the entire selector-id type. That includes an ambiguity check which is precisely what we need.
  • Implement the check when ConstrainedClassMethods is off as a special check that does nothing else.
  • As comment:4 acknowledges, GHC has not been making the claimed check for ages. Rather than making ConstrainedClassMethods on by default (as you propose), I plan to make it implied by MultiParamTypeClasses, which means that we are out of Haskell-98 land anyway. This means that some programs may break, but (a) the error message suggests adding ConstrainedClassMethods (b) the fix is easy. If necessary (e.g. on a Stackage run) we can make it on-by-default in 7.10 to ease transition.

Commit coming. Thanks for unblocking this.

Simon

comment:12 Changed 3 years ago by Simon Peyton Jones <simonpj@…>

In f66e0e695b0377c469fbe877d4850fc0ebca2010/ghc:

A raft of small changes associated with -XConstrainedClassMethods

See Trac #7854.  Specifically:

* Major clean up and simplification of check_op in checkValidClass;
  specifically
     - use checkValidType on the entire method-selector type to detect
       ambiguity
     - put a specific test for -XConstrainedClassMethods

* Make -XConstrainedClassMethods be implied by -XMultiParamTypeClasses
  (a bit ad-hoc but see #7854), and document in the user manual.

* Do the checkAmbiguity test just once in TcValidity.checkValidType,
  rather than repeatedly at every level. See Note [When to call checkAmbiguity]

* Add -XAllowAmbiguousTypes in GHC.IP, since 'ip' really is ambiguous.
  (It's a rather magic function.)

* Improve location info for check_op in checkValidClass

* Update quite a few tests, which had genuinely-ambiguous class
  method signatures.  Some I fixed by making them unambiguous; some
  by adding -XAllowAmbiguousTypes

comment:13 Changed 3 years ago by simonpj

Resolution: fixed
Status: patchclosed
Test Case: typechecker/should_fail/T7854module/mod39

OK done. I suggest we don't push this to 7.10 in case it breaks things.

Simon

comment:14 Changed 3 years ago by Simon Peyton Jones <simonpj@…>

In 3aa2519ec29156f57a862a033bc7a902b742a2e0/ghc:

Check for equality before deferring

This one was a bit of a surprise. In fixing Trac #7854, I moved
the checkAmbiguity tests to checkValidType. That meant it happened
even for monotypes, and that turned out to be very expensive in
T9872a, for reasons described in this (new) Note in TcUnify:

    Note [Check for equality before deferring]
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Particularly in ambiguity checks we can get equalities like (ty ~ ty).
    If ty involves a type function we may defer, which isn't very sensible.
    An egregious example of this was in test T9872a, which has a type signature
           Proxy :: Proxy (Solutions Cubes)
    Doing the ambiguity check on this signature generates the equality
       Solutions Cubes ~ Solutions Cubes
    and currently the constraint solver normalises both sides at vast cost.
    This little short-cut in 'defer' helps quite a bit.

I fixed the problem with a quick equality test, but it feels like an ad-hoc
solution; I think we might want to do something in the constraint solver too.

(The problem was there all along, just more hidden.)

comment:15 Changed 3 years ago by Simon Peyton Jones <simonpj@…>

In 9dc0d63cd7231392320e4afcfe78102801126d34/ghc:

Three other test updates following the fix to Trac #7854

comment:16 Changed 3 years ago by ekmett

FWIW- This started breaking some pre-existing code:

https://hackage.haskell.org/package/semigroupoids-1.2.6/docs/src/Data-Functor-Alt.html#some

Would having FlexibleContexts imply ConstrainedClassMethods as well also be too much to ask?

Otherwise I'm going to have to start putting CPP around the set of directives for the module.

comment:17 in reply to:  16 Changed 3 years ago by hvr

Replying to ekmett:

Otherwise I'm going to have to start putting CPP around the set of directives for the module.

Why CPP? Afaik, {-# LANGUAGE ConstrainedClassMethods #-} is supported since GHC 7.0.1 ...

comment:18 Changed 3 years ago by simonpj

Herbert's point is a good one.

But it would not be impossible to make FlexibleContext also imply ConstrainedClassMethods, if that was vastly better.

Simon

comment:19 Changed 3 years ago by ekmett

It has been around for a long time, but I know at least a few of us were rather startled when we started getting error messages about it and every occurrence I've found so far is a module where I have FlexibleContexts already enabled.

comment:20 Changed 3 years ago by simonpj

Well, bugs (and this was a bug) are like that!

It's not so hard, and it is truthful, to add ConstrainedClassMethods to those libraries. But if the CLC advises, we can also make FlexibleContexts imply it.

Remember this is not in 7.10 so libraries have time to adapt.

Simon

comment:21 Changed 3 years ago by ekmett

That was me speaking as me, not in any official capacity. =) I had honestly thought FlexibleContexts _was_ the extension for this sort of thing until now.

Putting on my CLC hat for a bit, the main reason I can see why it'd be nice to have FlexibleContexts "just work" is it means we don't have to make the hackage trustees go back and retrofit all sorts of old versions of packages with base upper bounds wherever they find themselves encountering this issue.

But mostly that rises to the left of "nice to have" not really "have to have".

comment:22 Changed 3 years ago by simonpj

Two thoughts

  • I'm relaxed about what we choose here. It really matters little; and it's been a long-standing bug in GHC.
  • You do have a point: that FlexibleContexts sounds as if it has to do with type-signature contexts (and it does); and so does ConstrainedClassMethods. So perhaps it would be more intuitive to make FlexibleContexts imply ConstrainedClassMethods than MultiParamTypeClasses?

But I bet there are packages that use MultiParamTypeClasses but not FlexibleContexts. So

  • Perhaps we should try making FlexibleContexts imply ConstrainedClassMethods, but not MultiParamTypeClasses, just to see what happens.
  • If it's painful, then maybe the path of least resistance is to make both imply ConstrainedClassMethods.

Any views from others? Would someone like to try the experiment?

Simon

comment:23 Changed 2 years ago by Sergei Trofimovich <siarheit@…>

In e4918034896948642718f15906f5b379b98f68cf/ghc:

Amend tcrun024, tcrun025 after Trac #7854 fix

Signed-off-by: Sergei Trofimovich <siarheit@google.com>

comment:24 Changed 2 years ago by Sergei Trofimovich <siarheit@…>

In 7c2293a07fb057e492278f46e8fde48b69d178a3/ghc:

Amend tcrun037 after Trac #7854 fix

Signed-off-by: Sergei Trofimovich <siarheit@google.com>

comment:25 Changed 2 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

Note: See TracTickets for help on using tickets.