[GHC] #12699: Suspicious treatment of renaming of field labels
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 (Type checker) | 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: -------------------------------------+------------------------------------- While working on my rework of interface file names (Phab:D2467) I noticed that there is some funniness in the treatment of renaming field selectors. Namely, `ifaceConDeclFields` are `IfaceTopBndr`s (and should therefore be renamed), renaming them causes `bkpreex06` to fail with the panic `find_lbl missing foo`. Looking at the environment reveals that there are selectors named `foo` in scope, but they were not renamed. I believe this is due to the implementation of `tcIfaceDataCons`, which typechecks the fields with `mapM (traverse lookupIfaceTop) (ifaceConDeclFields if_cons)`. In essence, this projects the field labels back to `OccName`s (in `ifaceConDeclFields`) and then looks each of these `OccName`s up in the name cache, thereby retrieving the un-renamed `Name`. This seems wrong. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler (Type | Version: 8.1 checker) | 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: | -------------------------------------+------------------------------------- Description changed by bgamari: @@ -4,2 +4,2 @@ - renamed), renaming them causes `bkpreex06` to fail with the panic - `find_lbl missing foo`. + renamed), renaming them (in `RnModIface`) causes `bkpreex06` to fail with + the panic `find_lbl missing foo`. New description: While working on my rework of interface file names (Phab:D2467) I noticed that there is some funniness in the treatment of renaming field selectors. Namely, `ifaceConDeclFields` are `IfaceTopBndr`s (and should therefore be renamed), renaming them (in `RnModIface`) causes `bkpreex06` to fail with the panic `find_lbl missing foo`. Looking at the environment reveals that there are selectors named `foo` in scope, but they were not renamed. I believe this is due to the implementation of `tcIfaceDataCons`, which typechecks the fields with `mapM (traverse lookupIfaceTop) (ifaceConDeclFields if_cons)`. In essence, this projects the field labels back to `OccName`s (in `ifaceConDeclFields`) and then looks each of these `OccName`s up in the name cache, thereby retrieving the un-renamed `Name`. This seems wrong. -- -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: adamgundry Type: bug | Status: new Priority: normal | Milestone: Component: Compiler (Type | Version: 8.1 checker) | 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: => adamgundry Comment: Adam, might you look at this? Thanks! -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699#comment:2 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: adamgundry Type: bug | Status: new Priority: normal | Milestone: Component: Compiler (Type | Version: 8.1 checker) | 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 ezyang): I'm also happy to advise on the intended semantics of interface renaming, but I really don't understand why the TcIface code goes into an OccName, and then back to a Name afterwards. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: adamgundry Type: bug | Status: new Priority: normal | Milestone: Component: Compiler (Type | Version: 8.1 checker) | 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 adamgundry): I don't really understand what is going on here with the interface renaming, unfortunately. The basic idea of the `DuplicateRecordFields` implementation is that we distinguish the `FieldLabelString`/`OccName` of a field from the `Name` of a record selector. For example, `data T = MkT { foo :: Int }` gives rise to a `FieldLabelString` `foo` whose selector `Name` is `$sel:foo:MkT`. Perhaps `ifConFields :: [IfaceTopBndr]` is the wrong type? I suspect it should originally have been `[FieldLabelString]`, i.e. the old code treated the `[OccName]` as a list of field labels (and hence had to look up the selectors corresponding to the labels in `tcIfaceDataCons`). Note that record selectors are not implicit TyThings, i.e. they have their own `IfaceDecl`s separate from the `IfaceDecl` of the datatype. Following Phab:D2467 I wonder if the right thing is to have `ifConFields :: [FieldLabel]` or `[FieldLbl IfaceTopBndr]`, so that interface files would store both the `FieldLabelString` and selector `Name` (avoiding duplication when the former can be derived from the latter)? That would allow the selector name to be renamed without affecting the label, and we could get rid of the `find_lbl` lookup business in `tcIfaceDataCons`. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: adamgundry Type: bug | Status: new Priority: normal | Milestone: Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: backpack 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 ezyang): * keywords: => backpack -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: adamgundry Type: bug | Status: patch Priority: normal | Milestone: Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: backpack Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3174 Wiki Page: | -------------------------------------+------------------------------------- Changes (by ezyang): * status: new => patch * differential: => Phab:D3174 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: ezyang Type: bug | Status: patch Priority: normal | Milestone: Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: backpack Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3174 Wiki Page: | -------------------------------------+------------------------------------- Changes (by adamgundry): * owner: adamgundry => ezyang Comment: Thanks very much for sorting this out! -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#12699: Suspicious treatment of renaming of field labels
-------------------------------------+-------------------------------------
Reporter: bgamari | Owner: ezyang
Type: bug | Status: patch
Priority: normal | Milestone:
Component: Compiler (Type | Version: 8.1
checker) |
Resolution: | Keywords: backpack
Operating System: Unknown/Multiple | Architecture:
| Unknown/Multiple
Type of failure: None/Unknown | Test Case:
Blocked By: | Blocking:
Related Tickets: | Differential Rev(s): Phab:D3174
Wiki Page: |
-------------------------------------+-------------------------------------
Comment (by Edward Z. Yang
#12699: Suspicious treatment of renaming of field labels -------------------------------------+------------------------------------- Reporter: bgamari | Owner: ezyang Type: bug | Status: closed Priority: normal | Milestone: Component: Compiler (Type | Version: 8.1 checker) | Resolution: fixed | Keywords: backpack Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3174 Wiki Page: | -------------------------------------+------------------------------------- Changes (by ezyang): * status: patch => closed * resolution: => fixed -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/12699#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
participants (1)
-
GHC