Be less aggressive about fragile-context warrnings
authorSimon Peyton Jones <simonpj@microsoft.com>
Thu, 6 Apr 2017 11:27:43 +0000 (12:27 +0100)
committerSimon Peyton Jones <simonpj@microsoft.com>
Thu, 6 Apr 2017 11:34:01 +0000 (12:34 +0100)
In the implementation of WarnSimplifiableClassConstraints, be
less aggressive about reporting a problem. We were complaining
about a "fragile" case that in fact was not fragile.

See Note [Simplifiable given constraints] in TcValidity.

This fixes Trac #13526.

compiler/typecheck/TcValidity.hs
testsuite/tests/partial-sigs/should_compile/SomethingShowable.stderr
testsuite/tests/typecheck/should_compile/T13526.hs [new file with mode: 0644]
testsuite/tests/typecheck/should_compile/T13526.stderr [new file with mode: 0644]
testsuite/tests/typecheck/should_compile/all.T

index 3023dfe..c28c21d 100644 (file)
@@ -41,7 +41,7 @@ import HsSyn            -- HsType
 import TcRnMonad        -- TcType, amongst others
 import TcEnv       ( tcGetInstEnvs )
 import FunDeps
-import InstEnv     ( ClsInst, lookupInstEnv, isOverlappable )
+import InstEnv     ( InstMatch, lookupInstEnv )
 import FamInstEnv  ( isDominatedBy, injectiveBranches,
                      InjectivityCheckResult(..) )
 import FamInst     ( makeInjectivityErrors )
@@ -810,7 +810,8 @@ check_class_pred env dflags ctxt pred cls tys
 
   | otherwise
   = do { check_arity
-       ; check_simplifiable_class_constraint
+       ; warn_simp <- woptM Opt_WarnSimplifiableClassConstraints
+       ; when warn_simp check_simplifiable_class_constraint
        ; checkTcM arg_tys_ok (predTyVarErr env pred) }
   where
     check_arity = checkTc (classArity cls == length tys)
@@ -833,25 +834,22 @@ check_class_pred env dflags ctxt pred cls tys
        | DataTyCtxt {} <- ctxt   -- Don't do this check for the "stupid theta"
        = return ()               -- of a data type declaration
        | otherwise
-       = do { instEnvs <- tcGetInstEnvs
-            ; let (matches, _, _) = lookupInstEnv False instEnvs cls tys
-                  bad_matches = [ inst | (inst,_) <- matches
-                                       , not (isOverlappable inst) ]
-            ; warnIf (Reason Opt_WarnSimplifiableClassConstraints)
-                     (not (null bad_matches))
-                     (simplifiable_constraint_warn bad_matches) }
-
-    simplifiable_constraint_warn :: [ClsInst] -> SDoc
-    simplifiable_constraint_warn (match : _)
+       = do { envs <- tcGetInstEnvs
+            ; case lookupInstEnv False envs cls tys of
+                 ([m], [], _) -> addWarnTc (Reason Opt_WarnSimplifiableClassConstraints)
+                                           (simplifiable_constraint_warn m)
+                 _ -> return () }
+
+    simplifiable_constraint_warn :: InstMatch -> SDoc
+    simplifiable_constraint_warn (match, _)
      = vcat [ hang (text "The constraint" <+> quotes (ppr (tidyType env pred)))
                  2 (text "matches an instance declaration")
             , ppr match
             , hang (text "This makes type inference for inner bindings fragile;")
                  2 (text "either use MonoLocalBinds, or simplify it using the instance") ]
-    simplifiable_constraint_warn [] = pprPanic "check_class_pred" (ppr pred)
 
 {- Note [Simplifiable given constraints]
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 A type signature like
    f :: Eq [(a,b)] => a -> b
 is very fragile, for reasons described at length in TcInteract
@@ -862,9 +860,27 @@ fragility. But if we /infer/ the type of a local let-binding, things
 can go wrong (Trac #11948 is an example, discussed in the Note).
 
 So this warning is switched on only if we have NoMonoLocalBinds; in
-that case the warning discourages uses from writing simplifiable class
-constraints, at least unless the top-level instance is explicitly
-declared as OVERLAPPABLE.
+that case the warning discourages users from writing simplifiable
+class constraints.
+
+The warning only fires if the constraint in the signature
+matches the top-level instances in only one way, and with no
+unifiers -- that is, under the same circumstances that
+TcInteract.matchInstEnv fires an interaction with the top
+level instances.  For example (Trac #13526), consider
+
+  instance {-# OVERLAPPABLE #-} Eq (T a) where ...
+  instance                   Eq (T Char) where ..
+  f :: Eq (T a) => ...
+
+We don't want to complain about this, even though the context
+(Eq (T a)) matches an instance, because the user may be
+deliberately deferring the choice so that the Eq (T Char)
+has a chance to fire when 'f' is called.  And the fragility
+only matters when there's a risk that the instance might
+fire instead of the local 'given'; and there is no such
+risk in this case.  Just use the same rules as for instance
+firing!
 -}
 
 -------------------------
index 9f0ea1f..ca06301 100644 (file)
@@ -7,7 +7,9 @@ Dependent packages: [base-4.10.0.0, ghc-prim-0.5.0.0,
                      integer-gmp-1.0.0.1]
 
 SomethingShowable.hs:5:1: warning: [-Wsimplifiable-class-constraints (in -Wdefault)]
-    The constraint ‘Show Bool’ matches an instance declaration
-    instance Show Bool -- Defined in ‘GHC.Show’
-    This makes type inference for inner bindings fragile;
-      either use MonoLocalBinds, or simplify it using the instance
+    • The constraint ‘Show Bool’ matches an instance declaration
+      instance Show Bool -- Defined in ‘GHC.Show’
+      This makes type inference for inner bindings fragile;
+        either use MonoLocalBinds, or simplify it using the instance
+    • When checking the inferred type
+        somethingShowable :: Show Bool => Bool -> String
diff --git a/testsuite/tests/typecheck/should_compile/T13526.hs b/testsuite/tests/typecheck/should_compile/T13526.hs
new file mode 100644 (file)
index 0000000..efe32bd
--- /dev/null
@@ -0,0 +1,22 @@
+{-# LANGUAGE FlexibleContexts, MultiParamTypeClasses, FlexibleInstances #-}
+
+module T13526 where
+
+class C a where
+  op :: a -> a
+
+instance {-# OVERLAPPING #-} C [Char] where
+  op x = x
+
+instance C a => C [a] where
+  op (x:xs) = [op x]
+
+instance C a => C (Maybe a) where
+  op x = error "urk"
+
+-- We should get no complaint
+foo :: C [a] => a -> [a]
+foo x = op [x]
+
+bar :: C (Maybe a) => a -> Maybe a
+bar x = op (Just x)
diff --git a/testsuite/tests/typecheck/should_compile/T13526.stderr b/testsuite/tests/typecheck/should_compile/T13526.stderr
new file mode 100644 (file)
index 0000000..7a0f2ae
--- /dev/null
@@ -0,0 +1,7 @@
+
+T13526.hs:21:8: warning: [-Wsimplifiable-class-constraints (in -Wdefault)]
+    • The constraint ‘C (Maybe a)’ matches an instance declaration
+      instance C a => C (Maybe a) -- Defined at T13526.hs:14:10
+      This makes type inference for inner bindings fragile;
+        either use MonoLocalBinds, or simplify it using the instance
+    • In the type signature: bar :: C (Maybe a) => a -> Maybe a
index bbf3ccd..c41da18 100644 (file)
@@ -553,4 +553,5 @@ test('T13490', normal, compile, [''])
 test('T13474', normal, compile, [''])
 test('T13524', expect_broken(13524), compile, [''])
 test('T13509', normal, compile, [''])
+test('T13526', normal, compile, [''])