AMP (#8004) almost finished, review would be nice
Hey ghc-devs, I finished #8004. The patch introduces a new compiler flag, "-fwarn-amp", that is on by default. The changes are pretty local, affecting TcRnDriver.lhs, PrelNames.lhs and DynFlags.hs, and you can view it here: https://github.com/quchen/ghc/compare/amp_warnings?expand=1 (Note that that's my very dirts personal branch, It needs some squashing to make the actual patch out of this; also note that it includes coloured warning output so I could debug better. Other than that it's the real thing though.) If you have the time I'd like someone who knows his way around GHC to take a look at it so I don't break stuff unexpectedly :-) A couple of issues remain: 1. Validation does not work due to the warnings issued. Since "sh validate" uses -Werror, an AMP warning will stop the compilation before tests can even be run. Unfortunately, the build system seems to use the variables 'GhcStage1HcOpts' in the build process of phase 1, which is of course done with the current (7.6.3 or whatever is installed) compiler. When adding "-fno-warn-amp" to that variable phase 1 won't build because the parameter is unknown to the old compiler. Is there some sort of -"ignore next parameter if unknown" hack, or is there a smart solution? 2. Temporarily removing the -Werror constraint from validate-settings.mk (or by using custom-settings.mk) makes the validation build go through. However, the testsuite defines a couple of violating modules, therefore there is unexpected STDERR output, hence a handful of tests fail. Should a fix for this be included in the AMP patch, or be done as a separate instance? 3. Similarly, GHC defines around 50 offending modules, creating warnings in the standard build process. Again, should this be included, or can we push that to after the feature freeze and regard it as bugfixing? Greetings, David/quchen
Have not looked at patch. Excerpts from David Luposchainsky's message of Sun Sep 01 12:23:06 -0700 2013:
1. Validation does not work due to the warnings issued. Since "sh validate" uses -Werror, an AMP warning will stop the compilation before tests can even be run. Unfortunately, the build system seems to use the variables 'GhcStage1HcOpts' in the build process of phase 1, which is of course done with the current (7.6.3 or whatever is installed) compiler. When adding "-fno-warn-amp" to that variable phase 1 won't build because the parameter is unknown to the old compiler. Is there some sort of -"ignore next parameter if unknown" hack, or is there a smart solution?
Well, can you just fix all of the errors? Otherwise, do an OPTIONS_GHC block on top of the offending module further bracketed by the appropriate preprocessor macro.
2. Temporarily removing the -Werror constraint from validate-settings.mk (or by using custom-settings.mk) makes the validation build go through. However, the testsuite defines a couple of violating modules, therefore there is unexpected STDERR output, hence a handful of tests fail. Should a fix for this be included in the AMP patch, or be done as a separate instance?
They should either ignore the warnings or get rid of the error. I think it makes sense to include it in this process.
3. Similarly, GHC defines around 50 offending modules, creating warnings in the standard build process. Again, should this be included, or can we push that to after the feature freeze and regard it as bugfixing?
(See above) If you define some of the missing instances you may break some orphan instances, but if it's just in GHC this should not be a big deal. Edward
On 2013-09-01 21:45, Edward Z. Yang wrote:
Well, can you just fix all of the ["clash with join/<*>"] errors?
Not that easily; there are a couple of places that have <*> in their API, namely Hoopl and the StgCmm modules. This raises an important issue: can I rename their <*> to say <*|*> (which pairs nicely with |*><*|)? Containers use 'join' internally a lot, but since it's not part of the API that rename should be painless.
Otherwise, do an OPTIONS_GHC block on top of the offending module further bracketed by the appropriate preprocessor macro.
I'm afraid this doesn't work, since OPTIONS_GHC flags are *pre*pended to the given options, and therefore overwritten by stuff like -Wall. Corresponding GHC docs entry: http://www.haskell.org/ghc/docs/7.6.3/html/users_guide/ch04s02.html#source-f...
If you define some of the missing instances you may break some orphan instances, but if it's just in GHC this should not be a big deal.
Adding instances doesn't cause any problems so I'll go ahead with this then. Thanks for your reply, David/quchen
I'm afraid this doesn't work, since OPTIONS_GHC flags are *pre*pended to the given options, and therefore overwritten by stuff like -Wall.
This can't be right, because we definitely have used OPTIONS_GHC to add some -fno-warn-* options in the past (just do a quick grep). Have you tried it? Edward
A user in #ghc noticed the patches I provided were hard to read due to the unrelated whitespace changes. I scolded my editor for cleaning up automatically and reverted the whitespace changes, the new version can be found here: https://github.com/quchen/ghc/compare/ghc:master...amp_warnings?expand=1 Sorry for the inconvenience, David/quchen
David Thanks for doing this. Some comments on the code. * Use warnTc or addWarnTc, rather than fiddling with mkPlainWarnMsg etc. * Ditto in checkShouldInst, instead of returning a (Maybe WarnMsg), just use warnTc inside checkShouldInst. * Don't use tryTc in tcLookupClassMaybe -- it's a sledgehammer to crack a nut. The only classes you are looking up are monad, applicative, alternative, and they will always be found -- or at least if not we have a problem, and tcLookupClass will rightly report an error. So I see no need for this tryTc and matching on maybes. * Why not just getInstEnvs and lookupInstEnv in checkShouldInst, much as in TcInteract.matchClassInst? Instead you are passing parameters around that are readily available in the Tc monad. * I don't see any documentation. I don't know about this validation stuff; maybe others can help. Why not *not* add -fno-warn-amp to GcStage1HcOpts? Simon | 1. Validation does not work due to the warnings issued. Since "sh | validate" uses -Werror, an AMP warning will stop the compilation before | tests can even be run. Unfortunately, the build system seems to use the | variables 'GhcStage1HcOpts' in the build process of phase 1, which is of | course done with the current (7.6.3 or whatever is installed) compiler. | When adding "-fno-warn-amp" to that variable phase 1 won't build because | the parameter is unknown to the old compiler. Is there some sort of | -"ignore next parameter if unknown" hack, or is there a smart solution? | | 2. Temporarily removing the -Werror constraint from validate-settings.mk | (or by using custom-settings.mk) makes the validation build go through. | However, the testsuite defines a couple of violating modules, therefore | there is unexpected STDERR output, hence a handful of tests fail. Should | a fix for this be included in the AMP patch, or be done as a separate | instance? | | 3. Similarly, GHC defines around 50 offending modules, creating warnings | in the standard build process. Again, should this be included, or can we | push that to after the feature freeze and regard it as bugfixing? | | Greetings, | David/quchen | | _______________________________________________ | ghc-devs mailing list | ghc-devs@haskell.org | http://www.haskell.org/mailman/listinfo/ghc-devs
On 2013-09-03 16:19, Simon Peyton-Jones wrote:
Some comments on the code.
Use warnTc or addWarnTc, rather than fiddling with mkPlainWarnMsg etc. Ditto in checkShouldInst, instead of returning a (Maybe WarnMsg), just use warnTc inside checkShouldInst.
Why not just getInstEnvs and lookupInstEnv in checkShouldInst, much as in TcInteract.matchClassInst? Instead you are passing parameters around that are readily available in the Tc monad.
Still getting used to the API, will refactor :-)
Don't use tryTc in tcLookupClassMaybe -- it's a sledgehammer to crack a nut. The only classes you are looking up are monad, applicative, alternative, and they will always be found -- or at least if not we have a problem, and tcLookupClass will rightly report an error. So I see no need for this tryTc and matching on maybes.
The use of tryTc there fixed the problem we (me and Dan) discussed recently on ghc-devs: the testsuite errors with GHC/Base.lhs:1:1: GHC internal error: ‛GHC.Base.Monad’ is not in scope during type checking, but it passed the renamer tcl_env of environment: [] (and the full build errored with "missing interfaces for GHC.Base"). The maybe business fixed this. (Subject of the discussion: "Cannot make ghc: Failed to load interface for GHC.Base") Also note that the Prelude is not necessarily imported, so I think the lookups here can fail regardless of the issue mentioned before.
I don't see any documentation.
In what sense? More comments, longer function headers? I thought the names used were clear, with comments giving an overview over longer sections. (If you're talking about Haddock: the AMP functions are not exported, hence no HTML docs. tcAmpWarn is called from inside tcTopSrcDecls in the same module.)
I don't know about this validation stuff; maybe others can help. Why not *not* add -fno-warn-amp to GcStage1HcOpts?
The flag is mysteriously passed to the existing GHC, i.e. it appears when building phase 1. However, the current compiler doesn't know about the flag and fails. (This was my initial attempt.) Thanks for the advice, David/quchen
Excerpts from David Luposchainsky's message of Tue Sep 03 07:40:38 -0700 2013:
I don't see any documentation.
In what sense? More comments, longer function headers? I thought the names used were clear, with comments giving an overview over longer sections. (If you're talking about Haddock: the AMP functions are not exported, hence no HTML docs. tcAmpWarn is called from inside tcTopSrcDecls in the same module.)
It's a command line flag, so the manual needs to be updated to describe it. It also needs to be added to the changelog. Cheers, Edward
| GHC/Base.lhs:1:1: | GHC internal error: ‛GHC.Base.Monad’ is not in scope during type | checking, but it passed the renamer tcl_env of environment: [] I don't understand this. The message comes from TcEnv.notFound, presumably from tcLookupGlobal. But the latter only calls notFound for an internal name (which this is not), or when compiling the very same module (GHC.Base in this case). But I think you are saying this message So something mysterious is going on. Can you try the obvious thing again and let's look at it? (I suppose you could commit as-is (when you've unravelled the validation questions) so that I can see it too.) Simon | | (and the full build errored with "missing interfaces for GHC.Base"). The | maybe business fixed this. (Subject of the discussion: "Cannot make | ghc: Failed to load interface for GHC.Base") | | Also note that the Prelude is not necessarily imported, so I think the | lookups here can fail regardless of the issue mentioned before. | | | | > I don't see any documentation. | | In what sense? More comments, longer function headers? I thought the | names used were clear, with comments giving an overview over longer | sections. (If you're talking about Haddock: the AMP functions are not | exported, hence no HTML docs. tcAmpWarn is called from inside | tcTopSrcDecls in the same module.) | | | | > I don't know about this validation stuff; maybe others can help. | > Why not *not* add -fno-warn-amp to GcStage1HcOpts? | | The flag is mysteriously passed to the existing GHC, i.e. it appears | when building phase 1. However, the current compiler doesn't know about | the flag and fails. (This was my initial attempt.) | | | | Thanks for the advice, | David/quchen
David You'll recall this interchange about the AMP patch | > Don't use tryTc in tcLookupClassMaybe -- it's a sledgehammer to crack | > a nut. The only classes you are looking up are monad, applicative, | > alternative, and they will always be found -- or at least if not we | > have a problem, and tcLookupClass will rightly report an error. So I | > see no need for this tryTc and matching on maybes. | | The use of tryTc there fixed the problem we (me and Dan) discussed | recently on ghc-devs: the testsuite errors with | | GHC/Base.lhs:1:1: | GHC internal error: ‛GHC.Base.Monad’ is not in scope during type | checking, but it passed the renamer tcl_env of environment: [] I think I know roughly what's happening. Clearly we can't look up Monad in the type environment until we've typechecked that class. But the amp-warning stuff will try to look it up *regardless*. Ditto Applicative. Trying to look it up will make GHC look for Control.Applicative.hi, which won't exist until Control.Applicative is compiled. Hence the error. I'll tidy this up with a bit of refactoring Simon | -----Original Message----- | From: ghc-devs [mailto:ghc-devs-bounces@haskell.org] On Behalf Of Simon | Peyton-Jones | Sent: 04 September 2013 12:25 | To: David Luposchainsky | Cc: ghc-devs@haskell.org | Subject: RE: AMP (#8004) almost finished, review would be nice | | | GHC/Base.lhs:1:1: | | GHC internal error: ‛GHC.Base.Monad’ is not in scope during type | | checking, but it passed the renamer tcl_env of environment: [] | | I don't understand this. The message comes from TcEnv.notFound, | presumably from tcLookupGlobal. But the latter only calls notFound for | an internal name (which this is not), or when compiling the very same | module (GHC.Base in this case). But I think you are saying this message | | So something mysterious is going on. Can you try the obvious thing | again and let's look at it? | | (I suppose you could commit as-is (when you've unravelled the validation | questions) so that I can see it too.) | | Simon | | | | | (and the full build errored with "missing interfaces for GHC.Base"). | The | | maybe business fixed this. (Subject of the discussion: "Cannot make | | ghc: Failed to load interface for GHC.Base") | | | | Also note that the Prelude is not necessarily imported, so I think the | | lookups here can fail regardless of the issue mentioned before. | | | | | | | | > I don't see any documentation. | | | | In what sense? More comments, longer function headers? I thought the | | names used were clear, with comments giving an overview over longer | | sections. (If you're talking about Haddock: the AMP functions are not | | exported, hence no HTML docs. tcAmpWarn is called from inside | | tcTopSrcDecls in the same module.) | | | | | | | | > I don't know about this validation stuff; maybe others can help. | | > Why not *not* add -fno-warn-amp to GcStage1HcOpts? | | | | The flag is mysteriously passed to the existing GHC, i.e. it appears | | when building phase 1. However, the current compiler doesn't know | about | | the flag and fails. (This was my initial attempt.) | | | | | | | | Thanks for the advice, | | David/quchen | _______________________________________________ | ghc-devs mailing list | ghc-devs@haskell.org | http://www.haskell.org/mailman/listinfo/ghc-devs
participants (3)
-
David Luposchainsky -
Edward Z. Yang -
Simon Peyton-Jones