This fixes Trac #15471
In the typechecker we check that the splice has the right type but we
crucially don't zonk the generated expression. This is because we might
end up unifying type variables from outer scopes later on.
simonpj | |
goldfire | |
bgamari |
This fixes Trac #15471
In the typechecker we check that the splice has the right type but we
crucially don't zonk the generated expression. This is because we might
end up unifying type variables from outer scopes later on.
Lint Warnings | Excuse: todo |
Severity | Location | Code | Message |
---|---|---|---|
Warning | compiler/typecheck/TcHsSyn.hs:779 | TXT3 | Line Too Long |
Warning | compiler/typecheck/TcSplice.hs:485 | TXT3 | Line Too Long |
Warning | compiler/typecheck/TcSplice.hs:513 | TXT3 | Line Too Long |
Warning | compiler/typecheck/TcSplice.hs-boot:34 | TXT3 | Line Too Long |
No Unit Test Coverage |
Time | Test | |
---|---|---|
0 ms | Test: Test #!/bin/bash -eo pipefail
mkdir -p test-results
make test THREADS=`mk/detect-cpu-count.sh` SKIP_PERF_TESTS=YES JUNIT_FILE=../../test-results/junit.xml
| |
0 ms | Test: Test #!/bin/bash -eo pipefail
mkdir -p test-results
make test THREADS=`mk/detect-cpu-count.sh` SKIP_PERF_TESTS=YES JUNIT_FILE=../../test-results/junit.xml
| |
0 ms | Boot: Boot #!/bin/bash -eo pipefail
./boot
| |
0 ms | Boot: Boot #!/bin/bash -eo pipefail
./boot
| |
0 ms | Build: Build #!/bin/bash -eo pipefail
make -j`mk/detect-cpu-count.sh` V=0
| |
View Full Test Results (2 Failed · 20 Passed) |
This looks like it works, but I think it can be refactored to be simpler. Thanks!
compiler/hsSyn/HsExpr.hs | ||
---|---|---|
46 | Ew. Do we really need this? I don't think so -- see my comments in TcSplice. | |
2453 | It would be nice to have a Note somewhere describing how this new system works. | |
2592 | Unevaluated typed splice, perhaps? | |
compiler/typecheck/TcSplice.hs | ||
495 | I don't see the motivation for storing this as a delayed computation. I'd much rather see HsSplicedT store the bits and pieces necessary for the zonker just to call runTopSplice. I think that would be considerably simpler (and easier to debug, print, etc.). | |
500 | Why does this need to be orig_expr? Can't it just be q_expr? That would mean we don't need to preserve orig_expr. | |
523 | Instead of adding this parameter, better just for the caller to zonk if it likes to. | |
testsuite/tests/th/all.T | ||
442 | Oops! |
If you look at the first version of this patch, I did as you suggested and persisted all the original parts but I think it was more complicated. You still need to persist TcLclEnv and so end up with an import cycle and several {-# SOURCE #-} imports in the zonker.
See: https://phabricator.haskell.org/D5286?vs=on&id=18534&whitespace=ignore-most#toc
This way, all the code which deals with running the splice still exists in TcSplice. The TcRnTypes.hs-bootconceptually just contains TcM which will not change.
compiler/typecheck/TcSplice.hs | ||
---|---|---|
495 | It was like this before but it meant importing a bunch of stuff into TcHsType which caused import cycles. Doing it this way means all the code to do with splices is in the same module rather than being split out into the zonker as well. | |
500 | It needs to be orig_expr as this is used to print the message to users which describes what the splice expands to. |
I had a call with Simon and he suggested two changes which I have implemented now. The finaliser bug should also be fixed.
CI: https://gitlab.staging.haskell.org/mpickering/ghc/pipelines/105
Looking good! Some small suggestions for Matthew
compiler/hsSyn/HsExpr.hs | ||
---|---|---|
2456 | say what each of these fields are. I'm still very suspicious about why we need the GhcRn version. | |
compiler/typecheck/TcRnDriver.hs | ||
419 ↗ | (On Diff #19046) | Add new_ev_binds to tcg_env, so that you don't need to pass them to zonkTcGblEnv |
447 ↗ | (On Diff #19046) | Worth commenting that this step could in principle give rise to more finalisers... or not? |
467 ↗ | (On Diff #19046) | If you are going to take a TcGblEnv as argument, surely you should return one as a result! That makes the client code much cleaner. |
compiler/typecheck/TcSplice.hs | ||
500 | What's the conclusion here? Can we drop orig_expr? | |
558 | What's happening here? Explain! | |
784 | Give a type signature for test_foo | |
795 | Explain why! Something like To run the splice, we must compile `test_foo` all the way to byte code. But at the moment when the type checker is looking at the splice, test_foo has type `Q (alpha -> alpha)` (NB: I think that's not quite right), and we certainly can't compile code involving unification variables! We could default `alpha` to `Any` but then we infer `qux :: Any -> Any` which definitely is not what we want. Moreover, if we had qux = [$$(test_foo), (\x -> x +1::Int)] then `alpha` would have to be `Int`. Conclusion: we must defer taking decisions about `alpha` until the typechecker is done; and *then* we can run the splice. It's fine to do it later, because we know it'll produce type-correct code. | |
801 | splice |
I applied Simon's suggestions apart from the one about TcGblEnv which I don't think made the code much clearer as it then wasn't obvious which bits to extract from the TcGblEnv to combine together with the other TcGblEnv in order to get the correct result.
I'll not put it up on gitlab and merge it once CI passes.
compiler/typecheck/TcSplice.hs | ||
---|---|---|
500 | Right I tried it out. Before we get: testsuite/tests/th/T15471.hs:7:10-17: Splicing expression test_foo ======> foo1 testsuite/tests/th/T15471.hs:9:12-29: Splicing expression list_foo [|| y_a1jc ||] ======> [y_a1jc, y_a1jc] and after we get testsuite/tests/th/T15471.hs:7:10-17: Splicing expression test_foo ======> foo1 testsuite/tests/th/T15471.hs:9:12-29: Splicing expression list_foo Language.Haskell.TH.Syntax.unsafeTExpCoerce [|| y_a1jc ||] ======> [y_a1jc, y_a1jc] So the two are not the same and I will keep the old behaviour. | |
558 | It's just an artifact of the fact that tcTopSpliceExpr doesn't zonk the result any more. Not sure what you want me to explain? |
ok. There are some other comments you might want to address when you finalise
compiler/typecheck/TcRnDriver.hs | ||
---|---|---|
447 ↗ | (On Diff #19046) | Any thoughts? |
compiler/typecheck/TcSplice.hs | ||
500 |
OK. Perhaps add that as a comment to explain why we need orig_expr? | |
795 | Did you act on this? | |
801 | Don't forget to fix this typo |