-
Notifications
You must be signed in to change notification settings - Fork 97
Conversation
About the two issues above:
|
Thanks a lot for the PR! I’ll have a detailed look at it next week but this looks great! It would be awesome if we could get some testcases for this. |
I have added a testcase based on the one for #212. However, something weird happens: loading such files in a command line works perfectly fine, but in the testcase runner loading times out. |
Thanks, @serras for the fix! This is great. Re: the test case timing out, what I figured out while writing the test cases in #212 and previous changes was that if your test case has no errors/warnings, there will be no diagnostics and your assertion code never runs. You need to intentionally add any error or warning, and assert that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks a lot! I have one question about the global_uses_th_qq
check and this needs to be refactored slightly to not break our use of ghcide
in DAML. As mentioned in the inline comment, I am happy to handle that myself, just let me know what you prefer.
Thanks again!
src/Development/IDE/Core/Rules.hs
Outdated
packageState <- hscEnv <$> use_ GhcSession file | ||
-- Figure out whether we need TemplateHaskell or QuasiQuotes support | ||
let global_uses_th_qq = uses_th_qq $ hsc_dflags packageState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to take care of the case in which TemplateHaskell
or QuasiQuotes
is enabled at the package level, but not in the file itself (for example, if you add them as extensions in your .cabal
file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that would still end up in the dflags of the file, is that not correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I am not sure about it. I guess we can always start with checking only the dflags
of the file, and if it appears to be the case that sometimes we need to check other things, we just uncomment the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
src/Development/IDE/Core/Compile.hs
Outdated
@@ -122,7 +123,7 @@ compileModule | |||
:: HscEnv | |||
-> [TcModuleResult] | |||
-> TcModuleResult | |||
-> IO ([FileDiagnostic], Maybe CoreModule) | |||
-> IO ([FileDiagnostic], Maybe (CoreModule, TcModuleResult)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use this function and the GenerateCore
rule in DAML where generating bytecode doesn’t make sense (if it works at all) and would slow things down unnecessarily. I think the most sensible option here is to split this in two:
- GenerateCore can be changed to return
CgGuts
. - We can add a new
GenerateByteCode
rule that depends onGenerateCore
to get theCgGuts
and then returns(CompiledByteCode, CgEntry)
.
I would probably just move the assembling of the HomeModInfo
to the call site of this rule.
If you are fine with it, I’m also happy to do this change myself and push to your branch but if you want to give it a shot yourself you are more than welcome to!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer you to do it, because I might not get it 100% right. I'll give you push access in my repo so you can just push whenever you are done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I’ll take care of it (hopefully tomorrow but definitely this week). I don’t think you need to give me access to your repo. The default on github is that you can push to the branch of a PR to your repo so unless you disabled that I should be all set.
* First attempt at TH support * Update TcModuleResult when generating core * Be a bit more cautious when asking for bytecode * Check need for bytecode not only in source file itself, also in global information * Add a test (based on haskell/ghcide#212) * Fix test (thanks, @jinwoo) * Split GenerateCore and GenerateByteCode
* First attempt at TH support * Update TcModuleResult when generating core * Be a bit more cautious when asking for bytecode * Check need for bytecode not only in source file itself, also in global information * Add a test (based on haskell/ghcide#212) * Fix test (thanks, @jinwoo) * Split GenerateCore and GenerateByteCode
* First attempt at TH support * Update TcModuleResult when generating core * Be a bit more cautious when asking for bytecode * Check need for bytecode not only in source file itself, also in global information * Add a test (based on haskell/ghcide#212) * Fix test (thanks, @jinwoo) * Split GenerateCore and GenerateByteCode
This is a first attempt to fix #34. Right now, things seem to work fine, at least examples do! :)
There are a few things that might need additional attention:
ghcide
is run. This is different from what Stack or Cabal do, because they assume thehs-source-dirs
of the project is the current directory. This means files cannot be loaded correctly by TemplateHaskell splices inghcide
.