[GHC] #14875: -ddump-splices pretty-printing oddities with case statements
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.2.2 Keywords: | Operating System: Unknown/Multiple Architecture: | Type of failure: Debugging Unknown/Multiple | information is incorrect Test Case: | Blocked By: Blocking: | Related Tickets: Differential Rev(s): | Wiki Page: -------------------------------------+------------------------------------- The latest installment in "Ryan finds minor bugs in `-ddump-splices`". Take this program: {{{#!hs {-# LANGUAGE ScopedTypeVariables #-} {-# LANGUAGE TemplateHaskell #-} {-# OPTIONS_GHC -ddump-splices #-} module Bug where $([d| f :: Bool -> Bool f x = case x of (True :: Bool) -> True (False :: Bool) -> False g :: Bool -> Bool g x = (case x of True -> True False -> False) :: Bool |]) }}} Compiling this gives: {{{ $ /opt/ghc/8.2.2/bin/ghci Bug.hs GHCi, version 8.2.2: http://www.haskell.org/ghc/ :? for help Loaded GHCi configuration from /home/rgscott/.ghci [1 of 1] Compiling Bug ( Bug.hs, interpreted ) Bug.hs:(6,3)-(15,6): Splicing declarations [d| f_a1sB :: Bool -> Bool f_a1sB x_a1sD = case x_a1sD of (True :: Bool) -> True (False :: Bool) -> False g_a1sC :: Bool -> Bool g_a1sC x_a1sE = (case x_a1sE of True -> True False -> False) :: Bool |] ======> f_a49Z :: Bool -> Bool f_a49Z x_a4a0 = case x_a4a0 of True :: Bool -> True False :: Bool -> False g_a49Y :: Bool -> Bool g_a49Y x_a4a1 = case x_a4a1 of True -> True False -> False :: Bool }}} Neither the `-ddump-splices` output for `f` nor `g` parse are legal Haskell: * In `f`, GHC fails to parenthesize the pattern signatures `True :: Bool` and `False :: Bool`. * In `g`, GHC fails to parenthesize the `case` expression which has an explicit `Bool` signature. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by alanz): * cc: alanz (added) -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by RyanGlScott): I talked with alanz about this, and unfortunately, this isn't quite as straightforward to fix as with previous pretty-priner bugs. This is that we need to teach `cvtl` to put parentheses around expressions when converting `SigE`s. At the same time, we don't want to add //unnecessary// parentheses—we wouldn't want to convert `Just True :: Maybe Bool` to `(Just True) :: Maybe Bool`, for instance. Unfortunately, the machinery in GHC just isn't quite up to the task. Currently, we have the [http://git.haskell.org/ghc.git/blob/5819ae2173d4b16f1fde067d39c3c215a6adfe97... hsExprNeedsParens function], but this seems to assume that the argument `HsExpr` is occurring in a function application context. In this scenario, however, that's not the case: we have a `case` expression appearing in a type annotation context. Alas, `hsExprNeedsParens` cannot distinguish between the two contexts. I think the right path forward here is to introduce a new precedence argument to `hsExprNeedsParens`. Something like: {{{#!hs data Prec = TopPrec -- Top-level | SigPrec -- Argument of a type annotation (_ :: Foo) | OpPrec -- Argument of an infix operator (_ + 1) | AppPrec -- Argument of a prefix function (f _) }}} And use that to inform `hsExprNeedsParen` in the relevant cases. It's worth noting that this data type is tantalizingly close the existing [http://git.haskell.org/ghc.git/blob/5819ae2173d4b16f1fde067d39c3c215a6adfe97... TyPrec] data type: {{{#!hs data TyPrec = TopPrec -- No parens | FunPrec -- Function args; no parens for tycon apps | TyOpPrec -- Infix operator | TyConPrec -- Tycon args; no parens for atomic }}} Save for the fact that `TyPrec` currently doesn't have a `SigPrec` constructor, and `TyPrec` has this funny business with `FunPrec`. It might be worth considering if my proposed `Prec` and `TyPrec` could be merged. Alas, I've run out of time, so this won't be happening today. One last note: it turns out there's similar problems in the Template Haskell pretty-printer as well: {{{#!hs {-# LANGUAGE ScopedTypeVariables #-} {-# LANGUAGE TemplateHaskell #-} module Bug2 where import Language.Haskell.TH main :: IO () main = putStrLn $([d| f :: Bool -> Bool f x = case x of (True :: Bool) -> True (False :: Bool) -> False g :: Bool -> Bool g x = (case x of True -> True False -> False) :: Bool |] >>= stringE . pprint) }}} {{{ f_0 :: GHC.Types.Bool -> GHC.Types.Bool f_0 x_1 = case x_1 of GHC.Types.True :: GHC.Types.Bool -> GHC.Types.True GHC.Types.False :: GHC.Types.Bool -> GHC.Types.False g_2 :: GHC.Types.Bool -> GHC.Types.Bool g_2 x_3 = case x_3 of GHC.Types.True -> GHC.Types.True GHC.Types.False -> GHC.Types.False :: GHC.Types.Bool }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:2> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): Yes, using a precedence argument must be the Right Thing. We do this in quite a few places, notably `TyCoRep` and even `RtClosureInspect` and `Debugger`. Maybe the thing to do is to have {{{ newtype PprPrec = Prec Int deriving( Eq, Ord ) }}} in `BasicTypes` or `Outputable`, and use it for everything. Then we can have local defns like {{{ topPrec = 0 funPrec = 1 ... }}} and the various applications of it won't conflict with each other. Maybe `topPrec` can be shared. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by RyanGlScott): Indeed. As currently designed, however, `TyPrec` doesn't fit neatly into this `PprPrec` paradigm, since the `Eq`/`Ord` instances for `TyPrec` treat `FunPrec` and `TyOpPrec` as having the same precedence (but they still need to be separate constructors, so sayeth [http://git.haskell.org/ghc.git/blob/cab3e6bfa8486c6c8eecac269c54d662f1371a0c... this note]). But in any case, I don't think I need to touch anything `Type`-related in this patch to begin with. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:4> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj):
As currently designed, however, TyPrec doesn't fit neatly into this PprPrec paradigm
Yes it does! The Note says that currently `FunPrec` and `TyOpPrec` are treated as equal. So we could say {{{ funPrec = Prec 2 tyOpPrec = Prec 2 }}} We have two names so that we can later give them different precedence if we want. As the note says {{{ But the two are different constructors of TyPrec so we could make (->) bind more or less tightly if we wanted. }}} Seems a neat fit to me. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:5> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by RyanGlScott): Oh, right you are. I thought that there was code in GHC that was //distinguishing// `FunPrec` from `TyOpPrec`, but after a quick audit, that does not appear to be the case. (Which leaves me a bit baffled that we didn't just define `funPrec = TyOpPrec` and avoided the need for such bizarre `Eq`/`Ord` instances. Oh well.) In that case, this should present a nice opportunity for some code cleanup. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:6> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: patch Priority: normal | Milestone: Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D4688 Wiki Page: | -------------------------------------+------------------------------------- Changes (by RyanGlScott): * status: new => patch * differential: => Phab:D4688 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:7> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: closed Priority: normal | Milestone: 8.6.1 Component: Compiler | Version: 8.2.2 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D4688 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * status: patch => closed * resolution: => fixed * milestone: => 8.6.1 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:8> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#14875: -ddump-splices pretty-printing oddities with case statements -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: bug | Status: closed Priority: normal | Milestone: 8.6.1 Component: Compiler | Version: 8.2.2 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Debugging | Unknown/Multiple information is incorrect | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D4688 Wiki Page: | -------------------------------------+------------------------------------- Comment (by Ben Gamari <ben@…>): In [changeset:"21e1a00c0ccf3072ccc04cd1acfc541c141189d2/ghc" 21e1a00c/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="21e1a00c0ccf3072ccc04cd1acfc541c141189d2" Fix #14875 by introducing PprPrec, and using it Trying to determine when to insert parentheses during TH conversion is a bit of a mess. There is an assortment of functions that try to detect this, such as: * `hsExprNeedsParens` * `isCompoundHsType` * `hsPatNeedsParens` * `isCompoundPat` * etc. To make things worse, each of them have slightly different semantics. Plus, they don't work well in the presence of explicit type signatures, as #14875 demonstrates. All of these problems can be alleviated with the use of an explicit precedence argument (much like what `showsPrec` currently does). To accomplish this, I introduce a new `PprPrec` data type, and define standard predences for things like function application, infix operators, function arrows, and explicit type signatures (that last one is new). I then added `PprPrec` arguments to the various `-NeedsParens` functions, and use them to make smarter decisions about when things need to be parenthesized. A nice side effect is that functions like `isCompoundHsType` are now completely unneeded, since they're simply aliases for `hsTypeNeedsParens appPrec`. As a result, I did a bit of refactoring to remove these sorts of functions. I also did a pass over various utility functions in GHC for constructing AST forms and used more appropriate precedences where convenient. Along the way, I also ripped out the existing `TyPrec` data type (which was tailor-made for pretty-printing `Type`s) and replaced it with `PprPrec` for consistency. Test Plan: make test TEST=T14875 Reviewers: alanz, goldfire, bgamari Reviewed By: bgamari Subscribers: rwbarton, thomie, carter GHC Trac Issues: #14875 Differential Revision: https://phabricator.haskell.org/D4688 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/14875#comment:9> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC