Duplicate documentation on records

[ghc-mirror]

Original reporter: danburton.email@

Haddock has strange behavior for occurrences when you document the same field on different constructors of a record type: it includes all of the documentation every time it displays that field.

Here's one quick example that can reproduce the issue:

cabal update && cabal unpack QuickCheck-2.4.2 && cd QuickCheck-2.4.2 && cabal configure && cabal haddock && firefox ./dist/doc/html/QuickCheck/Test-QuickCheck.html#t:Result

[ghc-mirror]

Original reporter: fuuzetsu@

Interesting. I'll have a look.

[ghc-mirror]

Original reporter: fuuzetsu@

I have investigated further but I'm slowly running out of steam. Here
are my findings:

First of all, here's the test file I've been using:

module Bug195 where

data T =
  A
  { someField :: () -- ^ Doc for someField of A
  , someOtherField :: () -- ^ Doc for someOtherField of A
  }
  |
  B
    { someField :: () -- ^ Doc for someField of B
    , someOtherField :: () -- ^ Doc for someOtherField of B
    }
  |
  C
    { someField :: () -- ^ Doc for someField of C
    , someOtherField :: () -- ^ Doc for someOtherField of C
    }

We properly get the documentation from GHC so it's not something
inherently broken on their side. Here's some debug output using my
test file showing more or less what the structure looks like

    data main:Bug195.T{tc rmY} {}
      = {/home/shana/programming/haddock/html-test/src/Bug195.hs:(4,3)-(7,3)}
        {/home/shana/programming/haddock/html-test/src/Bug195.hs:4:3}
        main:Bug195.A{d ro1} {{/home/shana/programming/haddock/html-test/src/Bug195.hs:5:5-13}
                              main:Bug195.someField{v ro2} :: {/home/shana/programming/haddock/html-test/src/Bug195.hs:5:18-19}
                                                              () {/home/shana/programming/haddock/html-test/src/Bug195.hs:5:21-47}
                                                                  Doc for someField of A,
                              {/home/shana/programming/haddock/html-test/src/Bug195.hs:6:5-18}
                              main:Bug195.someOtherField{v ro3} :: {/home/shana/programming/haddock/html-test/src/Bug195.hs:6:23-24}
                                                                   () {/home/shana/programming/haddock/html-test/src/Bug195.hs:6:26-57}
                                                                       Doc for someOtherField of A} |
        {/home/shana/programming/haddock/html-test/src/Bug195.hs:(9,3)-(12,5)}
        {/home/shana/programming/haddock/html-test/src/Bug195.hs:9:3}
        main:Bug195.B{d ro0} {{/home/shana/programming/haddock/html-test/src/Bug195.hs:10:7-15}
                              main:Bug195.someField{v ro2} :: {/home/shana/programming/haddock/html-test/src/Bug195.hs:10:20-21}
                                                              () {/home/shana/programming/haddock/html-test/src/Bug195.hs:10:23-49}
                                                                  Doc for someField of B,
                              {/home/shana/programming/haddock/html-test/src/Bug195.hs:11:7-20}
                              main:Bug195.someOtherField{v ro3} :: {/home/shana/programming/haddock/html-test/src/Bug195.hs:11:25-26}
                                                                   () {/home/shana/programming/haddock/html-test/src/Bug195.hs:11:28-59}
                                                                       Doc for someOtherField of B} |
        {/home/shana/programming/haddock/html-test/src/Bug195.hs:(14,3)-(17,5)}
        {/home/shana/programming/haddock/html-test/src/Bug195.hs:14:3}
        main:Bug195.C{d rnZ} {{/home/shana/programming/haddock/html-test/src/Bug195.hs:15:7-15}
                              main:Bug195.someField{v ro2} :: {/home/shana/programming/haddock/html-test/src/Bug195.hs:15:20-21}
                                                              () {/home/shana/programming/haddock/html-test/src/Bug195.hs:15:23-49}
                                                                  Doc for someField of C,
                              {/home/shana/programming/haddock/html-test/src/Bug195.hs:16:7-20}
                              main:Bug195.someOtherField{v ro3} :: {/home/shana/programming/haddock/html-test/src/Bug195.hs:16:25-26}
                                                                   () {/home/shana/programming/haddock/html-test/src/Bug195.hs:16:28-59}
                                                                       Doc for someOtherField of C}

I have tracked the problem all the way from the rendering, up through
main function and down to the interface creation. In fact, the problem
is in {mkMaps} function in Create.hs. This function creates
documentation maps that we later use to refer to things. One of these
maps is a DocMap which is an alias for Map Name (Doc a).
Name is a GHC type and it contains some interesting things, such
as the original location of the name in question.

In mkMaps We use a helper function unhelpfully called f
which creates our DocMap from [[(Name, Doc Name)]]. To get
the appropriate value of this type, we call subordinates on a
list of declarations [(LHsDecl Name, [HsDocString])] which we
obtain earlier in the file and is passed in as an argument. The debug
blob above shows more or less what it looks like for my test file. The
type of subordinates is InstMap -> HsDecl Name -> [(Name, [HsDocString], Map Int HsDocString)]. We're not too worried about
InstMap here as that's always empty in our test case. What it
effectively does is takes HsDecl which is precisely our sole
LHsDecl we were just talking about with its location information
removed and returns a list of names and their appropriate docs. The
idea behind the function is that it returns things ‘under’ it: in this
case, the fields under each constructor. This is also where it goes
wrong: amongst other things, subordinates drops the
Location information that is wrapped around each field
Name. In fact, here's the debug output of one of the helper
functions in subordinates where it happens:

    [(main:Bug195.someField{v ro2}, [ Doc for someField of A], []),
     (main:Bug195.someOtherField{v ro3},
      [ Doc for someOtherField of A],
      []),
     (main:Bug195.someField{v ro2}, [ Doc for someField of B], []),
     (main:Bug195.someOtherField{v ro3},
      [ Doc for someOtherField of B],
      []),
     (main:Bug195.someField{v ro2}, [ Doc for someField of C], []),
     (main:Bug195.someOtherField{v ro3},
      [ Doc for someOtherField of C],
      [])]

keeping this in mind, we refer back to mkMaps and our f
function that actually creates the maps for us. Here it is in its
entirety:

    f :: (Ord a, Monoid b) => [[(a, b)]] -> Map a b
    f = M.fromListWith (<>) . concat

Very quickly we should be able to spot what's wrong. In fact, here's
the debug output of f after running on massaged result of
subordinates to do the comment parsing:

[(main:Bug195.someField{v ro2},
       DocAppend (DocParagraph (DocString "Doc for someField of C")) (DocAppend (DocParagraph (DocString "Doc for someField of B")) (DocParagraph (DocString "Doc for someField of A")))),
      (main:Bug195.someOtherField{v ro3},
       DocAppend (DocParagraph (DocString "Doc for someOtherField of C")) (DocAppend (DocParagraph (DocString "Doc for someOtherField of B")) (DocParagraph (DocString "Doc for someOtherField of A"))))]

and here's our problem! When creating the map, f sees that all
the Names it's given look the same and they get collapsed into a
single Name in the DocMap.

I think not all is lost however. Perhaps we could keep the
Located tag in subordinates so that f can tell the
Names apart. Here's the output of the same helper without the
Located tag stripped so the information is definitely there:

    [({/home/shana/programming/haddock/html-test/src/Bug195.hs:5:5-13}
      main:Bug195.someField{v ro2},
      [ Doc for someField of A],
      []),
     ({/home/shana/programming/haddock/html-test/src/Bug195.hs:6:5-18}
      main:Bug195.someOtherField{v ro3},
      [ Doc for someOtherField of A],
      []),
     ({/home/shana/programming/haddock/html-test/src/Bug195.hs:10:7-15}
      main:Bug195.someField{v ro2},
      [ Doc for someField of B],
      []),
     ({/home/shana/programming/haddock/html-test/src/Bug195.hs:11:7-20}
      main:Bug195.someOtherField{v ro3},
      [ Doc for someOtherField of B],
      []),
     ({/home/shana/programming/haddock/html-test/src/Bug195.hs:15:7-15}
      main:Bug195.someField{v ro2},
      [ Doc for someField of C],
      []),
     ({/home/shana/programming/haddock/html-test/src/Bug195.hs:16:7-20}
      main:Bug195.someOtherField{v ro3},
      [ Doc for someOtherField of C],
      [])]

I think the next step is investigating whether we can propagate this
information up. I worry that any parts of Haddock currently looking
things up by Name in this map might not have access to the
Located tag which would make indexing into this new map
impossible and we'd be stuck. I have not attempted it yet as many
things use the DocMap and it's quite a trip around the code to
adapt it. I'm quite out of steam after debugging this for 2 days. I'll
give it more time tomorrow but of course, anyone is welcome to fix it
or help out!

PS: I'm using GHC's Outputable to get the debug output. I'm using
pprTrace to print and I have defined a lot of ad-hoc Outputable
instances during debugging. I'm posting the parts which I put in
Types.hs below if anyone wants to have a go.

To get the actual comment contents of the documentation strings,
you'll need GHC no older than ~15 hours or all you'll get is the
string "".

instance O.Outputable Interface where
  ppr (Interface mod orig info doc rndoc opt declm docm argm rndocm rnargm subm
       exp rnexp exports vexpots moda inst faminst cov wm) =
    O.vcat
    [ O.text "ifaceMod" O.$+$ O.ppr mod
    , O.text "ifaceOrigFilename" O.$+$ O.ppr orig
    , O.text "ifaceInfo" O.$+$ O.ppr info
    , O.text "ifaceDoc" O.$+$ O.ppr doc
    , O.text "ifaceRnDoc" O.$+$ O.ppr rndoc
    , O.text "ifaceOptions" O.$+$ O.ppr opt
    , O.text "ifaceDeclMap" O.$+$ O.ppr declm
    , O.text "ifaceDocMap" O.$+$ O.ppr docm
    , O.text "ifaceArgMap" O.$+$ O.ppr argm
    , O.text "ifaceRnDocMap" O.$+$ O.ppr rndocm
    , O.text "ifaceRnArgMap" O.$+$ O.ppr rnargm
    , O.text "ifaceSubMap" O.$+$ O.ppr subm
    , O.text "ifaceExportItems" O.$+$ O.ppr exp
    , O.text "ifaceRnExportItems" O.$+$ O.ppr rnexp
    , O.text "ifaceExports" O.$+$ O.ppr exports
    , O.text "ifaceVisibleExports" O.$+$ O.ppr vexpots
    , O.text "ifaceModuleAliases" O.$+$ O.ppr moda
    , O.text "ifaceInstances" O.$+$ O.ppr inst
    , O.text "ifaceFamInstances" O.$+$ O.ppr faminst
    , O.text "ifaceHaddockCoverage" O.$+$ O.ppr cov
    , O.text "ifaceWarningMap" O.$+$ O.ppr wm
    ]


instance O.Outputable (HaddockModInfo a) where
  ppr _ = O.text "HaddockModInfo"

instance Show Name where
  show _ = "Name"

instance O.Outputable DocOption where
  ppr = O.text . show

instance (Show a, O.OutputableBndr a) => O.Outputable (ExportItem a) where
  ppr (ExportDecl decl doc subdocs insts) =
    O.text "ExportDecl" O.<+> {-O.ppr decl-} O.text "decl" O.<+> O.ppr doc
    O.<+> O.ppr subdocs O.<+> {- O.ppr insts -} O.text "insts"

  ppr _ = O.text "unknown ExportItem ppr"

instance O.OutputableBndr a => O.Outputable (ConDeclField a) where
  ppr (ConDeclField fldn fldt flddoc) = O.text "ConcDeclField"
                                        O.<+> O.vcat [ O.ppr fldn
                                                     , O.ppr fldt
                                                     , O.ppr $ fmap (fmap (\(HsDocString x) -> x)) flddoc ]

instance O.Outputable DocName where
  ppr (Documented n m) = O.text "Documented" O.<+> O.ppr n O.<+> O.ppr m
  ppr (Undocumented n) = O.text "Documented" O.<+> O.ppr n

instance O.OutputableBndr DocName where
  pprPrefixOcc = O.ppr
  pprInfixOcc = O.ppr

instance O.Outputable O.SDoc where
  ppr = id

-- instance O.Outputable a => Show a where
--   show = O.showSDoc undefined . O.ppr

deriving instance Show a => Show (Header a)
deriving instance Show a => Show (Doc a)
deriving instance Eq a => Eq (Header a)
deriving instance Eq a => Eq (Doc a)

instance IsString RdrName where
  fromString = mkVarUnqual . fsLit

instance IsString (Doc RdrName) where
  fromString = DocString

instance IsString a => IsString (Maybe a) where
  fromString = Just . fromString

-- deriving instance Show a => Show (Header a)
-- -- deriving instance Show a => Show (Doc a)
instance Show DocName where
  show (Documented n m) = "Documented" ++ foo n ++ show m
  show (Undocumented n) = "Undocumented"  ++ foo n

foo = O.showSDoc undefined . O.ppr

instance Show Module where
  show _ = "Module"

instance Show ModuleName where
  show _ = "ModuleName"

instance Show OccName where
  show = O.showSDoc undefined . O.ppr

-- instance O.Outputable a => Show (Doc a) where
--   show = O.showSDoc undefined . O.ppr
instance Show a => O.Outputable (Documentation a) where
  ppr (Documentation a b) = O.text "Documentation" O.<+> O.text (show a) O.<+> O.text (show b)

instance Show a => O.Outputable (Doc a) where
  ppr = O.text . show

instance O.Outputable InstalledInterface where
  ppr (InstalledInterface mod info dm am exp ve op sub) =
    O.text "InstalledInterface" O.$+$ vcat [ O.ppr mod
                                           , O.ppr info
                                           , O.ppr dm
                                           , O.ppr am
                                           , O.ppr exp
                                           , O.ppr ve
                                           , O.ppr op
                                           , O.ppr sub
                                           ]

[ghc-mirror]

Original reporter: fuuzetsu@

After a brief chat on IRC, we decided that only rendering the documentation of the first field present is a fair work-around: after all, all these fields are actually one and the same function. Arguably, the reported behaviour is actually the most reasonable if we fixed the order at which the documentation gets joined up. Note that even if the first field has no documentation, that's what will be used.

The new behaviour is not the most intuitive to the user but the effort required to provide the ‘expected’ rendering far outweighs the benefits.

I'll mark this as fixed although quite regrettably. Perhaps it can be revisited in the future if someone files a ticket and perhaps provides patches.