Ticket #69 (closed defect: fixed)

Opened 6 years ago

Last modified 23 months ago

Errors with class members

Reported by: NeilMitchell Owned by:
Priority: major Milestone:
Version: 2.5.0 Keywords:
Cc: ndmitchell@…, marlowsd@…

Description

Given the files:

-- Boo.hs
module Boo where

class Boo a  where
    boo1, boo2, boo3 :: a
    boo4 :: a
-- Test.hs
module Test(Boo, boo1, boo2, boo3, boo4) where
import Boo
$ haddock --html Test.hs Boo.hs

I would expect Haddock to infer that there is one class with 4 methods. Instead it infers that there is one class (Boo), two methods (boo1, boo4) and two top-level functions (boo1, boo4). Haddock seems to get confused by the boo1,boo2,boo3 comma separated identifiers. The incorrect top-level functions may be a symptom of the same bug, or may be a separate bug.

This issue causes the GHC documentation not to include even, {{{tan} and lots of others. It also mess up Hoogle. I'd mark this as priority blocker!

All using the latest HEAD Haddock with GHC 6.10.1.

Change History

  Changed 6 years ago by anonymous

It seems that Haddock generates documentation with tan after all, but still not even. However, a fresh run looses the tan for me...

  Changed 6 years ago by ross

See also #67.

  Changed 6 years ago by waern

  • milestone changed from 2.5.0 to 2.4.2

  Changed 6 years ago by waern

#67 fixes the problem with comma separated identifiers.

The other issue of incorrect top-level functions seems to be a behavior present also in the 0.x versions of Haddock. Subordinate decls exported separetely show up separately in the docs even if the parent decl is also exported.

We should fix this so that whenever we export both a parent decl and a subordinate decl, that decl should be shown under the parent decl only. This regardless of whether the parent decl is abstract or not.

  Changed 6 years ago by anonymous

  • cc marlowsd@… added

Ok, after implementing a fix, I run the test suite and find that Bug7.hs fails. :-)

The documentation for Bug7 says:

data B

.. with its field, but the field is named separately in the export list (should still be visible as a field name)

@Simon: what is the reason for showing the field as a top-level function in this case, when it is already shown in the record declaration?

follow-up: ↓ 7   Changed 6 years ago by waern

Should have pasted the source code instead, here it comes:

-- |
-- .. with its field, but the field is named separately in the export list
-- (should still be visible as a field name)
data B = B { b :: Int }

(And it's from Bug6, not Bug7).

in reply to: ↑ 6 ; follow-up: ↓ 8   Changed 6 years ago by waern

Reply from Simon Marlow (off-Trac):

"The idea is that Haddock simply documents each entity in the export list in the order that the entities occur. If you have duplicates in the export list, then you get duplicate entries in the documentation too. Record fields and class methods aren't any different from other entities in this respect. I think this is consistent behaviour, and if you don't want it you can always change the export list."

in reply to: ↑ 7 ; follow-up: ↓ 9   Changed 6 years ago by waern

Replying to waern:

Reply from Simon Marlow (off-Trac): "The idea is that Haddock simply documents each entity in the export list in the order that the entities occur. If you have duplicates in the export list, then you get duplicate entries in the documentation too. Record fields and class methods aren't any different from other entities in this respect. I think this is consistent behaviour, and if you don't want it you can always change the export list."

I agree with this, but in this particular case, the record fields are not duplicated in the export list, because the class is exported abstractly. When exporting a class abstractly, and its fields separately, the fields are added to the class definition in the documentation, *and* shown at the top level.

in reply to: ↑ 8 ; follow-up: ↓ 10   Changed 6 years ago by simonmar

Replying to waern:

I agree with this, but in this particular case, the record fields are not duplicated in the export list, because the class is exported abstractly. When exporting a class abstractly, and its fields separately, the fields are added to the class definition in the documentation, *and* shown at the top level.

Oh, in that case it's a bug then. Sorry for missing the point!

in reply to: ↑ 9   Changed 6 years ago by simonmar

Replying to simonmar:

Replying to waern:

I agree with this, but in this particular case, the record fields are not duplicated in the export list, because the class is exported abstractly. When exporting a class abstractly, and its fields separately, the fields are added to the class definition in the documentation, *and* shown at the top level.

Oh, in that case it's a bug then. Sorry for missing the point!

(simon thinks a bit more...) So I think the point illustrated by Bug6.hs is that if you export a record field separately from the record constructor, clients can still use the field in pattern matching and so on. Hence we should make it visible in the documentation that this is a record field and not just a function. Similarly for class methods: if the methods are exported separately from the class, this might enable clients to make instances of the class, if enough of the methods are visible.

So now I think that Haddock should emit a warning in this case. Perhaps GHC itself should emit a warning too. The user should change the export list to read

module M (B(b,B), ...)

because that is semantically equivalent to

module M (B(B), b, ...)

but is better for documentation.

Given the second version, whether Haddock generates two copies of the documentation for b or just one, I don't really mind, as long as it emits a warning to say what it is doing.

  Changed 5 years ago by waern

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

I have added the warning and made Haddock just generate one copy for b.

  Changed 5 years ago by waern

  • status changed from closed to reopened
  • resolution fixed deleted

Oops, the patch that I wrote has a bug. We give the warning message in too many cases. I will keep this ticket open until the bug has been fixed.

  Changed 5 years ago by waern

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

Fixed by this patch:

Fri Feb 27 22:37:20 CET 2009  David Waern <david.waern@gmail.com>
  * Bug fix
  We tried to filter out subordinates that were already exported through their parent.
  
  This didn't work properly since we were in some cases looking at the
  grand-parent and not the parent.  We now properly compute all the parent-child
  relations of a declaration, and use this information to get the parent of a
  subordinate.
  
  We also didn't consider record fields with multiple parents. This is now
  handled correctly.
  
  We don't currently support separately exported associated types. But when we
  do, they should be handled correctly by this process too.
  
  Also slightly improved the warning message that we give when filtering out
  subordinates.

  Changed 23 months ago by anonymous

  • milestone 2.4.2 deleted

Milestone 2.4.2 deleted

Note: See TracTickets for help on using tickets.