Make TcRnMonad independent of TcSplice (#14391)
ClosedPublic

Authored by monoidal on Sep 6 2018, 2:33 PM.

Details

Summary

See comments at Trac #14391

Test Plan

validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
monoidal created this revision.Sep 6 2018, 2:33 PM

I've noticed runRemoteModFinalizers and finishTH are now used only in TcSplice. Should I move them there?

monoidal retitled this revision from Make TcRnMonad independent of TcSplice (#) to Make TcRnMonad independent of TcSplice (#14391).Sep 6 2018, 2:43 PM
monoidal updated this revision to Diff 17934.Sep 6 2018, 3:01 PM

Linter fix

simonpj requested changes to this revision.Sep 7 2018, 9:28 AM

Good. But I think we want TcLclEnv not Maybe TcLclEnv. Thanks!

Does it validate?

compiler/rename/RnSplice.hs
669

Better to do a getLclEnv and store that.

compiler/typecheck/TcRnTypes.hs
636

No Maybe

This revision now requires changes to proceed.Sep 7 2018, 9:28 AM

Yes, validate passed on my machine.

compiler/rename/RnSplice.hs
669

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?

monoidal updated this revision to Diff 18156.Oct 1 2018, 8:08 AM

Don't use Maybe

@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?

simonpj accepted this revision.Oct 1 2018, 8:31 AM
This revision is now accepted and ready to land.Oct 1 2018, 8:31 AM
monoidal updated this revision to Diff 18160.Oct 1 2018, 10:01 AM

Add a comment

Here's what happened previously:

  1. At the very beginning, we start with a local environment, which has a tcl_errs equal to some reference r1
  2. We call checkNoErrs, this creates a new reference r2 to store errors.
  3. 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.
  4. At the end of checkNoErrs, we check whether there are any errors in r2. There aren't, so nothing happens.
  5. 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
This revision was automatically updated to reflect the committed changes.