RE: [GHC] #11824: GHC error in desugarer lookup
Ben I'm offline, so can't reply on Phab. Calling mkTypeableBinds in tcRnHsBootDecls isn't quite right, because it'll generate lots of *bindings* whereas all we want is the *Ids* in the GlobalValEnv. (It's possible that tcg_binds is ignored in the boot-module case, but it's unsavoury to generate the bindings at all. They are generated when we process the main module.) I suggest the easiest fix is, in mkModIdBinding and mkTypeableTyConBinds, guard the binding-generation with a test for whether isHsBootOrSig is true. Simon | -----Original Message----- | From: ghc-tickets [mailto:ghc-tickets-bounces@haskell.org] On Behalf Of GHC | Sent: 13 April 2016 12:06 | Cc: ghc-tickets@haskell.org | Subject: Re: [GHC] #11824: GHC error in desugarer lookup | | #11824: GHC error in desugarer lookup | -------------------------------------+------------------------------------- | Reporter: darchon | Owner: | Type: bug | Status: patch | Priority: normal | Milestone: 8.0.1 | Component: Compiler | Version: 8.0.1-rc3 | Resolution: | Keywords: | Operating System: Unknown/Multiple | Architecture: | Type of failure: Compile-time | Unknown/Multiple | crash | Test Case: | Blocked By: | Blocking: | Related Tickets: | Differential Rev(s): Phab:D2107, | Wiki Page: | Phab:D2108 | -------------------------------------+------------------------------------- | Changes (by bgamari): | | * status: new => patch | * differential: => Phab:D2107, Phab:D2108 | | | -- | Ticket URL: | <https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fghc.haskell. | org%2ftrac%2fghc%2fticket%2f11824%23comment%3a6&data=01%7c01%7csimonpj%40064d | .mgd.microsoft.com%7cd4862d0ed83c493164fb08d3638c0642%7c72f988bf86f141af91ab2 | d7cd011db47%7c1&sdata=JaXInqbrHs1nrAM9JsMUfX6J6hfbzJHI2k5KKRbH9i0%3d> | GHC | <https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fwww.haskell. | org%2fghc%2f&data=01%7c01%7csimonpj%40064d.mgd.microsoft.com%7cd4862d0ed83c49 | 3164fb08d3638c0642%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=S85zedvr8ceEm | 4UeWtejgMl%2buRm4XMrgdWiHwcXDVuU%3d> | The Glasgow Haskell Compiler | _______________________________________________ | ghc-tickets mailing list | ghc-tickets@haskell.org | https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fmail.haskell. | org%2fcgi-bin%2fmailman%2flistinfo%2fghc- | tickets&data=01%7c01%7csimonpj%40064d.mgd.microsoft.com%7cd4862d0ed83c493164f | b08d3638c0642%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=WywP4RhTbL5cEfl%2b | xztoUe1EYuHPthiCb3vS2%2b%2fP4F8%3d
Simon Peyton Jones
Ben
I'm offline, so can't reply on Phab.
Calling mkTypeableBinds in tcRnHsBootDecls isn't quite right, because it'll generate lots of *bindings* whereas all we want is the *Ids* in the GlobalValEnv.
(It's possible that tcg_binds is ignored in the boot-module case, but it's unsavoury to generate the bindings at all. They are generated when we process the main module.)
I suggest the easiest fix is, in mkModIdBinding and mkTypeableTyConBinds, guard the binding-generation with a test for whether isHsBootOrSig is true.
Are you concerned about the bindings being generated at all or merely that they are being added to the typechecking environment? If the latter there is no reason to fear: TcBinds.addTypecheckedBinds (which TcTypeable uses) ensures that no bindings are added to the environment if we are compiling a boot module, addTypecheckedBinds :: TcGblEnv -> [LHsBinds Id] -> TcGblEnv addTypecheckedBinds tcg_env binds | isHsBootOrSig (tcg_src tcg_env) = tcg_env -- Do not add the code for record-selector bindings -- when compiling hs-boot files | otherwise = tcg_env { tcg_binds = foldr unionBags (tcg_binds tcg_env) binds } Of course, we are still doing the work of producing the bindings; if it is this that you are concerned about then I can be more careful in guarding their production. Cheers, - Ben
| If the latter there is no reason to fear: TcBinds.addTypecheckedBinds
| (which TcTypeable uses) ensures that no bindings are added to the
| environment if we are compiling a boot module,
Ah, I had missed that; it's pretty deeply hidden.
Suggestion:
* Move tcRecSelBinds from tcAddImplicits, to mkTypeableBinds.
More precisely, make a new function addDerivedBindings, which
* add record-selector bindings (via tcRecSelBinds)
* adds typeable bindings (via mkTypableBinds)
and call it where we currently call mkTypeableBinds.
* Instead of burying the "drop the bindings" deep in addTypecheckedBinds,
inline it into addDerivedBindings:
- make mkTypeableBinds *return* bindings rather than extending gbl-env
- ditto tcRecSelBinds
Then addDerivedBinds can test hs-boot, and extend the tcg_binds if necessary
That would put the two together nicely, they really are very similar.
I'm not bothered about the cost of generating bindings and then discarding them; it only happens for hs-boot files. I _am_ concerned about clarity!
Thanks!
Simon
| -----Original Message-----
| From: Ben Gamari [mailto:ben@well-typed.com]
| Sent: 13 April 2016 14:53
| To: Simon Peyton Jones
Simon Peyton Jones
| If the latter there is no reason to fear: TcBinds.addTypecheckedBinds | (which TcTypeable uses) ensures that no bindings are added to the | environment if we are compiling a boot module,
Ah, I had missed that; it's pretty deeply hidden.
Suggestion:
* Move tcRecSelBinds from tcAddImplicits, to mkTypeableBinds. More precisely, make a new function addDerivedBindings, which * add record-selector bindings (via tcRecSelBinds) * adds typeable bindings (via mkTypableBinds) and call it where we currently call mkTypeableBinds.
* Instead of burying the "drop the bindings" deep in addTypecheckedBinds, inline it into addDerivedBindings: - make mkTypeableBinds *return* bindings rather than extending gbl-env - ditto tcRecSelBinds Then addDerivedBinds can test hs-boot, and extend the tcg_binds if necessary
That would put the two together nicely, they really are very similar.
Lumping Typeable binding generation in with record selector generation is tricky... The problem is this: we must generate and typecheck record selectors on a per-group basis, since they may be used by later groups. However, to generate Typeable TyCon bindings we need to first generate a Module binding. However, we should only generate one Module binding per typechecked module, so this clearly can't be done per-group; moreover the Module binding can only be generated after types have been typechecked (since we may be generating bindings for GHC.Types, where Module itself is defined). The alternative here is to wire-in the Module and TyCon types, which I did previously but this lead to ticket:11120#comment:31. All of this is quite tiresome and I suspect things would be on the whole simpler if we just went back to wiring in these types, but that would require that we find a way to address comment:31. Of course, comment:31 itself would be moot if we could always rely on word64PrimTy being available, even on 64-bit architectures. I have long wondered why this isn't the case as it's a nasty inconsistency which adds a fair amount of ugliness in unexpected places (see, for instance, the definition of Word64 in GHC.Word). It seems to me like you should be able to use Word# if you just want a word-sized type and Word64# if you want a 64-bit type (regardless of the machine's word-size). Perhaps fixing this would and returning to wiring-in Module and TyCon be the easiest way forward. This would allow us to emit a Module binding at the beginning of typechecking and then emit TyCons in a per-group manner, just as we do for record selectors. Thoughts? Future plans aside,, would you be okay with pushing D2108 to the ghc-8.0 branch as-is in the interest of moving the release along? Cheers, - Ben
| The problem is this: we must generate and typecheck record selectors on | a per-group basis, since they may be used by later groups. Why? Record selectors are value bindings which aren't used when typechecking data types. | Future plans aside,, would you be okay with pushing D2108 to the ghc-8.0 | branch as-is in the interest of moving the release along? I guess so. But at least prominently comment the bit I missed. Simon
Simon Peyton Jones
| The problem is this: we must generate and typecheck record selectors on | a per-group basis, since they may be used by later groups.
Why? Record selectors are value bindings which aren't used when typechecking data types.
Yes, they are value bindings and may be used in bindings in later groups. For instance, let's consider the following, data ARecord = ARecord { aField :: Int } $(return []) -- group delimiter aFieldIncr :: ARecord -> Int aFieldIncr = succ . aField My point in,
| we must generate and typecheck record selectors on a per-group basis
is that we must generate the `aField` binding before proceeding to typecheck the second group (containing `aFieldIncr`). Failing to do this would result in `aField` being out of scope when we attempted to typecheck the RHS of `aFieldIncr`. Perhaps I have misunderstood? Cheers, - Ben
Oh, you are right. I was thinking of generating the Typeable bindings (and record selectors) at the end of tcTyAndClassDecls. That would work perfectly EXCEPT that we only want to generate the module-name bindings once.
But it's easy to spot if it's been done already because tcg_tr_module is a Just.
So, I suggest you generate Typeable binding along with record selectors, at the end of tcTyAndClassDecls; but take care that mkModIdBindings is a no-op if tcg_tr_module says it's been done already.
But I don't mind if you want to leave it as-is, provided you comment to avoid the confusion I fell into.
Simon
| -----Original Message-----
| From: Ben Gamari [mailto:ben@well-typed.com]
| Sent: 17 April 2016 10:44
| To: Simon Peyton Jones
participants (2)
-
Ben Gamari -
Simon Peyton Jones