Support constructor Haddocks in more places
AbandonedPublic

Authored by harpocrates on Oct 15 2017, 9:47 PM.

Details

Summary

This adds support for adding Haddocks on individual non-record fields
of regular (and GADT) constructors. The following now parses just fine
with -haddock enabled:

data Foo
  = Baz             -- ^ doc on the `Baz` constructor
      Int           -- ^ doc on the `Int` field of `Baz`
      String        -- ^ doc on the `String` field of `Baz`

  | Int             -- ^ doc on the `Int` field of the `:*` constructor
      :*            -- ^ doc on the `:*` constructor
    String          -- ^ doc on the `String` field of the `:*` constructor

  | Boa             -- ^ doc on the `Boa` record constructor
      { y :: () }

The change is backwards compatible: if there is only one doc and it occurs
on the last field, it is lifted to apply to the whole constructor (as
before).

Test Plan

None yet

harpocrates created this revision.Oct 15 2017, 9:47 PM

Thank you for your contribution @harpocrates. I am not at all familiar with the parser. But I noted two general points.

compiler/parser/RdrHsSyn.hs
491

Please use lengthIs here. E.g. lengthIs [...] 1

499

Typos

RyanGlScott requested changes to this revision.Oct 16 2017, 8:17 AM
RyanGlScott added a subscriber: RyanGlScott.

This desperately needs some additional tests in the GHC testsuite to verify that it does what it claims.

This wouldn't require any changes on the GHC side, but it would really be nice to update the Haddock users' guide (source is here) to reflect all of these changes, as currently they're only explained in source code comments.

I'm also a bit unclear on some things, so perhaps the answers to these questions should also go into the Haddock users' guide:

  1. When exactly does "backwards compatibility mode" kick in? Is it only when there's a Haddock comment at the end of a constructor and no other Haddock comments? Or will it also kick in if there's incomplete information, such as in:
data Foo = MkFoo A -- | This has a comment
                 B -- This doesn't
                 C -- | This has a comment
  1. You briefly mention that this adds additional Haddock capabilities for GADT constructors, but I'm quite unclear on what the extent of the changes are. Does this allow for Haddock comments on GADT record fields?
data Foo where
  MkFoo :: { field1, field2 :: Int -- | Docs
           , field3 :: Double -- | Docs
           } -> Foo

Or on GADT return types?

data Foo where
  MkFoo :: Int -- | This is an Int
        -> Foo -- | This is a Foo
This revision now requires changes to proceed.Oct 16 2017, 8:17 AM

Thanks for the quick reviews.

This desperately needs some additional tests in the GHC testsuite to verify that it does what it claims.

Yep, I'll be adding some test cases shortly. I only just figured out where to put these test cases. I'm still new to this. :)

This wouldn't require any changes on the GHC side, but it would really be nice to update the Haddock users' guide (source is here) to reflect all of these changes, as currently they're only explained in source code comments.

Regarding the Haddock users's guide, I completely agree; I intend to follow up with a PR at the Haddock repo. There is already an old ticket in the Haddock tracker about this. That said, it shouldn't even be crucial that these happen one right after the other: I'll make the bold claim here that any existing code with Haddocks should still be parsed in the same way as before.

I'm also a bit unclear on some things, so perhaps the answers to these questions should also go into the Haddock users' guide:

I'll add test cases for your questions (and in time update the manual) but, may as well also answer here:

  1. "Backwards compatibility mode" kicks in only when there's a Haddock comment at the end of a constructor and no other Haddock comments. Incomplete information on anything more than at the end of the constructor disables that.
-- The doc comment on 'A' prevents the doc comment on 'C' from being applied to 'MkFoo'
data Foo = MkFoo A -- ^ This has a comment
                 B -- This doesn't
                 C -- ^ This has a comment
  1. Haddock comments on GADTs like the one you mention, have been supported for a while
-- These doc comments already work
data Foo where
MkFoo :: { field1, field2 :: Int -- ^ Docs for field1, field2
         , field3 :: Double      -- ^ Docs for field3
         } -> Foo
  1. Comments on GADT return types are supported. I wasn't sure whether or not to guard against those. It wouldn't be difficult to do so, but I thought it would be consistent to emulate the behaviour of Haddocks on function signatures. (Note that GADTs have never supported trailing -- ^ ... style Haddocks, so the return type doc causes no conflicts).
-- This is a 'foo' function signature Haddock GHC already accepts
mkFoo :: Int    -- ^ with an int...
      -> String -- ^ and a string...
      -> Foo    -- ^ you get a 'Foo'!

-- This would now be accepted on GADTs
data Foo where
  MkFoo :: Int    -- ^ with an int...
        -> String -- ^ and a string...
        -> Foo    -- ^ you get a 'Foo'!
  • Add some tests, address nits
harpocrates marked 2 inline comments as done.Oct 16 2017, 11:30 AM
harpocrates added inline comments.
compiler/parser/RdrHsSyn.hs
491

Cool. The stuff in compiler/utils is handy!

Yes, looks good to me. Thanks, @harpocrates!

bgamari accepted this revision.Oct 25 2017, 1:39 PM
bgamari requested changes to this revision.Nov 2 2017, 4:26 PM

That being said, let's hold off on merging this until the Haddock documentation has been updated so we don't lose track of it.

This revision now requires changes to proceed.Nov 2 2017, 4:26 PM
harpocrates marked an inline comment as done.

The changes to Haddock are nearly complete: see https://github.com/haskell/haddock/pull/709. Please feel free to provide feedback or suggestions!

I'm not planning on changing anything anymore at this point, only adding tests.


Given this code sample:

Haddocks before the PR:

Haddocks after the PR:

The change on the Haddock side is now complete. See the PR linked in the previous comment.

Anything more I should do for this to be merged @bgamari?

harpocrates updated this revision to Diff 14863.EditedNov 30 2017, 3:26 PM
  • Rebase

Alright. I now understand the three API annotation related failures I get. The [GADT decl discards annotations] note in the parser explains how the type production adds an annotation twice in different places since in some of the uses of that production (GADT constructors), the usual annotation would be lost.

My diff changes GADT constructors to use typedoc instead of type. Unsurprisingly, compared to type, I lose annotations. I can, of course, mimic the behaviour of the type production and add the problematic annotation twice. The problem is that then I end up with extra annotations elsewhere (everywhere typedoc is used).

I am unsure precisely what invariant annotations need to uphold.

My working hypothesis is that they simply need to contain enough information to reconstruct the initial source file. It is preferable, but not a hard requirement, that this information not be duplicated. If that is the case, it is simply a matter of moving the trick annotated by [GADT decl discards annotations] from type to typedoc (and then updating the tests output). Could someone confirm this makes sense?

  • Fix annotations

I have absolutely no clue why there is still a failing test. The test is about the exitcode obtained after running a compiled test case, while this diff is just supposed to be surface-level changes to the parser.

Help someone?

I have absolutely no clue why there is still a failing test. The test is about the exitcode obtained after running a compiled test case, while this diff is just supposed to be surface-level changes to the parser.

Help someone?

Looks fine on linux and windows so I would ignore it.

Right, this is certainly not your fault. The issue is being tracked as Trac #14538.

bgamari accepted this revision.Dec 11 2017, 2:03 PM

Yes, this looks good to me. Thanks, @harpocrates!

@bgamari Just checking in to see what the status is on merging this.

I'm asking because I'd like to get the considerably larger Haddock PR merged while it is still conflict free and because, being unfamiliar with the GHC workflow, I'm not sure what to expect at this point (for example: should I be regularly rebasing this diff?).

Merging this as we speak.

alexbiehl added a comment.EditedJan 3 2018, 3:54 AM

@bgamari will you merge this into 8.4? If yes I will make sure to merge the PR into corresponding haddock branch.

Unfortunately I am seeing T14588 fail with this patch (after rebasing). Do you think you could have a look , @harpocrates ?

@bgamari No problem. Should I go ahead and rebase then?

harpocrates added a comment.EditedJan 7 2018, 2:29 PM

@bgamari I've created https://phabricator.haskell.org/D4292 which is just these changes squashed together and on HEAD. My tests are still running, but everything in haddock and parser passes.

harpocrates abandoned this revision.Jan 13 2018, 10:13 AM

The diff in the previous comment has been merged.