[GHC] #10098: Refactor wild card renaming
#10098: Refactor wild card renaming -------------------------------------+------------------------------------- Reporter: | Owner: thomasw thoughtpolice | Status: new Type: bug | Milestone: 7.12.1 Priority: normal | Version: 7.11 Component: Compiler | Operating System: Unknown/Multiple Keywords: | Type of failure: None/Unknown Architecture: | Blocked By: Unknown/Multiple | Related Tickets: #9922 Test Case: | Blocking: | Differential Revisions: Phab:D613 | -------------------------------------+------------------------------------- Imported from #9922, comment #5: --------------------- Austin: let's merge this to 7.10.1. Thomas: I'm not very happy with the way that wild-card error reporting is done. I think we discussed this before, but left it on one side until it was all working. Which it now is. What I suggest is this: Things I dislike: * I dislike the `checkParitalTypeSignature` and `checkNoPartialType` stuff in `RdrHsSyn`. The only checks that belong in `RdrHsSym` are ones that prevent you building a syntax tree at all. All other checks are best done later, when good error reporting is easier, and you can recover from errors. * I particularly hate the `RnTypes.extractWildcards` stuff. It's like a whole extra renaming pass over the type, changing `HsWildCardTy` to `HsNamedWildCardTy` with an `Exact` `RdrName` in it. Yuk! Here is a possible plan: * Remove all the checking from `RdrHsSyn`, unless we can't build a syntax tree without it. * Collapse `HsWildcardTy` and `HsNamedWildcardTy` into one, with a boolean (or a `Named`/`Anonymous` flag) to distinguish. * Provide a specialised version of `rnLHsType`, perhpas `rnLHsTypeWithWildCards`, that does the inital pass to find the named wildcards and bring them into scope. This version is called in the places where you can have a type with wildcards, namely * `TypeSig` * `ExprWithTySig` * `rnHsBndrSig` * `rnLHsTypeWithWildCards` can work like this: 1. Collect all the named wildcards, and bring them into scope. This is a simple, ''pure'' function. 2. Call `rnLHsType`. When `rnLHsType` finds an anonymous wildcard, just make up a fresh name, rather than looking it up. 3. Collect all the wildcards (named or anonymous) to get a `[Name]`; again a pure function 4. Return the renamed type and the wildcard names That makes three passes, but each is simple. In fact (1) and (4) could perhaps be the same function, with a boolean flag to say which wildcards to return. * All this means that when `rnLHsType` is called directly (not via `rnLHsTypeWithWildCards`) on a type like `_ -> Int`, it will succeed, generateing a fresh name for the `_`. That's fine. In `tc_hs_type` we will find it is not in scope, so we can say "Unexpected wildcard in type", and the enclosing location information will nail down the details. * There are, I think, three places where `HsWithBndrs` is used: `HsDecls.HsTyPats`, `HsDecls.RuleBndr`, `HsPat.SigPatIn`. In the latter two I think that wildcards should be legal; in the first not so. (Do you have tests for all three?) So the caller of `rnHsBndrSig` should check for empty wildcards in the cases where there shouldn't be any. I this this is just in type/data family patterns. I have not throught throught the extra-constraints wild card, but I think a similar plan should work. Does this make sense? Might you do it? (To HEAD, of course.) Thanks Simon -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10098> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#10098: Refactor wild card renaming -------------------------------------+------------------------------------- Reporter: thoughtpolice | Owner: thomasw Type: bug | Status: new Priority: normal | Milestone: 7.12.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: None/Unknown | Unknown/Multiple Blocked By: | Test Case: Related Tickets: #9922 | Blocking: | Differential Revisions: Phab:D613 -------------------------------------+------------------------------------- Comment (by Austin Seipp <austin@…>): In [changeset:"058af6c90a0e8d122f2d1339b6b4fd0b5ec83d05/ghc"]: {{{ #!CommitTicketReference repository="ghc" revision="058af6c90a0e8d122f2d1339b6b4fd0b5ec83d05" Refactor wild card renaming Summary: Refactor wild card error reporting * Merge `HsWildcardTy` and `HsNamedWildcardTy` into one constructor `HsWildCardTy` with as field the new type `HsWildCardInfo`, which has two constructors: `AnonWildCard` and `NamedWildCard`. * All partial type checks are removed from `RdrHsSyn.hs` and are now done during renaming in order to report better error messages. When wild cards are allowed in a type, the new function `rnLHsTypeWithWildCards` (or `rnHsSigTypeWithWildCards`) should be used. This will bring the named wild cards into scope before renaming them. When this is not done, renaming will trigger "Unexpected wild card..." errors. Unfortunately, this has to be done separately for anonymous wild cards because they are given a fresh name during renaming, so they will not cause an out-of-scope error. They are handled in `tc_hs_type`, as a special case of a lookup that fails. The previous opt-out approach is replaced with an opt-in approach. No more panics because of forgotten checks! * `[t| _ |]` isn't caught by the above two checks, so it is currently handled by a special case. The error message (generated in the `DsM` monad) doesn't provide as much context information as the other cases. * Instead of three (!) functions that walk `HsType`, there is now only one pure function called `collectWildCards`. * Alternative approach: catch all unwanted wild cards in `rnHsTyKi` by looking at the `HsDocContext`. This will reduce the number of places to catch unwanted wild cards form three to one, and make the error messages more uniform, albeit less informative, as the error context for renaming is not as informative as the one for type checking. A new constructor of `HsDocContext` will be required for pattern synonyms signatures. Small problem: currently type-class type signatures can't be distinguished from type signatures using the `HsDocContext`. This requires an update to the Haddock submodule. Test Plan: validate Reviewers: goldfire, simonpj, austin Reviewed By: simonpj Subscribers: bgamari, thomie, goldfire Differential Revision: https://phabricator.haskell.org/D613 GHC Trac Issues: #10098 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10098#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#10098: Refactor wild card renaming -------------------------------------+------------------------------------- Reporter: thoughtpolice | Owner: thomasw Type: task | Status: new Priority: normal | Milestone: 7.12.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: #9922 | Differential Revisions: Phab:D613 -------------------------------------+------------------------------------- Changes (by thomie): * type: bug => task Comment: I think this is done. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10098#comment:2> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#10098: Refactor wild card renaming -------------------------------------+------------------------------------- Reporter: thoughtpolice | Owner: thomasw Type: task | Status: closed Priority: normal | Milestone: 7.12.1 Component: Compiler | Version: 7.11 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: #9922 | Differential Revisions: Phab:D613 -------------------------------------+------------------------------------- Changes (by thomie): * status: new => closed * resolution: => fixed -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10098#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC