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