[GHC] #10853: Refine addTopDecls

[GHC]

#10853: Refine addTopDecls
-------------------------------------+-------------------------------------
        Reporter:  goldfire          |                   Owner:
            Type:  bug               |                  Status:  new
        Priority:  normal            |               Milestone:  8.0.1
       Component:  Template Haskell  |                 Version:  7.10.2
      Resolution:                    |                Keywords:
Operating System:  Unknown/Multiple  |            Architecture:
                                     |  Unknown/Multiple
 Type of failure:  None/Unknown      |               Test Case:
      Blocked By:                    |                Blocking:
 Related Tickets:                    |  Differential Revisions:
-------------------------------------+-------------------------------------

Old description:

> This suggested refactoring/documentation task is spurred on by #10486.
> Readers do not have to read that ticket to understand this task.
>
> Currently, `addTopDecls` is restricted in what it can add. In an email
> exchange with Geoff Mainland (the original implementor of `addTopDecls`),
> it was unclear exactly why this restriction is in place. So, I propose
> lifting the restriction.
>
> We do have to be careful about ordering. As it is currently implemented,
> here is what happens:
>
> {{{
> -- region A, including everything up to but not including a top-level
> splice
> $( {- region B, the contents of a top-level splice -} )
> -- region C, everything after the splice
> }}}
>
> In addition to those regions, we also have
>
> {{{
> -- region D, consisting of all declarations added with `addTopDecls`.
> }}}
>
> By reading the code (`TcRnDriver.tc_rn_src_decls`), it seems that the
> expected behavior should be this:
>
> 1. Rename region A. Anything defined in other regions is not in scope.
> 2. Rename region D, with declarations from region A in scope.
> 3. Type-check regions A and D together.
> 4. When running TH code in region B, all declarations from A and D are in
> scope and can be accessed via `reify` and friends.
> 5. Combine regions B and C together, and recur to step 1 (where the
> combined B and C is now a new region A).
>
> This all means that A and D are mutually recursive w.r.t. the type-
> checker but not the renamer. This is a bit peculiar, but not terrible.
> For example, this means that there could be a function whose definition
> is in A and type signature in D. Proper mutual recursion would be hard
> (impossible?) without better renaming support.
>
> In any case, I don't see a reason to restrict what's allowed in
> `addTopDecls`.
>
> So, at a minimum, I propose:
> * Remove restrictions in `addTopDecls`
> * Document `addTopDecls` generally
> * Document the above behavior, detailing the interaction between these
> regions
>
> Perhaps better, we could do:
> * Remove restrictions in `addTopDecls`
> * Rejigger the implementation of `addTopDecls` to lump region D in with B
> and C. This looks quite straightforward, and it makes for a simpler story
> about mutual recursion. This is a potentially-breaking change, if
> existing code has, say, a function defined in A with its type signature
> in D.
> * Document the new, simpler behavior
>
> Regardless of which proposal we go with, some testing is in order, to
> make sure that this is all correct. I've done no testing in formulating
> this ticket, just reading code.

New description:

 This suggested refactoring/documentation task is spurred on by #10486.
 Readers do not have to read that ticket to understand this task.

 Currently, `addTopDecls` is restricted in what it can add. In an email
 exchange with Geoff Mainland (the original implementor of `addTopDecls`),
 it was unclear exactly why this restriction is in place. So, I propose
 lifting the restriction.

 We do have to be careful about ordering. As it is currently implemented,
 here is what happens:

 {{{
 -- region A, including everything up to but not including a top-level
 splice
 $( {- region B, the contents of a top-level splice -} )
 -- region C, everything after the splice
 }}}

 In addition to those regions, we also have

 {{{
 -- region D, consisting of all declarations added with `addTopDecls`,
 -- as called from splices in region A
 }}}

 By reading the code (`TcRnDriver.tc_rn_src_decls`), it seems that the
 expected behavior should be this:

 1. Rename region A. Anything defined in other regions is not in scope.
 2. Rename region D, with declarations from region A in scope.
 3. Type-check regions A and D together.
 4. When running TH code in region B, all declarations from A and D are in
 scope and can be accessed via `reify` and friends.
 5. Combine regions B and C together, and recur to step 1 (where the
 combined B and C is now a new region A).

 This all means that A and D are mutually recursive w.r.t. the type-checker
 but not the renamer. This is a bit peculiar, but not terrible. For
 example, this means that there could be a function whose definition is in
 A and type signature in D. Proper mutual recursion would be hard
 (impossible?) without better renaming support.

 In any case, I don't see a reason to restrict what's allowed in
 `addTopDecls`.

 So, at a minimum, I propose:
 * Remove restrictions in `addTopDecls`
 * Document `addTopDecls` generally
 * Document the above behavior, detailing the interaction between these
 regions

 Perhaps better, we could do:
 * Remove restrictions in `addTopDecls`
 * Rejigger the implementation of `addTopDecls` to lump region D in with B
 and C. This looks quite straightforward, and it makes for a simpler story
 about mutual recursion. This is a potentially-breaking change, if existing
 code has, say, a function defined in A with its type signature in D.
 * Document the new, simpler behavior

 Regardless of which proposal we go with, some testing is in order, to make
 sure that this is all correct. I've done no testing in formulating this
 ticket, just reading code.

 EDIT: Clarify that region D is created from splices from region A

--

Comment (by goldfire):

 The behavior in comment:2 is documented: See the last bullet point under
 section 7.17.1
 [https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/template-
 haskell.html here]. Mind you, I didn't say this is documented well or in
 an obvious place (though I think we could do worse). If you have a
 concrete suggestion of how to make this better, I'm happy to take it.

 Does this clarify?

--
Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10853#comment:3>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler