See comments at Trac #14391
- rGHCe5013a567b23: Make TcRnMonad independent of TcSplice (#14391)
- Trac Issues
Yes, validate passed on my machine.
I did this initially, and got a failure:
=====> TH_finalizer(normal) 1 of 1 [0, 0, 0] cd "th/TH_finalizer.run" && "/home/g/haskell/head/inplace/test spaces/ghc-stage2" -c TH_finalizer.hs -dcore-lint -dcmm-lint -no-user-package-db -rtsopts -fno-warn-missed-specialisations -fshow-warning-groups -fdiagnostics-color=never -fno-diagnostics-show-caret -dno-debug-output -XTemplateHaskell -package template-haskell -v0 Actual stderr output differs from expected: diff -uw "th/TH_finalizer.run/TH_finalizer.stderr.normalised" "th/TH_finalizer.run/TH_finalizer.comp.stderr.normalised" --- th/TH_finalizer.run/TH_finalizer.stderr.normalised 2018-09-06 19:12:15.932946873 +0200 +++ th/TH_finalizer.run/TH_finalizer.comp.stderr.normalised 2018-09-06 19:12:15.932946873 +0200 @@ -1,2 +0,0 @@ - -TH_finalizer.hs:1:1: warning: Just True *** unexpected failure for TH_finalizer(normal)
I did this initially, and got a failure:
Hmm. That's odd. It looks as if the finaliser is not being run *at all*; but it's very strange that this tcLclEnv stuff would have any effect on what is run and what is not
Can you investigate what is happening?
@simonpj Fixed, the code is back for review.
Here's what happened: the finalizers were run, but any error/warning messages reported inside finalizers via qReport were discarded. The error/warning messages are put in the local environment tcl_errs variable. We were calling checkNoErrs (rnTopSpliceDecls splice). checkNoErrs creates a new reference for error messages, and after rnTopSpliceDecls adds the finalizers and returns, it checks whether this reference is empty. When we were adding the finalizers, the pairs [(TcLclEnv, ThModFinalizers)] were storing an error variable that was never checked later. I moved the call to checkNoErrs so that it won't apply to adding finalizers and now all tests pass.
I didn't follow your comment precisely. In particular, it's no supposed to drop any errors on the floor, ever. Could you add a comment to the critical line or lines, to explain the subtleties, lest someone change it back?
I'm happy the Maybe is gone.
When committing can you ensure that the commit message explains the purpose of the change. Is it *only* to eliminate a module cycle?
Here's what happened previously:
- At the very beginning, we start with a local environment, which has a tcl_errs equal to some reference r1
- We call checkNoErrs, this creates a new reference r2 to store errors.
- Inside checkNoErrs, we call add_mod_finalizers_now. This adds the pair (env, finalizer) where tcl_errs env == r2, to finalizer list in the global environment.
- At the end of checkNoErrs, we check whether there are any errors in r2. There aren't, so nothing happens.
- Later, we run finalizers. One of them signals an error (via qReport, which in turn calls addErr). The problem is that this finalizer is run in the local environment from step 3, which means the error is added to r2 instead of r1. At this point, nothing checks r2, it was supposed to be just a temporary reference.
After the fix, add_mod_finalizers is no longer called inside checkNoErrs so the added local environment contains r1 instead of r2. I added a comment, is it better now?
Yes, the only purpose is to eliminate a cycle.
After the fix, add_mod_finalizers is no longer called inside checkNoErrs so the added local environment contains r1 instead of r2
Ah now I understand, thank you.
That seems terribly fragile, though. You say "After the fix, add_mod_finalizers is no longer called inside checkNoErrs so the added local environment contains r1 instead of r2". But what if r1 was also finished with and gone, as r2 was in this case? Then the same error would re-occur.
A more robust way seems to be this
- In run_th_modfinalizers, after setLclEnv lcl_env, then set the errors-variable to the one from outside.
- Or perhaps nicer, in run_th_modfinalizers, instead of setLclEnv lcl_env, set only the fields of the current TcLclEnv that are necessary. That is start from the current env, and extract from the lcl_env stored with the finaliser just the fields that are needed. What are they? An advantage of this approach is that that would become explicit.
- any error reported by