Run typed splices in the zonker
AbandonedPublic

Authored by mpickering on Oct 30 2018, 11:27 AM.

Details

Summary

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.

mpickering created this revision.Oct 30 2018, 11:27 AM
mpickering updated this revision to Diff 18538.Oct 31 2018, 8:09 AM
  • Rename the spliced expression in the right context
mpickering updated this revision to Diff 18539.Oct 31 2018, 8:13 AM
  • Add Simon's test as well
mpickering updated this revision to Diff 18540.Oct 31 2018, 9:33 AM
  • Add dummy dependency on GHC.Types
mpickering updated this revision to Diff 18541.Oct 31 2018, 9:49 AM
  • Revert "Add dummy dependency on GHC.Types"

The build works on hadrian but not on the old build system.

mpickering updated this revision to Diff 18638.Nov 9 2018, 2:42 PM
  • Simplify
  • Remove unused variable
mpickering updated this revision to Diff 18666.Nov 12 2018, 8:25 AM
  • Remove extra arguments
goldfire requested changes to this revision.Nov 12 2018, 8:44 AM

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.

2455

It would be nice to have a Note somewhere describing how this new system works.

2602

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.).

501

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.

524

Instead of adding this parameter, better just for the caller to zonk if it likes to.

testsuite/tests/th/all.T
442

Oops!

This revision now requires changes to proceed.Nov 12 2018, 8:44 AM
mpickering marked an inline comment as done.Nov 12 2018, 12:09 PM

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.

501

It needs to be orig_expr as this is used to print the message to users which describes what the splice expands to.

goldfire added inline comments.Nov 12 2018, 3:06 PM
compiler/typecheck/TcSplice.hs
495

Grumble. OK.

501

But, I think q_expr and orig_expr should pretty-print identically.

mpickering updated this revision to Diff 18674.Nov 12 2018, 4:27 PM
  • zonk after calling tcTopSpliceExpr
  • Remove merge artefact
  • Note and Richard's other comments
mpickering updated this revision to Diff 18678.Nov 13 2018, 3:35 AM
  • Simplify
  • Remove unused variable
  • Remove extra arguments
  • zonk after calling tcTopSpliceExpr
  • Remove merge artefact
  • Note and Richard's other comments
  • Remove diff churn
mpickering updated this revision to Diff 18679.Nov 13 2018, 3:39 AM
  • Remove blank line
  • Remove blank line
  • More formatting
mpickering planned changes to this revision.Nov 13 2018, 5:52 AM

Still one problem with finalisers getting run in the wrong place.

mpickering updated this revision to Diff 19044.Dec 7 2018, 8:18 AM
  • Simplify
  • Remove unused variable
  • Remove extra arguments
  • zonk after calling tcTopSpliceExpr
  • Note and Richard's other comments
  • Remove diff churn
  • Remove blank line
  • Remove blank line
  • More formatting
  • Store arguments to runTopSplice rather than whole TcM action
  • Run and zonk finalisers

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

mpickering updated this revision to Diff 19046.Dec 7 2018, 8:57 AM
  • Try to fix build ordering

@goldfire, perhaps you could have one final look over this?

Looking good! Some small suggestions for Matthew

compiler/hsSyn/HsExpr.hs
2458

say what each of these fields are.

I'm still very suspicious about why we need the GhcRn version.

compiler/typecheck/TcRnDriver.hs
415

Add new_ev_binds to tcg_env, so that you don't need to pass them to zonkTcGblEnv

457

Worth commenting that this step could in principle give rise to more finalisers... or not?

466

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
501

What's the conclusion here? Can we drop orig_expr?

559

What's happening here? Explain!

785

Give a type signature for test_foo

796

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.
802

splice

mpickering marked 11 inline comments as done.Jan 10 2019, 5:47 AM

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
501

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.

559

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
457

Any thoughts?

compiler/typecheck/TcSplice.hs
501

So the two are not the same and I will keep the old behaviour.

OK. Perhaps add that as a comment to explain why we need orig_expr?

796

Did you act on this?

802

Don't forget to fix this typo

mpickering marked 10 inline comments as done.Jan 10 2019, 5:58 AM
mpickering added inline comments.
compiler/typecheck/TcRnDriver.hs
457

Yes, the answer is that no more finalisers can occur because of some checks in the implementation of addTopDecls. I added a comment about this.

compiler/typecheck/TcSplice.hs
796

Yes.

802

Did this as well.

mpickering marked 4 inline comments as done.Jan 10 2019, 6:00 AM

Responded to new comments now.

compiler/typecheck/TcSplice.hs
501

done.