Opened 18 months ago

Last modified 2 months ago

#8226 new task

Remove the old style -- # Haddock comments.

Reported by: Fuuzetsu Owned by: adinapoli
Priority: normal Milestone: 7.12.1
Component: Compiler Version: 7.7
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

Haddock 0.x supported -- # style comments for Haddock pragma and while this seems to have been long forgotten and not documented, it's still kicking in the lexer. I think this is a good time to get rid of these all together.

Relevant Haddock ticket http://trac.haskell.org/haddock/ticket/171

I'll hopefully send in patches removing these from the lexer/parser tomorrow, if all goes well (unless anyone feels eager to do it, then please, you're most welcome to do so!).

Attachments (1)

0001-Tentative-fix-for-8226-remove-Haddock-0.x-style-comm.patch (882 bytes) - added by adinapoli 9 months ago.
Alfredo's proposed fix

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 months ago by thoughtpolice

  • Milestone changed from 7.8.3 to 7.10.1

Moving to 7.10.1

Changed 9 months ago by adinapoli

Alfredo's proposed fix

comment:2 Changed 9 months ago by adinapoli

(Rewriting comment that the tracker wiped :'( )

In a nutshell this is my first session of GHC hacking ever, so please be patient if - as I'm certain - my patch doesn't fully fix the issue or is going in the wrong direction. After all, I needed to start somewhere, nobody was born knowing the GHC codebase :)

I have manually tested the fix modifying slightly the testcase attached to the relevant haddock ticket:

http://trac.haskell.org/haddock/ticket/171

{- # LANGUAGE RecordWildCards      #-}

module Fail where

data Foo = Foo {
    a :: String
  , b :: Int
  }

showFoo Foo{..} = a

main :: IO ()
main = print $ showFoo (Foo "a" 10)

As pointed out that one is _not_ a pragma (mind the space), so it needs to be interpreted as a normal comment, but Haddock chokes on it:

-- pre-fix
[nix-shell:ghc]$ ghc -haddock Fail.hs
[1 of 1] Compiling Main             ( Fail.hs, Fail.o )

Fail.hs:1:4: parse error on input ‘#’

But after my patch it seems to accept it:

[nix-shell:ghc]$ ghc-dev -haddock Fail.hs
[1 of 1] Compiling Fail             ( Fail.hs, Fail.o )

Fail.hs:10:9:
    Illegal `..' in record pattern
    Use RecordWildCards to permit this

I have ran "validate" before and after the patch and it seems no regression was introduces, but being the Lexer such a delicate part I really want to be cautious with it :)

Not really sure if this ticket is fully completed, because I have found the Haddock 0.x style comments in at least one other place in the lexer, but I wasn't sure if it's something we need to get rid of:

https://github.com/ghc/ghc/blob/master/compiler/parser/Lexer.x#L285
https://github.com/ghc/ghc/blob/master/compiler/parser/Lexer.x#L295

Not sure though if they are Haddock specific and if they are firing at all.
Thanks guys,
hope I'm on the right track :)

Alfredo

Last edited 9 months ago by adinapoli (previous) (diff)

comment:3 Changed 9 months ago by simonpj

It'd be great if a Haddock-aware person was able to take a look. Thanks.

Simon

comment:4 Changed 9 months ago by adinapoli

  • Status changed from new to patch

comment:5 Changed 9 months ago by Fuuzetsu

Sorry I fell behind a bit on reading GHC tickets.

Yes adinapoli, those should also be removed, we do not use --# for anything and it served the same purpose as {-# as far as I know.

Also the data type used to store that type of comment should be removed:

https://github.com/ghc/ghc/blob/master/compiler/parser/Lexer.x#L625

FTR the Haddock ticket is now tracked at https://github.com/haskell/haddock/issues/171

Thanks for looking into this, feel free to poke me on IRC/e-mail if you need more immediate review.

comment:6 Changed 9 months ago by adinapoli

  • Owner set to adinapoli

comment:7 Changed 9 months ago by adinapoli

Hi guys,

I haven't forgot, is that I'm making basically no progress on it.
After Fuuzetsu's suggestion of removing the old data structure which held the 0.x style comments, I stumbled upon a series of difficulties, which are, unordered, the result of different edits on the Lexer;

  • Removing the data structure caused immediately GHC to complain on old style comments which are actually leftovers, an example is inside libraries/ghc-prim/GHC/Classes.hs. I couldn't understand from the sole documentation what to do, but it seems to me that, as Fuuzetsu said, they can be removed because they are equivalent to a pragma (in the example -- #hide == {-# OPTIONS_HADDOCK hide #-}. Unfortunately the lexer chokes in trying to parse "-- #hide" once the data structure has been removed, even though, theoretically, should be possible to modify the Lexer to just treat them as a normal comment ; after all the only thing which prevents this to happen, I believe, is the "isNormalComment" function. Unfortunately I can't modify it as per my first patch (aka removing the "#") because this will cause GHC to interpret RULES as normal comments (I discovered this thanks to an insight from SPJ).

Sorry for the noise, I thought it was worth documenting my efforts my myself in the future :)
If someone has some insights, please shout :)
Alfredo

comment:8 follow-up: Changed 9 months ago by Fuuzetsu

Hm, I'm unsure why this is proving difficult to implement. Looking at the lexer, it seems that it should be enough to:

  • Remove # from any sections lexing Haddock comment so sections like [$docSym \#] become just [$docSym] or whatever the appropriate syntax is there.
  • in <options_prags> remove the lexing of -- # all together.
  • Remove ITdocOptionsOld.
  • inside isNormalComment, I'm unsure exactly how to proceed. You say removing # from notFollowedByDocOrPragma stops RULES from working. In that case I say keep it there as it is now, in the end it does not matter because we'll stop treating it as a Haddock thing anyway. It will now simply check whether a # comment is special for RULES instead of for both RULES and Haddock. This is an assumption however so this might be where I'm going wrong.
  • withLexedDocType remove the case for '#': if we removed the case for -- # in <options_prags> then we should never enter withLexedDocType through multiline_doc_comment at least as far as I can see.
  • Remove the case in for ITdocOptionsOld in getOptions' in HeaderInfo.hs.

I think this should be enough to allow us to not treat either {- # nor -- # as a Haddock thing while preserving the behaviour of everything else as it is now. Am I missing something?

PS: It'd be great if you could create a wiki page (either on GHC wiki or NixOS wiki) on your workflow on GHC hacking with nix.

comment:9 Changed 9 months ago by Fuuzetsu

  • Owner adinapoli deleted
  • Status changed from patch to new

comment:10 Changed 9 months ago by Fuuzetsu

  • Owner set to adinapoli

comment:11 in reply to: ↑ 8 Changed 9 months ago by adinapoli

Replying to Fuuzetsu:

Hm, I'm unsure why this is proving difficult to implement.

Oh, it's easy, that's because it's my first foray into GHC and its Lexer hacking ever :)

  • Remove # from any sections lexing Haddock comment so sections like [$docSym \#] become just [$docSym] or whatever the appropriate syntax is there.
  • in <options_prags> remove the lexing of -- # all together.
  • Remove ITdocOptionsOld.
  • inside isNormalComment, I'm unsure exactly how to proceed. You say removing # from notFollowedByDocOrPragma stops RULES from working. In that case I say keep it there as it is now, in the end it does not matter because we'll stop treating it as a Haddock thing anyway. It will now simply check whether a # comment is special for RULES instead of for both RULES and Haddock. This is an assumption however so this might be where I'm going wrong.
  • withLexedDocType remove the case for '#': if we removed the case for -- # in <options_prags> then we should never enter withLexedDocType through multiline_doc_comment at least as far as I can see.
  • Remove the case in for ITdocOptionsOld in getOptions' in HeaderInfo.hs.

Apart from the docSym section I already tried was you said, with no luck.
This is a gist to a patch which follows your guidelines; unless I have tweaked the wrong things, it should roughly do what you listed:

https://gist.github.com/adinapoli/a08683a32412c8fddb43

This validates fine but it doesn't fix the problem! If I try the "Fail.hs" example attached in the ticket, the Lexer still chokes on that comment which "looks like a pragma". But originally my fix on "isNormalComment" fixed that, but amusingly if I tweak that function, as part of this patch, it triggers the problem I've reported, aka the RULES gets interpreted as comments and validate fails due to warnings for unutilized functions.
Where's the flaw in my reasoning? :)

PS: It'd be great if you could create a wiki page (either on GHC wiki or NixOS wiki) on your workflow on GHC hacking with nix.

More than happy to do so, didn't think was something people would have been interested in :)

Alfredo

comment:12 Changed 2 months ago by thoughtpolice

  • Milestone changed from 7.10.1 to 7.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

Note: See TracTickets for help on using tickets.