Warn instead of stopping on misplaced Haddock comments

Given a simple module like

module H where

data F = F () -- ^ Comment for first type argument
           ()

and trying to run Haddock on it, we'll get back

/tmp/H.hs:4:12: parse error on input ‘(’

The error message is rather sub-par in this scenario. It'd be great if we could instead print a warning saying that a Haddock comment is unexpected in that position and then continue on parsing.

Best case scenario would be a more informative message such as “Documenting each constructor argument is not supported” but that might be quite a bit harder.

Filing on GHC Trac as it's the parser that needs changing I believe.

Trac metadata

Trac field	Value
Version	7.9
Type	FeatureRequest
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	Compiler (Parser)
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

changed weight to 5

added Tfeature request Trac import parser labels

The following break GHC's validate (see https://phabricator.haskell.org/harbormaster/build/1562/)

-- * base
import Control.Monad    ( unless, liftM )
import GHC.Exts
import Data.Char
import Control.Monad    ( mplus )

-- * compiler/hsSyn
import HsSyn

I ended up having to remove '*' from the comment for it to work:

-- base
import Control.Monad    ( unless, liftM )
import GHC.Exts
import Data.Char
import Control.Monad    ( mplus )

-- compiler/hsSyn
import HsSyn

This seems to be the relevant part in Lexer.x:

"-- " ~[$docsym \#] .* { lineCommentToken }
"--" [^$symbol \ ] .* { lineCommentToken }

-- Next, match Haddock comments if no -haddock flag

"-- " [$docsym \#] .* / { ifExtension (not . haddockEnabled) } { lineCommentToken }

-- Now, when we've matched comments that begin with 2 dashes and continue
-- with a different character, we need to match comments that begin with three
-- or more dashes (which clearly can't be Haddock comments). We only need to
-- make sure that the first non-dash character isn't a symbol, and munch the
-- rest of the line.

"---"\-* ~$symbol .* { lineCommentToken }

-- Since the previous rules all match dashes followed by at least one
-- character, we also need to match a whole line filled with just dashes.

"--"\-* / { atEOL } { lineCommentToken }

-- We need this rule since none of the other single line comment rules
-- actually match this case.

"-- " / { atEOL } { lineCommentToken }

And it seems that there is no case for when haddockEnabled is true. What should be the action for such rule?

I have a fix for this in https://phabricator.haskell.org/D452. However, I just realized that I forgot to create a specific option for this warning. But does this even need an option??

Trac metadata

Trac field	Value
Differential revisions	→ D452

I think this should be enabled with -Wall, even if haddock is not enabled. That way, when I'm developing, but before trying to release and build docs, I can learn about bad Haddock comments.

Replying to [ticket:8944#comment:90655 rodlogic]:

I have a fix for this in https://phabricator.haskell.org/D452. However, I just realized that I forgot to create a specific option for this warning. But does this even need an option??

I did spend some time trying to make the following case work, which is a similar to the one described in the summary:

import Data.Maybe
-- * whatever
import Data.Functor

Parsing the above with the haddock flag turned on will lead to the following error:

a1.hs:3:1: parse error on input ‘import’

When the haddock flag is turned on, the comment line is scanned as a ITdocSection token instead of a ITlineComment. However, instead of being ignored by the lexer an ITdocSection is returned to the parser as a token. The problem is that the import rules are not expecting any documentation tokens and the parsing fails.

So fixing this requires modifying the import rule to be similar to how the export rules are implemented, i.e. explicitly taking into account the different haddock tokens.

I just find it a bit strange that both the parser and lexer need to know about haddock comments. Any changes to haddock (e.g. introducing a new haddock comment type) would require changing the GHC's lexer and the parser! I was expecting to see the lexer to generate a generic single or multiline comment token that would be incorporated into the AST by the parser, and then haddock would be able to further process these comments based on it's own rules.

Later this week I will see if I can address the summary case since it may be simpler, but it seems that it would be a good idea to generalize the parsing of comments and make GHC's parser/lexer a bit more independent of Haddock (in a different ticket). Alanz changes to the AST may be a good first step here and I will take a closer look there too.

After a some time getting my feet wet with the Lexer.x and Parser.y, I am coming to the conclusion that this is not a trivial change and involves more changes to the parser than I would like to or am able to handle at this point. I also noticed that #8822 (closed) may also be stuck based on the same/similar difficulty. The difficulty in debugging the parser or the somewhat long change/compile cycles also don't help.

I agree that this is a harder nut to crack than it initially appears.

But, I'm surprised about the long build times. Were you using make 2 in the ghc directory?

I didn't know that make 2 existed, thanks for the tip.

There is one thing that bothers me, though. It seems that it is going to be quite hard to get the parser behaving the same with/without the haddock flag (granted it may not be such an important problem). We'll need to change happy rules everywhere to make it accept the same source files. Wouldn't it make more sense to have comments in general (as opposed to haddock specific documentation attached to specific declarations) as first class AST nodes that can be included/excluded based on a general parse-comments flag? This way a parser plugin could be added to the pipeline, consume the AST with comments, and parse the comments however it wants based on whether it is before or after a declaration. And haddock could be completely independent from the GHC parser.

I understand that this may be a big change that is not worth pursuing at this point in time, but I am curious about what your opinion is.

Trac metadata

Trac field	Value
TypeOfFailure	OtherFailure → IncorrectWarning

It would be great to see this ticket revived: With the Hi Haddock changes, more people are going to enable -haddock by default.

IMHO this doesn't have to work perfectly in order to avoid most of the pain for -haddock users. Maybe it would be enough to update D452 to get an initial fix?

Patch by harpocrates in D5057.

Trac metadata

Trac field	Value
Differential revisions	D452 → D452, D5057

added Pnormal label

mentioned in issue #16972

I want to revive this ticket after failing to enable -haddock in the Nix haskell packages due to this issue, as many test suites would break. The motivation is that both ghcide and haskell-language-server rely on interface files for haddock metadata.

What happened to @harpocrates patch (D5057)? Can anyone retrieve it from phabricator along with any review comments?

@pepeiborra You are in luck! Although I gave up due to performance problems, @int-index independently took this up and his MR !2377 (closed) is (I think) pretty close to being merged.

mentioned in commit 1c26dc4c

mentioned in merge request !2377 (closed)

Thanks for directing my attention to this issue – I've added a dedicated test case for it.

assigned to @int-index

mentioned in commit 6a48d5b2

mentioned in commit 83c58077

mentioned in commit 4bda699c

mentioned in commit 19e80b9a

Fixed in 19e80b9a

closed