Opened 2 years ago

Closed 2 years ago

Last modified 17 months ago

#9939 closed feature request (fixed)

Warn for duplicate superclass constraints

Reported by: crockeea Owned by:
Priority: normal Milestone: 8.0.1
Component: Compiler Version: 7.8.3
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: typecheck/should_compile/T9939
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

With the following code, GHC warns that there are duplicate constraints:

{-# LANGUAGE FlexibleInstances, UndecidableInstances #-}

module Foo where

class Foo a where
  foo :: Int -> a

instance (Integral a, Integral a) => Foo a where
  foo x = (fromIntegral x) `div` 2

However, when writing the code, I might start with:

{-# LANGUAGE FlexibleInstances, UndecidableInstances #-}

module Foo where

class Foo a where
  foo :: Int -> a

instance Foo a where
  foo x = (fromIntegral x) `div` 2

without any constraints on the instance. GHC complains that it needs Num a and Integral a, but of course Num is implied by Integral. I'm not asking that GHC figure this out on its own and only request the strongest constraint necessary. Rather, I'm suggesting that if I followed GHC's suggestion and wrote

{-# LANGUAGE FlexibleInstances, UndecidableInstances #-}

module Foo where

class Foo a where
  foo :: Int -> a

instance (Num a, Integral a) => Foo a where
  foo x = (fromIntegral x) `div` 2

then GHC should warn

Duplicate constraint(s): Num a
 In the context: (Num a, Integral a)
 (Num a) is implied by (Integral a)

or something similar.

The motivation for this feature request is that in large instances/programs, it is difficult for a human to keep track of superclasses. In large instances, GHC tends to request "weak" constraints first (say Num), then ask for progressively stronger constraints (say Integral). Again, I'm not suggesting that behavior should change. However, it tends to lead to instances that look like (Num a, Real a, RealFrac a, RealFloat a) => ... if by chance I happened to use methods from each class.

It seems fairly simple for GHC to look at each constraint for an instance (or function), trace back up the class hierarchy to get a set of all implied constraints, and then warn if one set is a subset of another.

Change History (5)

comment:1 Changed 2 years ago by goldfire

Another example I hit recently. Language.Haskell.TH.Syntax used to have

class (Applicative m, Monad m) => Quasi m where ...

When I saw this recently, I realized the Applicative m constraint is now redundant and so I removed it. However, it would have been helpful if GHC had issued a warning.

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

In 32973bf3c2f6fe00e01b44a63ac1904080466938/ghc:

Major patch to add -fwarn-redundant-constraints

The idea was promted by Trac #9939, but it was Christmas, so I did
some recreational programming that went much further.

The idea is to warn when a constraint in a user-supplied context is
redundant.  Everything is described in detail in
  Note [Tracking redundant constraints]
in TcSimplify.

Main changes:

 * The new ic_status field in an implication, of type ImplicStatus.
   It replaces ic_insol, and includes information about redundant
   constraints.

 * New function TcSimplify.setImplicationStatus sets the ic_status.

 * TcSigInfo has sig_report_redundant field to say whenther a
   redundant constraint should be reported; and similarly
   the FunSigCtxt constructor of UserTypeCtxt

 * EvBinds has a field eb_is_given, to record whether it is a given
   or wanted binding. Some consequential chagnes to creating an evidence
   binding (so that we record whether it is given or wanted).

 * AbsBinds field abs_ev_binds is now a *list* of TcEvBiinds;
   see Note [Typechecking plan for instance declarations] in
   TcInstDcls

 * Some significant changes to the type checking of instance
   declarations; Note [Typechecking plan for instance declarations]
   in TcInstDcls.

 * I found that TcErrors.relevantBindings was failing to zonk the
   origin of the constraint it was looking at, and hence failing to
   find some relevant bindings.  Easy to fix, and orthogonal to
   everything else, but hard to disentangle.

Some minor refactorig:

 * TcMType.newSimpleWanteds moves to Inst, renamed as newWanteds

 * TcClassDcl and TcInstDcls now have their own code for typechecking
   a method body, rather than sharing a single function. The shared
   function (ws TcClassDcl.tcInstanceMethodBody) didn't have much code
   and the differences were growing confusing.

 * Add new function TcRnMonad.pushLevelAndCaptureConstraints, and
   use it

 * Add new function Bag.catBagMaybes, and use it in TcSimplify

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

comment:4 Changed 2 years ago by simonpj

Resolution: fixed
Status: newclosed
Test Case: typecheck/should_compile/T9939

Well, I went way overboard on this, but it was fun! Thanks for the suggestion. It showed up masses of unnecessary constraints in the compiler itself, and in many libraries, as you can see from

commit c409b6f30373535b6eed92e55d4695688d32be9e
Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Tue Jan 6 13:46:35 2015 +0000

    Remove redundant constraints from libraries, discovered by -fwarn-redundant-constraints

 libraries/array                   |    2 +-
 libraries/base/Data/Data.hs       |    8 +++----
 libraries/base/Data/Foldable.hs   |    4 ++--
 libraries/base/GHC/Arr.hs         |   46 ++++++++++++++++++-------------------
 libraries/base/GHC/Event/Array.hs |    4 ++--
 libraries/base/GHC/IOArray.hs     |    4 ++--
 libraries/base/GHC/Real.hs        |    6 ++---
 libraries/base/Text/Printf.hs     |    2 +-
 libraries/deepseq                 |    2 +-
 libraries/hoopl                   |    2 +-
 libraries/parallel                |    2 +-
 mk/validate-settings.mk           |    3 +++

and

commit 39337a6d97c853a88fa61d6b12a04eb8c2e5984f
Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Mon Jan 5 16:57:01 2015 +0000

    Remove redundant constraints in the compiler itself, found by -fwarn-redundant-constraints

 compiler/basicTypes/Name.hs                     |    3 ++-
 compiler/cmm/CmmExpr.hs                         |    8 ++++----
 compiler/cmm/Hoopl/Dataflow.hs                  |    2 +-
 compiler/coreSyn/TrieMap.hs                     |    4 ++--
 compiler/deSugar/MatchLit.hs                    |    2 +-
 compiler/ghci/ByteCodeItbls.hs                  |    6 ++++--
 compiler/ghci/Linker.hs                         |    2 +-
 compiler/hsSyn/HsDecls.hs                       |    8 +++-----
 compiler/hsSyn/HsExpr.hs                        |    8 ++++----
 compiler/main/CmdLineParser.hs                  |    2 +-
 compiler/main/GHC.hs                            |   10 +++++++---
 compiler/main/GhcMonad.hs                       |   17 ++++++++++++----
 compiler/main/InteractiveEval.hs                |    3 +--
 compiler/nativeGen/RegAlloc/Graph/SpillClean.hs |   11 ++++-------
 compiler/nativeGen/RegAlloc/Linear/Main.hs      |    4 ++--
 compiler/nativeGen/SPARC/Base.hs                |    2 +-
 compiler/typecheck/TcRnMonad.hs                 |    3 ++-
 compiler/types/CoAxiom.hs                       |    2 +-
 compiler/utils/Binary.hs                        |    2 +-
 compiler/utils/GraphColor.hs                    |    6 +++---
 compiler/utils/GraphOps.hs                      |   24 ++++++++++-------------
 compiler/utils/GraphPpr.hs                      |    9 ++++-----
 compiler/utils/Maybes.hs                        |    4 ++++
 compiler/utils/Serialized.hs                    |    4 ++--
 compiler/utils/UniqSet.hs                       |    2 +-

comment:5 Changed 17 months ago by thomie

Milestone: 8.0.1
Note: See TracTickets for help on using tickets.