Add `-W(no-)xxx` aliases for `-f(no-)warn-xxx` flags
ClosedPublic

Authored by quchen on Dec 14 2015, 4:43 AM.

Details

Summary
IMPORTANT: This has been published on behalf of David Luposchainsky (quchen) -- Make sure the commit author is set appropriately before landing

Also changes the user's guide to use the -W-based warnings by default.

This is part of
https://ghc.haskell.org/wiki/Design/Warnings
and addresses Trac #11218

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
hvr retitled this revision from to Add `-W(no-)xxx` aliases for `-f(no-)warn-xxx` flags.Dec 14 2015, 4:43 AM
hvr edited the test plan for this revision. (Show Details)
hvr added a reviewer: quchen.
hvr updated the Trac tickets for this revision.
hvr updated this object.

I threw this patch together in a hurry without testing it yesterday, which explains why it doesn't build because of a duplicate name. What's also missing is mentioning the new flags in the user's guide, if that's desirable.

My overall plan for GHC 8.2 is to refactor GHC's warning system to be a bit more tunable and user-friendly by ...

  1. Add -Werror=... to make specific warnings errors
  2. Mention which flag produced a warning in the warning message itself, so it can be disabled easily (see gcc, clang)
  3. Pack warnings in a nice ADT so we can standardize their form a bit more; including a "helpful hint" field would be an example of this
quchen commandeered this revision.Dec 14 2015, 5:28 AM
quchen edited reviewers, added: hvr; removed: quchen.
hvr added a comment.Dec 14 2015, 5:38 AM
In D1613#47773, @quchen wrote:
  1. Add -Werror=... to make specific warnings errors

That's Trac #11219

  1. Mention which flag produced a warning in the warning message itself, so it can be disabled easily (see gcc, clang)

That's Trac #10752

I've assigned those tickets to you so you're the owner until further notice

quchen updated this revision to Diff 5663.Dec 14 2015, 6:33 AM

Fix name clash

hvr added inline comments.Dec 14 2015, 6:42 AM
compiler/main/DynFlags.hs
195

was this really redundant (including for GHC 7.8 which be need to be able to bootstrap with)?

2616–2621

you probably didn't mean to add this yet, did you?

quchen updated this revision to Diff 5664.Dec 14 2015, 6:56 AM
quchen marked an inline comment as done.
  • Fix bootstrapping issue
  • Remove unwanted deprecations of -f flags
quchen marked an inline comment as done.Dec 14 2015, 6:57 AM
quchen added inline comments.
compiler/main/DynFlags.hs
195

I completely forgot about bootstrapping when I removed this warning. Re-adding the import.

2616–2621

I committed this too early. Removed.

quchen marked 3 inline comments as done.Dec 14 2015, 6:57 AM

I agree that this is the right direction. However, let's make sure that we are clear about the path forward regarding compatibility; the principle question is how long are we going to retain the -fwarn options?

Otherwise, there are a few more places that should be updated,

  • the flags in utils/mkUserGuidePart/Options
  • the users guide should be scanned for old references
  • there should be a fairly loud note about the introduction of the -W flags in the release notes, as well as some language describing the deprecation schedule

User manual?

Deprecate the -f versions with a message saying using -w versions?

Simon

quchen updated this revision to Diff 5669.Dec 14 2015, 9:26 AM
  • Mark old -f(no-)warn* flags as hidden
  • Fix GHCi test
quchen added a comment.EditedDec 14 2015, 9:29 AM

User manual?

Yes, will do once the features are decided.

Deprecate the -f versions with a message saying using -w versions?

Maybe we want to keep the -f versions around un-deprecated for compatibility? I *really* don't want to have a compatibility discussion about this. The pain of simply keeping the -f is miniscule. (The flags are now hidden, and could remain an undocumented legacy feature for as long as we want.)

hvr added a comment.Dec 14 2015, 11:18 AM
In D1613#47872, @quchen wrote:

Deprecate the -f versions with a message saying using -w versions?

Maybe we want to keep the -f versions around un-deprecated for compatibility? I *really* don't want to have a compatibility discussion about this. The pain of simply keeping the -f is miniscule.

(The flags are now hidden, and could remain an undocumented legacy feature for as long as we want.)

I like that. Let's just write something into the user's guide to the effect of -Wxxx being supported starting with GHC 8.0, and that for backward-compat, GHC will continue (until further notice) to accept and translate the legacy -fwarn-xxx flags into the respective -Wxxx flags.

If the user's guide mentions -fwarn-xxx only in a single place, and otherwise only refers to -Wxxx flags, in a few years we may be in a position to *consider* actually deprecating the -fwarn-* aliases. But right now it's way too early to make any decision regarding deprecation, IMO.

quchen updated this revision to Diff 5673.Dec 14 2015, 11:32 AM

Update the user's guide with -W* warnings

Should we mention the old -f syntax in the user's guide at all? I completely removed it with the newest commit.

Note that the actual usages of -fwarn will remain untouched, or we will break bootstrapping capabilities. What's left to do is updating the testsuite to use the new flags.

In D1613#47902, @hvr wrote:
In D1613#47872, @quchen wrote:

Deprecate the -f versions with a message saying using -w versions?

Maybe we want to keep the -f versions around un-deprecated for compatibility? I *really* don't want to have a compatibility discussion about this. The pain of simply keeping the -f is miniscule.

(The flags are now hidden, and could remain an undocumented legacy feature for as long as we want.)

I like that. Let's just write something into the user's guide to the effect of -Wxxx being supported starting with GHC 8.0, and that for backward-compat, GHC will continue (until further notice) to accept and translate the legacy -fwarn-xxx flags into the respective -Wxxx flags.

If the user's guide mentions -fwarn-xxx only in a single place, and otherwise only refers to -Wxxx flags, in a few years we may be in a position to *consider* actually deprecating the -fwarn-* aliases. But right now it's way too early to make any decision regarding deprecation, IMO.

I agree with this, no reason making life harder than it needs to be and forcing people to keep up with this.

hvr added a comment.EditedDec 14 2015, 12:35 PM
In D1613#47908, @quchen wrote:

Should we mention the old -f syntax in the user's guide at all? I completely removed it with the newest commit.

IMO, a short side-note doesn't hurt. The use-case is for people who want to write code that works pre/post 8.0 w/o CPP (think {-# OPTIONS_GHC -fno-warn-noise #-}). Then it's useful to know that -fwarn-* is a legacy feature available on purpose (rather than by oversight/accident) for when pre-8.0 compat is needed.

quchen updated this revision to Diff 5708.Dec 15 2015, 2:18 AM

Rebase onto HEAD

quchen updated this revision to Diff 5710.Dec 15 2015, 3:29 AM

Fix rebase fallout (update user's guide with -W again)

quchen updated this revision to Diff 5711.Dec 15 2015, 3:46 AM

Fix rebase fallout again ... waaah

quchen updated this object.Dec 15 2015, 4:13 AM
hvr added inline comments.Dec 15 2015, 6:28 AM
docs/users_guide/using-warnings.rst
22

Somewhere around here would be a good place, IMHO, to concisely mention the legacy pre-GHC8.0 -f(no-)warn-... aliases...

quchen updated this revision to Diff 5715.Dec 15 2015, 6:47 AM

Switch mkUserGuidePart to -W syntax

This revision is now accepted and ready to land.Dec 15 2015, 7:07 AM
bgamari requested changes to this revision.Dec 15 2015, 3:08 PM
bgamari added inline comments.
docs/users_guide/using-warnings.rst
22

Agreed.

This revision now requires changes to proceed.Dec 15 2015, 3:08 PM
quchen added inline comments.Dec 16 2015, 1:10 AM
docs/users_guide/using-warnings.rst
22

It's in line 88, should I move it up?

quchen added inline comments.Dec 16 2015, 1:13 AM
docs/users_guide/using-warnings.rst
22

Actually it should not be here, since this is the "warning collections" section, and there is no -fwarn-all. On the other hand, the atomic warning flags begin in line 88, so that's where the -f aliases should be described.

hvr added inline comments.Dec 16 2015, 1:35 AM
docs/users_guide/using-warnings.rst
86–91

you're right... this is indeed a better place to insert that note...

bgamari accepted this revision.Dec 16 2015, 4:26 AM
This revision is now accepted and ready to land.Dec 16 2015, 4:26 AM
This revision was automatically updated to reflect the committed changes.