Avoid unnecessary recompilation due to -haddock #4596
Conversation
|
I would love to implement a regression test for this, but I am currently unsure how. |
fendor
force-pushed
the
fix/haddock-recompilation
branch
2 times, most recently
from
3f84a3d to
16d4457
Compare
fendor
force-pushed
the
fix/haddock-recompilation
branch
from
16d4457 to
2606976
Compare
|
I manually tested it on GHC and think I can confirm this fixes the recompilation issue but I struggle to come up with a test case in HLS itself. |
|
possible issue with this PR -- the haddocks for definitions in the local module don't appear to be available in the completion details. I instead see: in the lsp server's log output. non-local details DO appear to be resolvable, however, and that warning isn't printed. if I just use master instead of applying this PR, this warning is never printed for either local or non-local bindings. |
|
Thank you for testing this change! I will look into it. |
fendor
force-pushed
the
fix/haddock-recompilation
branch
2 times, most recently
from
af7a56d to
e66c922
Compare
|
Can't reproduce, with GHC 9.6.7, I get: and the logs are clear. |
fendor
force-pushed
the
fix/haddock-recompilation
branch
2 times, most recently
from
fd3ee2d to
d3d1869
Compare
fendor
force-pushed
the
fix/haddock-recompilation
branch
from
d3d1869 to
5456c1d
Compare
Due to unprincipled adding and removing the `-haddock` flag during compilation and recompilation checking, we were performing more work than necessary. We avoid this by compiling everything with `-haddock` by default. This is safe nowadays, we have essentially been doing this for many releases, and know this is fine. For the occasion where we actually want to parse without the `-haddock` flag, we keep explicitly disabling it. We enable `-haddock` flag during session loading, since we already perform a number of DynFlags tweaks. This behaviour is dependent on the `OptHaddockParse` opton, which can, currently, only be modified at compile-time.
fendor
force-pushed
the
fix/haddock-recompilation
branch
from
5456c1d to
67bda98
Compare
|
I am unsure how to test this, I manually verified that the behaviour reported in #4511 doesn't seem to be occurring in GHC. Are we fine with merging this as is? |
| -- Embed haddocks in the interface file | ||
| (diags, mb_pm) <- liftIO $ getParsedModuleDefinition hsc opt f (withOptHaddock ms) | ||
| (diags, mb_pm) <- liftIO $ getParsedModuleDefinition hsc opt f ms |
There was a problem hiding this comment.
We do still need the comment above ?
| -- Otherwise, return the result of parsing without Opt_Haddock, so | ||
| -- that the parsed module contains the result of Opt_KeepRawTokenStream, | ||
| -- which might be necessary for hlint. | ||
| -- Otherwise, return the result of parsing without Opt_Haddock. |
There was a problem hiding this comment.
OptHaddockParse is not a boolean so we do not need -- Otherwise, return the result of parsing without Opt_Haddock., I think.
Due to unprincipled adding and removing the
-haddockflag during compilation and recompilation checking, we were performing more work than necessary.We avoid this by compiling everything with
-haddockby default. This is safe nowadays, we have essentially been doing this for many releases, and know this is fine.For the occasion where we actually want to parse without the
-haddockflag, we keep explicitly disabling it.We enable
-haddockflag during session loading, since we already perform a number of DynFlags tweaks.This behaviour is dependent on the
OptHaddockParseoption, which can, currently, only be modified at compile-time.Closes #4511