#573, make haddock errors warnings with the word Haddock in front

[ndmitchell]

Fixes #573. Downgrade Haddock errors to warnings, and prepend Haddock in front of them. Now we are manipulating Haddock warnings, its easier to dedupe them by range than trying to merge arbitrary diagnostics.

[pepeiborra]

There's probably more changes needed to handle Haddock errors in dependencies, see:

https://github.com/digital-asset/ghcide/blob/master/src/Development/IDE/Core/Rules.hs#L632-L635

[pepeiborra]

do we need this special logic? I would think that a comment can only have Haddock warnings

[ndmitchell]

We run the parser twice. If you have ( as your source file, it will be a parse error with both Haddock and without, so you'll get the same error twice. This dedupe ensures you only get the warning once.

[pepeiborra]

so the comment is a bit misleading, it's only non Haddock warnings that need deduping, right?

[ndmitchell]

Comment updated. Is that clearer?

[ndmitchell]

I've had much more issues with haddock warnings in files of interest because I just typed -- | ... to comment out a guard, whereas haddock issues in dependencies hasn't cropped up as much. I see that it that case we don't parse twice, but only once - does that mean a Haddock in a dependency is a fatal error?

[pepeiborra]

I've had much more issues with haddock warnings in files of interest because I just typed -- | ... to comment out a guard, whereas haddock issues in dependencies hasn't cropped up as much. I see that it that case we don't parse twice, but only once - does that mean a Haddock in a dependency is a fatal error?

I would think so, yes, and the solution is similar - parse again without OptHaddock.

[ndmitchell]

It feels odd that we have two locations where we do a parse with haddock/parse without dance, and in one we use concurrently, and in one we don't. We should probably do the same thing in both cases.

My guess is "the right thing" is to parse with Haddock, and if there are no errors, continue on. If there were errors, parse again without Haddock, and turn all errors that came from Haddock only into warnings. For files with parse errors it's slightly more latency, but the latency on a parse error is pretty low already, so that isn't a massive problem.

@pepeiborra if that makes sense to you, I'll try that.

[pepeiborra]

It feels odd that we have two locations where we do a parse with haddock/parse without dance, and in one we use concurrently, and in one we don't. We should probably do the same thing in both cases.

My guess is "the right thing" is to parse with Haddock, and if there are no errors, continue on. If there were errors, parse again without Haddock, and turn all errors that came from Haddock only into warnings. For files with parse errors it's slightly more latency, but the latency on a parse error is pretty low already, so that isn't a massive problem.

@pepeiborra if that makes sense to you, I'll try that.

The reason we don't parse with -haddock always-on is that it throws away annotations (ref. @cocreature ), which can be troublesome. For interface files we don't care as the parsed AST is not stored, but for files of interest we probably do care.

[ndmitchell]

OK, I'll try figuring out some sane abstractions to handle that mess.

[cocreature]

The reason we don't parse with -haddock always-on is that it throws away annotations (ref. @cocreature ), which can be troublesome. For interface files we don't care as the parsed AST is not stored, but for files of interest we probably do care.

Adding a bit more context here: GHC has Opt_Haddock and Opt_KeepRawTokenStream which are mutually exclusive. In theory, the latter preserves all information for haddock to work based on it afaik and there is an issue somewhere for making it work. In practice, haddock currently only works if you use Opt_Haddock which throws away information that tools like HLint rely on and which is included in Opt_KeepRawTokenStream.

[ndmitchell]

So we still have two different behaviours. For getModIface we try with Haddock, and if that fails, fall back to without. Otherwise, we do both in parallel and merge them. I centralised the "merge them" code, so they aren't too different, but the weird corner cases around Haddock are pretty frustrating!

[cocreature]

Thanks!