Ghcide: Parse error on comment that ghc accepts

[googleson78]

ghcide on current master errors out when trying to parse this:

blademaster :: ~ » cat test.hs
main :: IO ()
main = do
  -- ^
  pure ()

with:

blademaster :: ~ » ghcide test.hs
ghcide version: 0.1.0 (GHC: 8.6.5) (PATH: /home/googleson78/.local/bin/ghcide) (GIT hash: a1cb4eb8fa27821a28b48ce05bb690d82792a18f)
Ghcide setup tester in /home/googleson78.
Report bugs at https://github.com/digital-asset/ghcide/issues

Step 1/6: Finding files to test in /home/googleson78
Found 1 files

Step 2/6: Looking for hie.yaml files that control setup
Found 1 cradle

Step 3/6, Cradle 1/1: Implicit cradle for /home/googleson78
Cradle {cradleRootDir = "/home/googleson78", cradleOptsProg = CradleAction: Default}

Step 4/6, Cradle 1/1: Loading GHC Session
Interface files cache dir: /home/googleson78/.cache/ghcide/da39a3ee5e6b4b0d3255bfef95601890afd80709

Step 5/6: Initializing the IDE

Step 6/6: Type checking the files
File:     /home/googleson78/test.hs
Hidden:   no
Range:    3:2-3:6
Source:   parser
Severity: DsError
Message:  parse error on input ‘-- ^’

Completed (1 file worked, 0 files failed)

But ghc(i) does fine:

blademaster :: ~ » ghc test.hs
[1 of 1] Compiling Main             ( test.hs, test.o )
Linking test ...

blademaster :: ~ » ghci test.hs
GHCi, version 8.6.5: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/googleson78/.ghci
[1 of 1] Compiling Main             ( test.hs, interpreted )
Ok, one module loaded.
>

[mpickering]

I don't think this will stop the IDE working. In fact you can see in your log that
Completed (1 file worked, 0 files failed) so all the IDE features should still work as normal.

In fact I think the bug report here is that parse errors reported when parsing with Opt_Haddock should be downgraded to warnings instead of displaying at errors. Thoughts @pepeiborra ?

[googleson78]

Indeed, you're right. I just saw my editor flaring up with red together with a parse error and automatically assumed the the IDE wouldn't work :smile:

[kosmikus]

I just discovered this as well.

(And fwiw, I think this should not be reported at all, not even as a warning. I don't want a correct program to suddenly have errors or warnings in my IDE.)

[ndmitchell]

@kosmikus if you ever want to run haddock on this file it will fail. That seems like something that definitely should be fixed (ideally in Haddock, to not fail, but in the absence of that, in all programs)

[googleson78]

Perhaps ghcide could have a flag toggling this. Or is it a bad idea to have this flag in ghcide?

[pepeiborra]

Is this a bug or a feature? ghcide parses Haddock comments in your project to display them in hovers, so it's only reasonable that it shows an error if a Haddock comment doesn't parse. I can see how this can be surprising if you were not intending to write a Haddock comment at all! But (to me) this seems like a minor inconvenience.

We could very easily turn this error into a warning, but then we would get bug reports of the form "ghcide displays Haddocks for all my modules except Foo". So I'm not keen on doing that, but I'm not opposed either.

[mpickering]

FWIW, I really like this feature as an IDE user and I have now started writing a lot more haddock comments than before as they become immediately useful. It should probably be just a warning rather than an error though.

[ndmitchell]

I hate configuration options as they turn into a support nightmare. For every bug, you have to get the users config. You have to document it. And the number of users who configure something is tiny, so the benefit is low.

I think these should definitely be warnings. Warnings mean your code runs, but something undesirable - and there that "something undesirable" is no docs. I think it gets very confusing for people when warnings don't mean there code doesn't run - I did that with HLint for a while and it just confused everyone calling the suggestions errors.

[kosmikus]

Normally, I can turn off warnings that I don't want in GHC. With this one, I take it that it would not be possible to turn off. I think that's really bad. I think it's a case where ghcide is unfortunately trying to do too much, and less is more. The expectation that a file that's fine for GHC should be fine for ghcide sounds reasonable to me.

[kosmikus]

And just to illustrate: the situation where I'm encountering this is commenting out a line with a guard. I find it completely natural during development to do that, even if this may not end up being "final" code. I find bogus warnings / errors for something like this very distracting.

[pepeiborra]

Andres, you make a good case about the inconvenience of the current situation, but I'm confused by your other comments, in particular the less is more one. Are you saying that ghcide is going too far by displaying Haddocks in the current project?

[kosmikus]

I think this comment is from a perspective where I originally thought ghcide was deliberately trying to do only few things, and do them well, and lots of more involved features would be part of haskell-language-server. Personally, I would indeed prefer if ghcide would focus on GHC and nothing else (no hlint, no haddock, no ...). That problem space seems difficult enough.

But I am sorry, I should not have generalised so much, and I should not have mixed it up with this issue, and it is not my place to question the overall direction of ghcide which in principle I enjoy using very much.

[pepeiborra]

Your perspective is correct, its just that Haddocks fall inside the GHC sphere because they are produced by Parsing and embedded into interface files.

@wz1000 @mpickering have you by any chance already addressed this issue in hls and want to upstream the fix to ghcide?

[kosmikus]

I see. If this is actually produced by GHC, is there a flag I can add to the ghc-options of my cabal file that triggers these errors also during normal compilation then, so that I could at least re-establish consistency of behaviour in the other direction?

[pepeiborra]

Yes, it's the -haddock flag if I remember correctly.

[Sir4ur0n]

My 2 cents (in case anyone cares 😄)

Flag vs warning vs error

I agree that adding a flag for such a thing seems overkill, and not worth the cost (maintenance, bug tracking, documentation, etc.).

I personally don't mind if it is displayed as an error, but I understand some concerns/inconveniences, so displaying as a warning would be fair, too.

If I had to pick, I would keep it as an error: I prefer not to write or read code that will not be able to display documentation, it looks like a code smell to me

Error message

That being said, I think the error message should be improved. I met this issue a couple of days ago (and opened an issue to HLS) and it never occurred to me the problem was related to Haddock.

Notice that in @googleson78 original post, there is no mention of Haddock whatsoever. And Ghcide never mentions it either.

Had I been informed it could not be parsed by Haddock, I would have solved this problem in seconds.
But because of this error message, I spent several minutes trying to understand why GHC was complaining, then several more online trying to figure out this issue.

At least when running stack haddock the context (lines above and below the error) help me guess it's about Haddock:

$ stack haddock
# Much noise...

Running Haddock on library for foobar-0.1.0.0..

src/Lib.hs:9:3: error: parse error on input ‘-- $’
  |
9 |   -- $
  |   ^^^^
Haddock coverage:

[ndmitchell]

As someone who made the mistake of classifying non-errors using the error word in HLint, we _really_ want this to be a warning and not an error. Errors stop your code from running. People get super confused if their code runs _and_ has an error. Super confused people raise issues. Issues need to be replied to.

It's a shame that upstream GHC doesn't include the word Haddock parser error, but I guess that means we have to munge it ourselves.

[ndmitchell]

Patch in #608.