Add -Wmissing-deriving-strategieswip/D5451

Warn users when -XDerivingStrategies is enabled but not used, at each
potential use site.

add -Wmissing-deriving-strategies

Reviewers: bgamari, RyanGlScott

Subscribers: andrewthad, rwbarton, carter

GHC Trac Issues: #15798

Differential Revision: https://phabricator.haskell.org/D5451

13 files changed, 96 insertions, 1 deletions

diff --git a/compiler/main/DynFlags.hs b/compiler/main/DynFlags.hs
index 02ad366338..7296809155 100644
--- a/compiler/main/DynFlags.hs
+++ b/compiler/main/DynFlags.hs
@@ -834,6 +834,7 @@ data WarningFlag =
    | Opt_WarnStarBinder                   -- Since 8.6
    | Opt_WarnImplicitKindVars             -- Since 8.6
    | Opt_WarnSpaceAfterBang
+   | Opt_WarnMissingDerivingStrategies    -- Since 8.8
    deriving (Eq, Show, Enum)
 
 data Language = Haskell98 | Haskell2010
@@ -4022,6 +4023,7 @@ wWarningFlagsDeps = [
   flagSpec "wrong-do-bind"               Opt_WarnWrongDoBind,
   flagSpec "missing-pattern-synonym-signatures"
                                     Opt_WarnMissingPatternSynonymSignatures,
+  flagSpec "missing-deriving-strategies" Opt_WarnMissingDerivingStrategies,
   flagSpec "simplifiable-class-constraints" Opt_WarnSimplifiableClassConstraints,
   flagSpec "missing-home-modules"        Opt_WarnMissingHomeModules,
   flagSpec "unrecognised-warning-flags"  Opt_WarnUnrecognisedWarningFlags,
diff --git a/compiler/rename/RnSource.hs b/compiler/rename/RnSource.hs
index c76eb31abc..e3e852984e 100644
--- a/compiler/rename/RnSource.hs
+++ b/compiler/rename/RnSource.hs
@@ -1009,6 +1009,7 @@ rnSrcDerivDecl (DerivDecl _ ty mds overlap)
            <- rnLDerivStrategy DerivDeclCtx mds $ \strat_tvs ppr_via_ty ->
               rnAndReportFloatingViaTvs strat_tvs loc ppr_via_ty "instance" $
               rnHsSigWcType BindUnlessForall DerivDeclCtx ty
+       ; warnNoDerivStrat mds' loc
        ; return (DerivDecl noExt ty' mds' overlap, fvs) }
   where
     loc = getLoc $ hsib_body $ hswc_body ty
@@ -1710,6 +1711,29 @@ rnDataDefn doc (HsDataDefn { dd_ND = new_or_data, dd_cType = cType
            ; return (cL loc ds', fvs) }
 rnDataDefn _ (XHsDataDefn _) = panic "rnDataDefn"
 
+warnNoDerivStrat :: Maybe (LDerivStrategy GhcRn)
+                 -> SrcSpan
+                 -> RnM ()
+warnNoDerivStrat mds loc
+  = do { dyn_flags <- getDynFlags
+       ; when (wopt Opt_WarnMissingDerivingStrategies dyn_flags) $
+           case mds of
+             Nothing -> addWarnAt
+               (Reason Opt_WarnMissingDerivingStrategies)
+               loc
+               (if xopt LangExt.DerivingStrategies dyn_flags
+                 then no_strat_warning
+                 else no_strat_warning $+$ deriv_strat_nenabled
+               )
+             _ -> pure ()
+       }
+  where
+    no_strat_warning :: SDoc
+    no_strat_warning = text "No deriving strategy specified. Did you want stock"
+                       <> text ", newtype, or anyclass?"
+    deriv_strat_nenabled :: SDoc
+    deriv_strat_nenabled = text "Use DerivingStrategies to specify a strategy."
+
 rnLHsDerivingClause :: HsDocContext -> LHsDerivingClause GhcPs
                     -> RnM (LHsDerivingClause GhcRn, FreeVars)
 rnLHsDerivingClause doc
@@ -1720,6 +1744,7 @@ rnLHsDerivingClause doc
   = do { (dcs', dct', fvs)
            <- rnLDerivStrategy doc dcs $ \strat_tvs ppr_via_ty ->
               mapFvRn (rn_deriv_ty strat_tvs ppr_via_ty) dct
+       ; warnNoDerivStrat dcs' loc
        ; pure ( cL loc (HsDerivingClause { deriv_clause_ext = noExt
                                          , deriv_clause_strategy = dcs'
                                          , deriv_clause_tys = cL loc' dct' })
diff --git a/compiler/typecheck/TcDerivUtils.hs b/compiler/typecheck/TcDerivUtils.hs
index 86205de5fd..5f48e5f003 100644
--- a/compiler/typecheck/TcDerivUtils.hs
+++ b/compiler/typecheck/TcDerivUtils.hs
@@ -62,7 +62,7 @@ import ListSetOps (assocMaybe)
 -- is a simple reader around 'TcRn'.
 type DerivM = ReaderT DerivEnv TcRn
 
--- | Is GHC processing a stanalone deriving declaration?
+-- | Is GHC processing a standalone deriving declaration?
 isStandaloneDeriv :: DerivM Bool
 isStandaloneDeriv = asks (go . denv_ctxt)
   where
diff --git a/docs/users_guide/8.8.1-notes.rst b/docs/users_guide/8.8.1-notes.rst
index 6e52a63038..836736499b 100644
--- a/docs/users_guide/8.8.1-notes.rst
+++ b/docs/users_guide/8.8.1-notes.rst
@@ -84,6 +84,10 @@ Compiler
 
 - The deprecated ghc-flag ``-Wamp`` has been removed.
 
+- Add new :ghc-flag:`-Wmissing-deriving-strategies` flag that warns users when they are not
+  taking advantage of :extension:`DerivingStrategies`. The warning is supplied at each
+  ``deriving`` site.
+
 Runtime system
 ~~~~~~~~~~~~~~
 
diff --git a/docs/users_guide/extending_ghc.rst b/docs/users_guide/extending_ghc.rst
index a913684a15..02847c9b86 100644
--- a/docs/users_guide/extending_ghc.rst
+++ b/docs/users_guide/extending_ghc.rst
@@ -851,6 +851,7 @@ In general, the ``pluginRecompile`` field has the following type::
 
 The ``PluginRecompile`` data type is an enumeration determining how the plugin
 should affect recompilation. ::
+
     data PluginRecompile = ForceRecompile | NoForceRecompile | MaybeRecompile Fingerprint
 
 A plugin which declares itself impure using ``ForceRecompile`` will always
diff --git a/docs/users_guide/using-warnings.rst b/docs/users_guide/using-warnings.rst
index 6a6166bf0d..03ca184531 100644
--- a/docs/users_guide/using-warnings.rst
+++ b/docs/users_guide/using-warnings.rst
@@ -904,6 +904,27 @@ of ``-W(no-)*``.
     This option isn't enabled by default because it can be very noisy,
     and it often doesn't indicate a bug in the program.
 
+.. ghc-flag:: -Wmissing-deriving-strategies
+    :shortdesc: warn when a deriving clause is missing a deriving strategy
+    :type: dynamic
+    :reverse: -Wno-missing-deriving-strategies
+    :category:
+
+    :since: 8.8.1
+
+    The datatype below derives the ``Eq`` typeclass, but doesn't specify a
+    strategy. When :ghc-flag:`-Wmissing-deriving-strategies` is enabled,
+    the compiler will emit a warning about this. ::
+
+        data Foo a = Foo a
+          deriving (Eq)
+
+    The compiler will warn here that the deriving clause doesn't specify a
+    strategy. If the warning is enabled, but :extension:`DerivingStrategies` is
+    not enabled, the compiler will suggest turning on the
+    :extension:`DerivingStrategies` extension. This option is not on by default,
+    having to be turned on manually or with :ghc-flag:`-Weverything`.
+
 .. ghc-flag:: -Wmissing-fields
     :shortdesc: warn when fields of a record are uninitialised
     :type: dynamic
diff --git a/testsuite/tests/rename/should_compile/T15798a.hs b/testsuite/tests/rename/should_compile/T15798a.hs
new file mode 100644
index 0000000000..d34e55bdab
--- /dev/null
+++ b/testsuite/tests/rename/should_compile/T15798a.hs
@@ -0,0 +1,12 @@
+{-# LANGUAGE DerivingStrategies #-}
+
+{-# OPTIONS_GHC -Wmissing-deriving-strategies #-}
+
+module T15798a () where
+
+data Foo a = Foo a
+  deriving stock (Eq)
+
+data Bar a = Bar a
+  deriving (Eq, Show)
+  deriving stock (Ord)
diff --git a/testsuite/tests/rename/should_compile/T15798a.stderr b/testsuite/tests/rename/should_compile/T15798a.stderr
new file mode 100644
index 0000000000..6832205228
--- /dev/null
+++ b/testsuite/tests/rename/should_compile/T15798a.stderr
@@ -0,0 +1,3 @@
+
+T15798a.hs:11:3: warning: [-Wmissing-deriving-strategies]
+    No deriving strategy specified. Did you want stock, newtype, or anyclass?
diff --git a/testsuite/tests/rename/should_compile/T15798b.hs b/testsuite/tests/rename/should_compile/T15798b.hs
new file mode 100644
index 0000000000..8504d6fee1
--- /dev/null
+++ b/testsuite/tests/rename/should_compile/T15798b.hs
@@ -0,0 +1,9 @@
+{-# LANGUAGE StandaloneDeriving #-}
+
+{-# OPTIONS_GHC -Wmissing-deriving-strategies #-}
+
+module T15798b () where
+
+data Foo a = Foo a
+
+deriving instance Eq a => Eq (Foo a)
diff --git a/testsuite/tests/rename/should_compile/T15798b.stderr b/testsuite/tests/rename/should_compile/T15798b.stderr
new file mode 100644
index 0000000000..de1b09ee48
--- /dev/null
+++ b/testsuite/tests/rename/should_compile/T15798b.stderr
@@ -0,0 +1,4 @@
+
+T15798b.hs:9:19: warning: [-Wmissing-deriving-strategies]
+    No deriving strategy specified. Did you want stock, newtype, or anyclass?
+    Use DerivingStrategies to specify a strategy.
diff --git a/testsuite/tests/rename/should_compile/T15798c.hs b/testsuite/tests/rename/should_compile/T15798c.hs
new file mode 100644
index 0000000000..f3048eded5
--- /dev/null
+++ b/testsuite/tests/rename/should_compile/T15798c.hs
@@ -0,0 +1,6 @@
+{-# OPTIONS_GHC -Wmissing-deriving-strategies #-}
+
+module T15798c () where
+
+data Foo a = Foo a
+  deriving (Eq)
diff --git a/testsuite/tests/rename/should_compile/T15798c.stderr b/testsuite/tests/rename/should_compile/T15798c.stderr
new file mode 100644
index 0000000000..868266aa3a
--- /dev/null
+++ b/testsuite/tests/rename/should_compile/T15798c.stderr
@@ -0,0 +1,4 @@
+
+T15798c.hs:6:3: warning: [-Wmissing-deriving-strategies]
+    No deriving strategy specified. Did you want stock, newtype, or anyclass?
+    Use DerivingStrategies to specify a strategy.
diff --git a/testsuite/tests/rename/should_compile/all.T b/testsuite/tests/rename/should_compile/all.T
index b6c06c151c..0bcd25ccf1 100644
--- a/testsuite/tests/rename/should_compile/all.T
+++ b/testsuite/tests/rename/should_compile/all.T
@@ -163,3 +163,7 @@ test('T14747', [], multimod_compile, ['T14747', '-v0'])
 test('T15149', [], multimod_compile, ['T15149', '-v0'])
 test('T13064', normal, compile, [''])
 test('T15994', [], run_command, ['$MAKE -s --no-print-directory T15994'])
+test('T15798a', normal, compile, [''])
+test('T15798b', normal, compile, [''])
+test('T15798c', normal, compile, [''])
+