[GHC 9.0] No need to parse without Haddocks

[pepeiborra]

Haddock markup parse errors are flagged as warnings in 9.0 (no longer errors) so there's no longer a need to parse twice

[yoshitsugu]

@pepeiborra
I'm now taking a look at this 👀 but what is the expected behavior?
In the current test case, it looks like no output is expected above GHC90
https://github.com/haskell/haskell-language-server/blob/master/ghcide/test/exe/Main.hs#L562-L564

[jneira]

I'm now taking a look at this but what is the expected behavior?

I think the expected behaviour is the same but we can remove the double parsing for ghc >= 9.0.1, as haddock will not throw error for those ghc versions

here:

haskell-language-server/ghcide/src/Development/IDE/Core/Rules.hs

Lines 893 to 900 in 229faac

	-- Embed haddocks in the interface file
	(diags, mb_pm) <- liftIO $ getParsedModuleDefinition hsc opt f (withOptHaddock ms)
	(diags, mb_pm) <- case mb_pm of
	Just _ -> return (diags, mb_pm)
	Nothing -> do
	-- if parsing fails, try parsing again with Haddock turned off
	(diagsNoHaddock, mb_pm) <- liftIO $ getParsedModuleDefinition hsc opt f ms
	return (mergeParseErrorsHaddock diagsNoHaddock diags, mb_pm)

and here

haskell-language-server/ghcide/src/Development/IDE/Core/Rules.hs

Lines 197 to 250 in 229faac

	-- | WARNING:
	-- We currently parse the module both with and without Opt_Haddock, and
	-- return the one with Haddocks if it -- succeeds. However, this may not work
	-- for hlint or any client code that might need the parsed source with all
	-- annotations, including comments.
	-- For that use case you might want to use `getParsedModuleWithCommentsRule`
	-- See https://github.com/haskell/ghcide/pull/350#discussion_r370878197
	-- and https://github.com/mpickering/ghcide/pull/22#issuecomment-625070490
	-- GHC wiki about: https://gitlab.haskell.org/ghc/ghc/-/wikis/api-annotations
	getParsedModuleRule :: Rules ()
	getParsedModuleRule =
	-- this rule does not have early cutoff since all its dependencies already have it
	define $ \GetParsedModule file -> do
	ModSummaryResult{msrModSummary = ms'} <- use_ GetModSummary file
	sess <- use_ GhcSession file
	let hsc = hscEnv sess
	opt <- getIdeOptions
	modify_dflags <- getModifyDynFlags dynFlagsModifyParser
	let ms = ms' { ms_hspp_opts = modify_dflags $ ms_hspp_opts ms' }
	let dflags = ms_hspp_opts ms
	mainParse = getParsedModuleDefinition hsc opt file ms
	reset_ms pm = pm { pm_mod_summary = ms' }
	-- Parse again (if necessary) to capture Haddock parse errors
	res@(_,pmod) <- if gopt Opt_Haddock dflags
	then
	liftIO $ (fmap.fmap.fmap) reset_ms mainParse
	else do
	let haddockParse = getParsedModuleDefinition hsc opt file (withOptHaddock ms)
	-- parse twice, with and without Haddocks, concurrently
	-- we cannot ignore Haddock parse errors because files of
	-- non-interest are always parsed with Haddocks
	-- If we can parse Haddocks, might as well use them
	--
	-- HLINT INTEGRATION: might need to save the other parsed module too
	((diags,res),(diagsh,resh)) <- liftIO $ (fmap.fmap.fmap.fmap) reset_ms $ concurrently mainParse haddockParse
	-- Merge haddock and regular diagnostics so we can always report haddock
	-- parse errors
	let diagsM = mergeParseErrorsHaddock diags diagsh
	case resh of
	Just _
	| HaddockParse <- optHaddockParse opt
	-> pure (diagsM, resh)
	-- If we fail to parse haddocks, report the haddock diagnostics as well and
	-- return the non-haddock parse.
	-- This seems to be the correct behaviour because the Haddock flag is added
	-- by us and not the user, so our IDE shouldn't stop working because of it.
	_ -> pure (diagsM, res)
	-- Add dependencies on included files
	_ <- uses GetModificationTime $ map toNormalizedFilePath' (maybe [] pm_extra_src_files pmod)
	pure res

(and maybe in more code call sites?)

[jneira]

liftIO $ (fmap.fmap.fmap.fmap)

wonderful 😅

[konn]

I think the expected behaviour is the same but we can remove the double parsing for ghc >= 9.0.1, as haddock will not throw error for those ghc versions

Then, perhaps also adding a regression test with erroneous Haddock comment(s) would make sense to ensure that HLS doesn't suffer from such malformed Haddock comments.

[jneira]

Then, perhaps also adding a regression test with erroneous Haddock comment(s) would make sense to ensure that HLS doesn't suffer from such malformed Haddock comments.

Hmm i think the linked test already does it: the haddock error is converted in a warning. So if it is reported incorrectly as an error the test would fail afaiu.

[yoshitsugu]

@jneira @konn thanks for quick reply 😄 i'll try this.

[jneira]

Thinking in this twice it is unfortunate that we are losing the haddock diagnostics for ghc >= 9.0as maybe some users find them useful, before they hit the error when building docs.
Maybe we should restore those diagnostics, but not in ghcide, where are not necessary but in the hls-haddock-plugin itslef: https://github.com/haskell/haskell-language-server/blob/master/plugins/hls-haddock-comments-plugin/src/Ide/Plugin/HaddockComments.hs

[yoshitsugu]

We can close this? @pepeiborra @jneira