add -Wmissing-deriving-strategies
AbandonedPublic

Authored by chessai on Dec 13 2018, 5:50 PM.

Details

Summary

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

add -Wmissing-deriving-strategies

chessai created this revision.Dec 13 2018, 5:50 PM

Don't accept this, please. I believe the warning message is inadequate, though I'm not quite sure how it should look, or how I would make it look better. If anyone has any advice on this please let me know.

Some thoughts:

  • Does this emit warnings for StandaloneDeriving clauses?
  • You'll want to add an entry to the GHC manual about the flag.
  • The error message might be improved with: "No deriving strategy specified. Did you want stock, newtype, or anyclass?" And then, if DerivingStrategies is not enabled, on the next line, "Use DerivingStrategies to specify a strategy." There's no need to mention via` though since that couldn't possibly have been the inferred strategy.
  • Make sure that -Wall does not enable this flag. I'm not sure if -Wall uses a whitelist or a blacklist.

You probably want to add Ryan Scott as a reviewer for this diff.

Whoops. I see now that you had already added an entry to the user manual. I think the flag should always cause warnings regardless of whether DerivingStrategies is currently enabled.

chessai added a comment.EditedDec 14 2018, 9:34 AM
  • Does this emit warnings for StandaloneDeriving clauses?

No, it doesn't, but it should!

  • The error message might be improved with: "No deriving strategy specified. Did you want stock, newtype, or anyclass?" And then, if DerivingStrategies is not enabled, on the next line, "Use DerivingStrategies to specify a strategy." There's no need to mention via` though since that couldn't possibly have been the inferred strategy.

That makes sense. I agree.

  • Make sure that -Wall does not enable this flag. I'm not sure if -Wall uses a whitelist or a blacklist.

It uses a whitelist, but that doesn't matter because I think it (-Wmissing-deriving-strategies) is on always. Compiling the following module:

module T15798b () where
data Foo a = Foo a
  deriving (Eq)

will actually trigger the warning, without any mention of it. Looks like I messed up. I think this happens because it inspects the deriving clause and just checks to see if the user has not supplied a strategy, but doesn't even check if the flag is enabled. oops.

You probably want to add Ryan Scott as a reviewer for this diff.

Thought I did. Oops. I will do that now.

chessai updated this revision to Diff 19154.Dec 14 2018, 10:22 AM
  • resolve a number of issues
  • Update the GHC manual entry to reflect that -Wmissing-deriving-strategies is independent of its related language extension.
  • Improve the error message provided by -Wmissing-deriving-strategies, and suggest to users who enable this warning to use -XDerivingStrategies if it is not enabled.

This currently doesn't warn on -XStandaloneDeriving clauses. It needs to.

chessai added a reviewer: RyanGlScott.EditedDec 14 2018, 10:22 AM

Ryan, I'd appreciate any feedback. I'll look into how to enable this for standalone deriving clauses.

chessai updated this revision to Diff 19155.Dec 14 2018, 10:42 AM
  • make -Wmissing-deriving-strategies work with -XStandaloneDeriving
chessai updated this revision to Diff 19156.Dec 14 2018, 10:57 AM
  • don't use OverloadedStrings to construct SDocs. Instead use the text convention.

This looks good to me other than the minor stylistic things I just added comments about.

compiler/rename/RnSource.hs
1012

No need for the wildcard binder here since it's unit.

1747

No need for the wildcard binder here since it's unit.

testsuite/tests/rename/should_compile/T15798a.stderr
2

Several of your tests are missing newlines at the end of the file. This doesn't cause problems for GHC, but some tools for dealing with text expect trailing newlines.

chessai updated this revision to Diff 19158.Dec 14 2018, 11:44 AM
  • some stylistic changes
RyanGlScott requested changes to this revision.Dec 15 2018, 9:00 AM

The CI is failing for reasons unrelated to this Diff. I'd recommend rebasing on top of master, since the issue has been fixed upstream.

Make sure to add an entry for this flag in docs/users_guide/using-warnings.rst (see Note [Documenting warning flags]).

testsuite/tests/rename/should_compile/T15798a.stderr
4

This message doesn't give any indication of which derived class is actually triggering the warning, which feels a bit strange to me.

This revision now requires changes to proceed.Dec 15 2018, 9:00 AM
This comment was removed by chessai.
testsuite/tests/rename/should_compile/T15798a.stderr
4

Would you prefer the error message be per-class, instead of per-deriving clause? I considered that, but wasn't sure if it was necessary. I guess if we saw a problematic deriving clause with multiple classes inside of it we could provide the error message for each of those classes. Let me know what you think.

chessai updated this revision to Diff 19169.Dec 17 2018, 9:00 AM

rebase on top of master to (hopefully) avoid test failures.

chessai updated this revision to Diff 19170.Dec 17 2018, 9:10 AM
  • add entry about flag in docs/users_guide/using-warnings.rst
chessai updated this revision to Diff 19171.Dec 17 2018, 9:16 AM
  • add missing newline to end of file
RyanGlScott added inline comments.Dec 17 2018, 11:29 AM
docs/users_guide/8.8.1-notes.rst
87

s/new -Wmissing-deriving-strategies/new -Wmissing-deriving-strategies flag/

docs/users_guide/using-warnings.rst
926

Don't bother saying "This option is new to 8.8.1"—after all, this document is intended to always reflect the latest in GHC flag developments. Instead, put a :since 8.8.1 line, which is more concise (see -Wmissing-export-lists below for an example).

testsuite/tests/rename/should_compile/T15798a.stderr
4

I don't have a strong preference as to whether the error message displays the entire deriving clause/declaration or displays an individual class. My concern is that, at the moment, the message that is shown here gives no indication whatsoever as to what code is triggering the warning (aside from a source location number of 11:3, which requires some detective work to determine where that lives in the source code). Having something like:

No deriving strategy specified. Did you want stock, newtype, or anyclass?
In the deriving clause for the class `Eq`

Would go a long way in helping users figure out where to look to fix the warning.

chessai updated this revision to Diff 19177.Dec 17 2018, 7:03 PM
  • doc changes suggested by Ryan

What do the following errors mean? The docs are failing to build.

Sphinx error:
ghc-flag (['-Wmissing-deriving-strategies', ':shortdesc: warn when a deriving clause is missing a deriving strategy', ':type: dynamic', ':reverse: -Wno-missing-deriving-strategies', ':category:']) directive missing :shortdesc: key
make[1]: * [docs/users_guide/ghc.mk:28: docs/users_guide/build-man/ghc.1] Error 2
make[1]:
* Waiting for unfinished jobs....
/home/chessai/ghc/docs/users_guide/extending_ghc.rst:854: WARNING: Unexpected indentation.
/home/chessai/ghc/docs/users_guide/glasgow_exts.rst:14309: WARNING: Duplicate ID: "pragma-SPECIALIZE".
/home/chessai/ghc/docs/users_guide/glasgow_exts.rst:14309: WARNING: Duplicate ID: "pragma-SPECIALIZE".
/home/chessai/ghc/docs/users_guide/using-optimisation.rst:: WARNING: Anonymous hyperlink mismatch: 1 references but 0 targets.
See "backrefs" attribute for IDs.

Sphinx error:
ghc-flag (['-Wmissing-deriving-strategies', ':shortdesc: warn when a deriving clause is missing a deriving strategy', ':type: dynamic', ':reverse: -Wno-missing-deriving-strategies', ':category:']) directive missing :shortdesc: key
make[1]: *** [docs/users_guide/ghc.mk:16: docs/users_guide/build-html/users_guide/index.html] Error 2
/home/chessai/ghc/docs/users_guide/extending_ghc.rst:854: WARNING: Unexpected indentation.
/home/chessai/ghc/docs/users_guide/glasgow_exts.rst:14309: WARNING: Duplicate ID: "pragma-SPECIALIZE".
/home/chessai/ghc/docs/users_guide/glasgow_exts.rst:14309: WARNING: Duplicate ID: "pragma-SPECIALIZE".
/home/chessai/ghc/docs/users_guide/using-optimisation.rst:: WARNING: Anonymous hyperlink mismatch: 1 references but 0 targets.
See "backrefs" attribute for IDs.

Sphinx error:
ghc-flag (['-Wmissing-deriving-strategies', ':shortdesc: warn when a deriving clause is missing a deriving strategy', ':type: dynamic', ':reverse: -Wno-missing-deriving-strategies', ':category:']) directive missing :shortdesc: key
make[1]: * [docs/users_guide/ghc.mk:16: docs/users_guide/users_guide.pdf] Error 2
make:
* [Makefile:128: all] Error 2

testsuite/tests/rename/should_compile/T15798a.stderr
4

I'll have to figure out how to add the typeclass name to the error message. Don't think it'll take long.

Hmm, a good question; I don't see anything wrong with your documentation. I'll have to build it locally.

bgamari requested changes to this revision.Dec 26 2018, 12:36 PM
bgamari added inline comments.
docs/users_guide/using-warnings.rst
929

The reason for the documentation build faillure is inconsistent indentation here. The previous paragraphs were indented with four spaces; this paragraph is indented by only three.

This revision now requires changes to proceed.Dec 26 2018, 12:36 PM
chessai updated this revision to Diff 19246.Dec 26 2018, 12:51 PM
  • fix indentation causing build failure
chessai updated this revision to Diff 19247.Dec 26 2018, 12:59 PM
  • fix indentation causing build failure