Use the GHC lexer for the Hyperlinker backend #714

harpocrates · 2017-12-06T02:50:10Z

Lead up discussion is here. Moving my concerns here:

CPP is not handled in the GHC lexer, yet Haddock will have to handle it. Currently, the solution is to recover from all lexical errors by turning problematic lines into comments (and then restarting the lexer from after that line). That seems like an OK failure mode to me, but I think it should be discussed...
Language pragmas (actually all top-level pragmas) are not lexed as pragmas. This (probably) requires a GHC change. In the meantime, language pragmas would look like regular comments.

GHC's handling of top-level pragmas is a (IMHO) huge lexer-level hack to try to avoid loading the whole file and we are feeling that here. (If bytestring ever becomes a stage 0 package, Data.ByteString.Lazy is what that really needs - not a hand coded list-based parser.)

This reverts commit b605510. Conflicts: haddock-api/haddock-api.cabal haddock-api/src/Haddock/Interface.hs

Things now run using the GHC lexer. There are still - stray debug statements - unnecessary changes w.r.t. master

Things are looking good. quasiquotes in particular look beautiful: the TH ones (with Haskell source inside) colour/link their contents too! Haven't yet begun to check for possible performance problems.

The support for these is hackier - but no more hacky than the existing support.

harpocrates · 2017-12-06T05:31:04Z

Alright. I think I've actually found an acceptable workaround to my two points from the previous comment. In a nutshell, we will do no better than we did before for parsing CPP and pragmas.

we still identify CPP based on the suboptimal criteria that the line starts with #
we still assumes anything of the form {-# ... #-} is a pragma

These both sound like acceptable workarounds. We could probably do better for the second point, but it would require some non-trivial GHC lexer hacking - which is probably not worth it.

TODO before this is mergeable:

test performance
update tests

alexbiehl · 2017-12-06T07:11:22Z

Nice! Finally a chance to throw the custom lexer over board! This sounds reasonable! Did you run the hyperlinker tests? I think a lot of performance improvement in this area came with 740458a which eliminates an accidentally quadric look-up. Since we are working with GHCs SrcLoc and SrcSpan we can also eliminate the custom Position and Span from haddock, right? Alec Theriault <[email protected]> schrieb am Mi., 6. Dez. 2017, 06:31:

…

Alright. I think I've actually found an acceptable workaround to my two points from the previous comment. In a nutshell, we will do no better than we did before for parsing CPP and pragmas than we did before. - we still identify CPP based on the suboptimal criteria that the line starts with # - we still assumes anything of the form {-# ... #-} is a pragma These both sound like acceptable workarounds. We could probably do better for the second point, but it would require some non-trivial GHC lexer hacking - which is probably not worth it. TODO before this is mergeable: - test performance - update tests — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#714 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AByiiYfRKbJ-9vIj9TwcJbUgNovElxWJks5s9iaZgaJpZM4Q3SiI> .

The tests were in some cases altered: I consider the new output to be more correct than the old one....

Replaces 'Position' -> 'GHC.RealSrcLoc' and 'Span' -> 'GHC.RealSrcSpan'.

harpocrates · 2017-12-07T22:23:57Z

I think this is ready to merge. Sample of changes (before and after):

Here are the only caveats I can think of:

Less flexibility around failure. If the lexer fails on some input, the whole chunk of Haskell source (bounded by the nearest CPP lines) will be marked as "unknown". We could conceivably just fall back onto the lexer we had before, but that isn't currently implemented. I'd like to witness some failing chunk of valid Haskell before bothering to do this.
CPP is a hack. In a nutshell, the file gets split up into CPP chunks and non-CPP chunks. The claim is that the non-CPP chunks can be lexed just fine on their own. I'm not sure this claim always holds. Besides, our heuristics for separating CPP from non-CPP could be buggy.

Please do try this out before merging - I'd like more confidence about performance in particular. 😄

Thanks!

alexbiehl · 2017-12-08T14:27:45Z

Exciting stuff! Will try this at home!

Boy, the next haddock release is packed with new features and fixes. Awesome!

alexbiehl

Almost there! Just one more nit.

alexbiehl · 2017-12-10T18:45:38Z

haddock-api/src/Haddock/Backends/Hyperlinker/Parser.hs

+          hSrc   = concat [ hLine   | Right hLine  <- hLinesRight  ]
+          cppSrc = concat [ cppLine | Left cppLine <- cppLinesLeft ]
+
+      in case L.lexTokenStream (stringToStringBuffer hSrc) pos dflags of


IIUC for each chunk (that is code between some CPP markers) we call the lexer. For each chunk we call stringToStringBuffer. If the lexer fails we make the offending part into a comment and try again with the rest, create a new StringBuffer and call the lexer again. Calling stringToStringBuffer is pretty expensive and doing it over and over this way seems quadric.

I don't think that is happening. If the lexer fails, that chunk gets marked as one big "unknown" token and we move on (see the PFailed case - we just shove hSrc into one big token).

alexbiehl · 2017-12-10T20:23:00Z

Right! The result looks really good!

harpocrates · 2017-12-11T00:25:14Z

Thanks for reviewing - I really appreciate how responsive you've been!

* Start changing to use GHC lexer * better cpp * Change SrcSpan to RealSrcSpan * Remove error * Try to stop too many open files * wip * wip * Revert "wip" This reverts commit b605510. Conflicts: haddock-api/haddock-api.cabal haddock-api/src/Haddock/Interface.hs * Remove pointless 'caching' * Use dlist rather than lists when finding vars * Use a map rather than list * Delete bogus comment * Rebase followup Things now run using the GHC lexer. There are still - stray debug statements - unnecessary changes w.r.t. master * Cleaned up differences w.r.t. current Haddock HEAD Things are looking good. quasiquotes in particular look beautiful: the TH ones (with Haskell source inside) colour/link their contents too! Haven't yet begun to check for possible performance problems. * Support CPP and top-level pragmas The support for these is hackier - but no more hacky than the existing support. * Tests pass, CPP is better recognized The tests were in some cases altered: I consider the new output to be more correct than the old one.... * Fix shrinking of source without tabs in test * Replace 'Position'/'Span' with GHC counterparts Replaces 'Position' -> 'GHC.RealSrcLoc' and 'Span' -> 'GHC.RealSrcSpan'. * Nits * Forgot entry in .cabal * Update changelog

* Start changing to use GHC lexer * better cpp * Change SrcSpan to RealSrcSpan * Remove error * Try to stop too many open files * wip * wip * Revert "wip" This reverts commit b605510. Conflicts: haddock-api/haddock-api.cabal haddock-api/src/Haddock/Interface.hs * Remove pointless 'caching' * Use dlist rather than lists when finding vars * Use a map rather than list * Delete bogus comment * Rebase followup Things now run using the GHC lexer. There are still - stray debug statements - unnecessary changes w.r.t. master * Cleaned up differences w.r.t. current Haddock HEAD Things are looking good. quasiquotes in particular look beautiful: the TH ones (with Haskell source inside) colour/link their contents too! Haven't yet begun to check for possible performance problems. * Support CPP and top-level pragmas The support for these is hackier - but no more hacky than the existing support. * Tests pass, CPP is better recognized The tests were in some cases altered: I consider the new output to be more correct than the old one.... * Fix shrinking of source without tabs in test * Replace 'Position'/'Span' with GHC counterparts Replaces 'Position' -> 'GHC.RealSrcLoc' and 'Span' -> 'GHC.RealSrcSpan'. * Nits * Forgot entry in .cabal * Update changelog (cherry picked from commit 4f75be9)

mpickering and others added 15 commits December 5, 2017 00:58

Start changing to use GHC lexer

a27e172

better cpp

be82cd5

Change SrcSpan to RealSrcSpan

ad9bb7b

Remove error

f4e0d68

Try to stop too many open files

5631e5b

wip

e86c667

wip

bcfce50

Revert "wip"

6dec391

This reverts commit b605510. Conflicts: haddock-api/haddock-api.cabal haddock-api/src/Haddock/Interface.hs

Remove pointless 'caching'

6b7129d

Use dlist rather than lists when finding vars

8a149ad

Use a map rather than list

730cc0a

Delete bogus comment

805c51a

Rebase followup

e889381

Things now run using the GHC lexer. There are still - stray debug statements - unnecessary changes w.r.t. master

Cleaned up differences w.r.t. current Haddock HEAD

a71bb77

Things are looking good. quasiquotes in particular look beautiful: the TH ones (with Haskell source inside) colour/link their contents too! Haven't yet begun to check for possible performance problems.

Support CPP and top-level pragmas

6e5d8fc

The support for these is hackier - but no more hacky than the existing support.

harpocrates added 5 commits December 7, 2017 12:23

Tests pass, CPP is better recognized

33cc917

The tests were in some cases altered: I consider the new output to be more correct than the old one....

Fix shrinking of source without tabs in test

4d9cc1c

Replace 'Position'/'Span' with GHC counterparts

2f70ab9

Replaces 'Position' -> 'GHC.RealSrcLoc' and 'Span' -> 'GHC.RealSrcSpan'.

Nits

ddbb5b9

Forgot entry in .cabal

8531174

harpocrates changed the title ~~WIP: Use the GHC lexer for the Hyperlinker backend~~ Use the GHC lexer for the Hyperlinker backend Dec 7, 2017

Update changelog

10dc80a

alexbiehl reviewed Dec 10, 2017

View reviewed changes

alexbiehl merged commit 3a34ce5 into haskell:master Dec 10, 2017

harpocrates deleted the ghc-lexer branch December 11, 2017 00:25

alexbiehl mentioned this pull request Dec 11, 2017

Quasiquoters rendering broken in hyperlinked sources #488

Closed

harpocrates mentioned this pull request Feb 12, 2018

Support Haddock's hyperlinker haskell/cabal#5140

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use the GHC lexer for the Hyperlinker backend #714

Use the GHC lexer for the Hyperlinker backend #714

Uh oh!

harpocrates commented Dec 6, 2017

Uh oh!

harpocrates commented Dec 6, 2017 •

edited

Loading

Uh oh!

alexbiehl commented Dec 6, 2017 via email •

edited

Loading

Uh oh!

harpocrates commented Dec 7, 2017

Uh oh!

alexbiehl commented Dec 8, 2017

Uh oh!

alexbiehl left a comment

Uh oh!

alexbiehl Dec 10, 2017 •

edited

Loading

Uh oh!

harpocrates Dec 10, 2017

Uh oh!

alexbiehl commented Dec 10, 2017

Uh oh!

harpocrates commented Dec 11, 2017

Uh oh!

Uh oh!

Use the GHC lexer for the Hyperlinker backend #714

Use the GHC lexer for the Hyperlinker backend #714

Uh oh!

Conversation

harpocrates commented Dec 6, 2017

Uh oh!

harpocrates commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexbiehl commented Dec 6, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harpocrates commented Dec 7, 2017

Uh oh!

alexbiehl commented Dec 8, 2017

Uh oh!

alexbiehl left a comment

Choose a reason for hiding this comment

Uh oh!

alexbiehl Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harpocrates Dec 10, 2017

Choose a reason for hiding this comment

Uh oh!

alexbiehl commented Dec 10, 2017

Uh oh!

harpocrates commented Dec 11, 2017

Uh oh!

Uh oh!

harpocrates commented Dec 6, 2017 •

edited

Loading

alexbiehl commented Dec 6, 2017 via email •

edited

Loading

alexbiehl Dec 10, 2017 •

edited

Loading