#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: | Blocking: | Differential Revisions: -------------------------------------+------------------------------------- Description changed by ezyang: Old description:

At the moment, SDoc computations have full access to the entirety of DynFlags, despite only a minusculely small amount of the data structure being relevant to them. This proposal is to split out a `PprFlags` structure which will be contained in a `DynFlags`, and contain dynamic flags JUST for pretty-printing. There will be a function `pprFlags :: DynFlags -> PprFlags`. This also helps eliminate the circular dependency between `DynFlags` and `Outputable`.

This structure is not complete, but it should give a sense for some of the things that need to be put in here:

{{{

data PprFlags = PprFlags { pprUserLength :: Int, pprCols :: Int,

useUnicode :: Bool, useUnicodeSyntax :: Bool, targetPlatform :: Platform,

suppressUniques :: Bool, errorSpans :: Bool, suppressModulePrefixes :: Bool } }}}

I have a patch in-progress which factors this out, but I want to get some sign-offs that this is a good refactoring before I finish it.

Thanks!

New description: At the moment, SDoc computations have full access to the entirety of DynFlags, despite only a minusculely small amount of the data structure being relevant to them. This proposal is to split out a `PprFlags` structure which will be contained in a `DynFlags`, and contain dynamic flags JUST for pretty-printing. There will be a function `pprFlags :: DynFlags -> PprFlags`. This also helps eliminate the circular dependency between `DynFlags` and `Outputable`. This structure is not complete, but it should give a sense for some of the things that need to be put in here: {{{ data PprFlags = PprFlags { pprUserLength :: Int, pprCols :: Int, useUnicode :: Bool, useUnicodeSyntax :: Bool, targetPlatform :: Platform, suppressUniques :: Bool, errorSpans :: Bool, suppressModulePrefixes :: Bool, printExplicitKinds :: Bool, printExplicitForalls :: Bool, suppressTypeSignatures :: Bool, suppressIdInfo :: Bool, suppressCoercions :: Bool, pprCaseAsLet :: Bool, pprShowTicks :: Bool, suppressTypeApplications :: Bool, sccProfilingOn :: Bool, } }}} I have a patch in-progress which factors this out, but I want to get some sign-offs that this is a good refactoring before I finish it. The most annoying bits are pretty-printing in the code generator, where all of the platform constants (e.g. word-size) are necessary to print the right way. -- -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: | Blocking: | Differential Revisions: -------------------------------------+------------------------------------- Comment (by simonpj): I'll all for it. Simon -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: #10961 | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by dalaing): Unless I've missed something, this might be required for #10961. I'm keen to have a go at this if ezyang isn't working on it anymore. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: #10961 | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by dalaing): I've been having a go at this, and I'm up to the code gen part. It looks like there's a decent amount of the Cmm code that takes a `DynFlags` argument in order to get access to `PlatformConstants`. The `PprFlags` change might be easier if these functions took `PlatformConstants` as an argument (which would then become part of the `PprFlags` structure). Does anyone have thoughts on whether I should just go ahead and do this? Or should I make `CmmFlags` and `cmmFlags :: DynFlags -> CmmFlags` in order to wrap it nicely? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: #10961 | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by dalaing): I've just been bringing everything used by the the pretty printer across from `DynFlags` into `PprFlags` up to this point. I agree that `PprFlags` isn't a great place for `PlatformConstants`, but at the moment the pretty printing code uses `PlatformConstants` (via the Outputable instances for Cmm, amongst others), so it seems like it is needed for this kind of direct translation (unless I'm misssing something here). At the moment the Cmm code in questions takes a `DynFlags` as an argument, solely to use the `PlatformConstants` contained within `DynFlags`. I've recently been playing with a `HasPlatformConstants` typeclass, with instances for `DynFlags` and `PprFlags`, since there are a few utility functions around the place that make of `PlatformConstants` via a `DynFlags` and may not be so easy to change. If `targetPlatform` joins that class it would open some other doors. Although that might be digging too far down the wrong rabbit hole. The separation of `PprFlags` from `DynFlags` also means that `sdocWithDynFlags` becomes `sdocWtihPprFlags`, and for some of the `Outputable` instances those flags are the only way they have access to certain kinds of information. That makes `PprFlags` into a bottleneck by which various Outputable instances gather information they need (including which of certain flags are on, which of certains extensions are enabled). It'd be nice to be able to address that. @ezyang mentioned in IRC that it might be easier to add another typeclass like `Outputable` but with access to `DynFlags`. I'll probably have a look at that next, but I'm worried that the work required to thread the `DynFlags` to where they are needed might be a neutral or net negative move. It would be nice to not have a random handful of flags and extensions in `PprFlags` though, so I'll definitely give it a go. In the short-term I'm focusing on getting everything but the Cmm code working, after which I'll give `OutputableWithDyn` a go. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: #10961 | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by dalaing): If `PprFlags` is to be strictly about rendering, then `SDocContext` either needs to continue to have `DynFlags` in it, or needs to carry around the `PlatformConstants`, the general flags, and a whole heap of other stuff from `DynFlags`. That's just to handle the existing use cases, where folks have used `sdocWithDynFlags` to get hold of info they need for pretty printing where that information isn't plumbed in from anywhere. We could shift the non-rendering information from `PprFlags` into `SDocContext` - but since `PprFlags` is only being used via an `SDocContext`, it wouldn't be much of a change from the way things are now. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: #10961 | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by ezyang): I think the more refined claim is something like, "SDoc computations only need a minusculely small amount of the data structure, EXCEPT for the use of pretty-printing in the code generator, which also needs access to platform constants." The code generation pretty-printing is a small part of the overall story, so it might be worth to somehow deal with it specially. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:13 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: #10961 | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): I think your goal is this (taken from the top of the ticket).

At the moment, SDoc computations have full access to the entirety of DynFlags, despite only a minusculely small amount of the data structure being relevant to them. This proposal is to split out a PprFlags structure which will be contained in a DynFlags, and contain dynamic flags JUST for pretty-printing.

Is that your goal? If so,seems a good goal. But that seems to contradict your comment:11, where you say

If PprFlags is to be strictly about rendering, then SDocContext either needs to continue to have DynFlags in it, or needs to carry around the PlatformConstants, the general flags, and a **whole heap of other stuff from DynFlags**.

My confusion is about whether it's true that `SDoc` computations need only a "miniscule" bit of `DynFlags`. If true, then this project seems worthwhile. If false, not so worthwhile. I think the reason no one is being clear about whether this change is wanted or not is that there is confusion about what "this change" actually is. Could you, for example, list exactly the things you propose to put in `PprFlags`? And what functions/data types change their signatures? Thanks -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:15 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10143: Separate PprFlags (used by Outputable) from DynFlags -------------------------------------+------------------------------------- Reporter: ezyang | Owner: ezyang Type: task | Status: new Priority: normal | Milestone: 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: #10961 | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by bgamari): Dalaing, what ever happened to this? I would like to see this happen. Currently it is significantly harder than necessary for API users to use the pretty printer due to the spurious dependence on the entirety of `DynFlags`. Moreover, this dependency leads to an awkward coupling between the printer and the all encompassing `DynFlags` module, leading to a proliferation of import loops. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10143#comment:17 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler