Fix #4986, and extra refactoring

[ezyang]

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.
• If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Added test for the bugfix

[phadej]

I have uneasy feeling about re-using Pretty for this. The idea is to use Pretty to display data to user. For debugging we could use Show but it's not pretty. I propose adding PShow class, and define show = pshow. I can make a PR soon. This can go in as is, but I will revert Pretty related commits

[ezyang]

@phadej Show is not all that appropriate either, because conventionally the Show instance of a datatype is intended to be valid Haskell, and I don't necessarily want to force Pretty-printed variants of types to be valid Haskell; a very simple case where this would be extremely annoying are newtypes of Text, where barfing out the constructor name would be a big problem for readability. So I'm 👎 on PShow, but 👍 for Outputable. I guess I'll add that class...

[phadej]

Given `IsString`, PShow could be a little "reality twister". My doubt is more about inconsistencies, how `Maybe InstalledPackageInfo` or `[PackageDescription]` would be debug output? Cabal-like syntax is compact, but it doesn't *compose* well. So I'm 👍 for Haskell-like record syntax for those. I'd recommend splitting this PR into bug fix, which can go in (with old style asserts) and then we can bikeshed on Outputtable/PShow/Pretty without hurry (that's not critical to get into 2.2, is it?)

…

Sent from my iPhone

On 2 Jan 2018, at 3.11, Edward Z. Yang ***@***.***> wrote: @phadej Show is not all that appropriate either, because conventionally the Show instance of a datatype is intended to be valid Haskell, and I don't necessarily want to force Pretty-printed variants of types to be valid Haskell; a very simple case where this would be extremely annoying are newtypes of Text, where barfing out the constructor name would be a big problem for readability. So I'm 👎 on PShow, but 👍 for Outputable. I guess I'll add that class... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[phadej]

FWIW PShow can have Generic implementation, taking into account newtypes and records!

A note about composability: parens!

[BardurArantsson]

Seems like outputting somethink S-expression-like would be pretty trivial and sufficient for debugging?

[ezyang]

I force-pushed the reduced patch set, without updated asserts or pretty-printing.

Given IsString, PShow could be a little "reality twister".

I don't know what you mean by this. Are you suggesting that a PackageName can acceptably render as "pkgname", because -XOverloadedStrings can interpret this as a PackageName?

But there are still situations where Haskell syntax is unacceptably verbose, and there is no way to "hack" it with OverloadedStrings. One example that came up in the patchset:

-- | A ConfiguredId is a package ID for a configured package.
--
-- Once we configure a source package we know its UnitId. It is still
-- however useful in lots of places to also know the source ID for the package.
-- We therefore bundle the two.
--
-- An already installed package of course is also "configured" (all its
-- configuration parameters and dependencies have been specified).
data ConfiguredId = ConfiguredId {
    confSrcId  :: PackageId
  , confCompName :: Maybe ComponentName
  , confInstId :: ComponentId
  }
  deriving (Eq, Ord, Generic)

By default, I'd like ConfiguredId to render as just its ComponentId, since that canonically identifies a ConfiguredId. However, if I am debugging corrupt ConfiguredId metadata, I might also like to display the confSrcId, confCompName. In GHC, the way we do this is we render x by default, but x{some metadata} if you turn on -dppr-debug. I think this behavior is quite useful and I'd like it to apply to Cabal too.

Cabal-like syntax is compact, but it doesn't compose well.

With pretty-printing, this is not true; indentation is all you need. To take your example of [PackageDescription], with pretty-printing it with the default list renderer will cause it to look something like this:

[name: foo
 field: 1
 field: 2,
 name: bar
 field: 3
 field: 4]

Which is basically as good as you're going to get with Haskell-style list syntax with multi-line elements. Or you be a bit more clever and have a different format:

- name: foo
  field: 1
  field: 2
- name: bar
  field: 3
  field: 4

Indentation is great! And it is much easier to parse than parentheses.

P.S. Also, why do I care so much about conciseness? Because if I'm debugging I'm going to have to read all of this information, and it is far easier to understand what is going on if a single ElaboratedConfiguredPackage can fit in a few lines, rather than multiple pages.

[23Skidoo]

LGTM.

[23Skidoo]

Merged, thanks!

[23Skidoo]

Cherry-picked the first four commits into 2.0.