Ticket #195 (closed defect: fixed)

Opened 2 years ago

Last modified 6 months ago

Duplicate documentation on records

Reported by: DanBurton Owned by: Fūzetsu
Priority: major Milestone:
Version: 2.14.0 Keywords:
Cc:

Description

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

Change History

Changed 2 years ago by anonymous

  • milestone 2.10.0 deleted

Milestone 2.10.0 deleted

Changed 6 months ago by Fūzetsu

  • owner set to Fūzetsu
  • status changed from new to assigned
  • version changed from 2.9.4 to 2.14.0

Interesting. I'll have a look.

Changed 6 months ago by Fūzetsu

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 "<documentation comment>".

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
                                           ]

Changed 6 months ago by Fūzetsu

  • status changed from assigned to closed
  • resolution set to fixed

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.

Note: See TracTickets for help on using tickets.