AMP (#8004) almost finished, review would be nice

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

David Luposchainsky
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



Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

Edward Z. Yang
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



Reply | Threaded
Open this post in threaded view
|

PS: AMP (#8004) almost finished, review would be nice

David Luposchainsky
In reply to this post by David Luposchainsky
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



Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

David Luposchainsky
In reply to this post by Edward Z. Yang
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-file-options



> 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



Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

Edward Z. Yang
> 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



Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

Simon Peyton Jones
In reply to this post by David Luposchainsky
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 at haskell.org
| http://www.haskell.org/mailman/listinfo/ghc-devs



Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

David Luposchainsky
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



Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

Edward Z. Yang
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



Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

Simon Peyton Jones
In reply to this post by David Luposchainsky
| 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

Reply | Threaded
Open this post in threaded view
|

AMP (#8004) almost finished, review would be nice

Simon Peyton Jones
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 at haskell.org] On Behalf Of Simon
| Peyton-Jones
| Sent: 04 September 2013 12:25
| To: David Luposchainsky
| Cc: ghc-devs at 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 at haskell.org
| http://www.haskell.org/mailman/listinfo/ghc-devs