[GHC] #11970: Simplify Parent
#11970: Simplify Parent -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Keywords: | Operating System: Unknown/Multiple Architecture: | Type of failure: None/Unknown Unknown/Multiple | Test Case: | Blocked By: Blocking: | Related Tickets: Differential Rev(s): | Wiki Page: -------------------------------------+------------------------------------- This commit {{{ commit 96621b1b4979f449e873513e9de8d806257c9493 Associate pattern synonyms with types in module exports }}} added `PatternSynonym` to `RdrName.Parent`. That's a pretty heavy hammer. It forces us to add `IsPatSyn` to `AvailInfo`, including binary serialisation etc. And I think it's needed in precisely one place, namely `RnNames.findPatSyns`. Right? The caller of `findPatSyns` and the associated `lookupChildren` is jolly obscure and I don't fully understand it. I **think** the issue is this: * I want to allow an export item `T( K )`, where `K` is a pattern synonym, even though it's not really (yet) associated with `T`. I think the goal of this code was to reject the `K` if it's a data constructor. But we also have a **further** check in `tcExports` to check that `K` has the right type. Surely it'd be better to nuke this `PatternSynonym` and `IsPatSyn` stuff? Just use`isDataOcc` in the export lookup part. Then the `tcExport` check will object if you write an export item like `Either( Just )`. Fewer moving parts! Matthew, does this make sense? Might you look at it? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by simonpj): * owner: => mpickering -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by mpickering): That sounds much simpler. I can make the change. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:2 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by mpickering): I have looked a bit at this. The problem from last time is that when renaming exports we need to reject data constructors if they are exported with a type constructor which is not their parent. This is how the code was structured before my changes. 1. `tcg_rdr_env` is converted into a `kids_env` which is a map from `NameEnv [GlobalRdrElt]`. The intention of this map that we can lookup a type constructor and find which data constructors are allowed to be bundled with it. 2. `lookupChildren` uses this map to check that all bundled constructors are with the correct type constructor. The problem which I ran into was that as we only have `GlobalRdrElt`s, at this stage I couldn't see a way of telling whether a certain `GlobalRdrElt` arose from a data constructor or a pattern synonym. It seemed to me that the sole purpose of the `Parent` datatype was to indicate when it should be allowed to export an identifier with a certain type constructor (its parent). Because of this, I made the necessary changes to `Parent` to indicate whether a `GRE` arose from a pattern synonyn. Do you see a better way of structuring this? Unless it is possible to distinguish between a pattern synonym and a data constructor at this point then it isn't safe to ignore an entry in the export list as it might be a data constructor which we are not allowed to export with that type constructor. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): Replying to [comment:3 mpickering]:
I have looked a bit at this. The problem from last time is that when renaming exports we need to reject data constructors if they are exported with a type constructor which is not their parent.
As I mention above, I think this would best be done in `tcExports`, which is where you reject pattern synonyms that are exported with a type constructor that is not their parent. Would that solve it? I think it might make the `rnExport` code quite a bit simpler. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by mpickering): Do you know if `Parent` used for anything apart from this check? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): So far as I know, that's all the `PatternSynonym` constructor of `Parent` is used for. Just use `NoParent` instead. And kill `IsPatSyn` too. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by mpickering): I had an evening looking at this last week but got stuck trying to work out how to check exported children were in scope. It wasn't clear to me which of the `lookupOccRn` functions is needed to find the children. Especially after export renaming already comes after renaming all other top level decls I expected using one of them to just work but I need to do some further exploration to find out why it isn't the case. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): I'm afraid I don't understand the problem with which you are wrestling. My suggestion is to radically simplify what happens * Always allow `T(K)` in the renamer. Make no attempt to check that K is a child of T. * Sort it out in `tcExports`. Simon -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by mpickering): Firstly, Simon and I talked this morning about this and he suggested my problem was because the constructor names were in the wrong namespace. His hunch was correct. However, there is an unforseen problem with this approach. Say that both modules `M` and `N` export symbols which have the same name, if one of them is a record selector and we try and export it with its parent then we can an ambiguous name warning. Concretely {{{ module M where data T = T { f :: Int } }}} {{{ module N where f = "f" }}} {{{ module Foo (T(f)) where import M import N }}} In this example, `f` is looked up but it is ambiguous whether it should be `N.f` or `M.T.f`. This currently works because renaming for children in export lists is a bit weird and converts the RdrNames to FastStrings and only checks that they are indeed children of the parent without using any of the `lookup*` functions. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): This is very similar to {{{ import M import N foo = T { f = 3 } }}} where we disambiguate 'f' (via `lookupSubBndrOcc`) to the `f` that comes from `T`. Can't we do ''exactly'' the same for `T(f)` in an export list? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:10 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by mpickering): I have finally finished this small refactoring. The overloaded record stuff complicated quite a bit and makes the implementation very ugly. I will put a diff up tomorrow once I have validated locally. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2179 Wiki Page: | -------------------------------------+------------------------------------- Changes (by mpickering): * differential: => Phab:D2179 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:12 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11970: Simplify Parent for patten synonyms
-------------------------------------+-------------------------------------
Reporter: simonpj | Owner: mpickering
Type: bug | Status: new
Priority: normal | Milestone:
Component: Compiler | Version: 7.10.3
Resolution: | Keywords:
Operating System: Unknown/Multiple | Architecture:
| Unknown/Multiple
Type of failure: None/Unknown | Test Case:
Blocked By: | Blocking:
Related Tickets: | Differential Rev(s): Phab:D2179
Wiki Page: |
-------------------------------------+-------------------------------------
Comment (by Matthew Pickering
#11970: Simplify Parent for patten synonyms -------------------------------------+------------------------------------- Reporter: simonpj | Owner: mpickering Type: bug | Status: closed Priority: normal | Milestone: Component: Compiler | Version: 7.10.3 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2179 Wiki Page: | -------------------------------------+------------------------------------- Changes (by mpickering): * status: new => closed * resolution: => fixed -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11970#comment:14 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
participants (1)
-
GHC