Support adding objects from TH
ClosedPublic

Authored by harpocrates on Nov 21 2017, 3:13 AM.

Details

Summary

The user facing TH interface changes are:

  • 'addForeignFile' is renamed to 'addForeignSource'
  • 'qAddForeignFile'/'addForeignFile' now expect 'FilePath's
  • 'RawObject' is now a constructor for 'ForeignSrcLang'
  • 'qAddTempFile'/'addTempFile' let you request a temporary file from the compiler.
Test Plan

unsure about this, added a TH test

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.
harpocrates created this revision.Nov 21 2017, 3:13 AM

This is a variant of https://phabricator.haskell.org/D4064 which is FilePath based instead.

(Also, I don't think the Windows failed build is related to anything I did)

Some bikeshedding of names:

  • addForeignSource is definitely a more appropriate name for today's addForeignFile
  • that said, reusing addForeignFile to means something _new_ (with the _same_ type signature) seems to be inviting breakage
  • qAddTempFile/addTempFile are misleading - no files are created

Alternate ways of naming things:

  • don't change the name of addForeignFile :: ForeignSrcLang -> String -> Q ()
  • introduce addForeignFilePath :: ForeignSrcLang -> FilePath -> Q ()
  • call the temporary path generation functions qTempFilePath/tempFilePath instead of`qAddTempFile`/addTempFile

I have no preference on this matter.

The breakage of addForeignFile is indeed concerning. I think retiring addForeignFile and naming the new function addForeignFilePath might be preferrable for this reason.

Also, it's still not clear which filesystem the given path is supposed to reside on in the case of a cross-compiled environment.

libraries/template-haskell/Language/Haskell/TH/Syntax.hs
469

Is this intentional?

ljli added a subscriber: ljli.Nov 21 2017, 9:38 PM

Regarding the addForeignFile change. The only real consumer (on hackage at least) seems to be inline-c. Just to have some estimation of the breakage.

$ rg --files-with-matches '(a|A)ddForeignFile' ~/hackage-srcs
th-orphans-0.13.4/CHANGELOG.md
ghc-8.2.1/typecheck/TcSplice.hs
ghci-8.2.1/GHCi/Message.hs
ghci-8.2.1/GHCi/TH.hs
template-haskell-2.12.0.0/Language/Haskell/TH/Syntax.hs
singletons-2.3/src/Data/Singletons/Util.hs
th-orphans-0.13.4/src/Language/Haskell/TH/Instances.hs
singletons-2.3/src/Data/Singletons/Single/Monad.hs
inline-c-0.6.0.5/src/Language/C/Inline/Internal.hs
  • Rename TH foreign file API
  • Typo in comment

The breakage of addForeignFile is indeed concerning. I think retiring addForeignFile and naming the new function addForeignFilePath might be preferrable for this reason.

I've updated the API. The change is currently:

  • rename addForeignFile to addForeignSource (and implement it in terms of addTempFile and addForeignFilePath)
  • remove qAddForeignFile from the Quasi class
  • add qAddForeignFilePath :: ForeignSrcLang -> FilePath -> m () and qAddTempFile :: String -> m () to the Quasi class
  • add a RawObject constructor to ForeignSrcLang

The breakage is felt by people using addForeignFile/qAddForeignFile, and the fix is to use addForeignSource instead. Does the author of inline-c @bitonic have an opinion on this matter?

Also, it's still not clear which filesystem the given path is supposed to reside on in the case of a cross-compiled environment.

I think @angerman had a good story in https://phabricator.haskell.org/D4064#115011 for eventually separating host and target using a hypothetical withHostIO function to do the actual writing and compiling of temporary files. Someone please correct me if I misunderstood something. :)

The breakage is felt by people using addForeignFile/qAddForeignFile, and the fix is to use addForeignSource instead. Does the author of inline-c @bitonic have an opinion on this matter?

Thanks for the ping. I have no problem with it, but generally I'd just leave addForeignFile there and deprecate it. No need to break the API needlessly.

I really *really* hope I'm getting around to finish D3608 this weekend. Will see if I can try some interplay between both diffs then.

The breakage is felt by people using addForeignFile/qAddForeignFile, and the fix is to use addForeignSource instead. Does the author of inline-c @bitonic have an opinion on this matter?

Thanks for the ping. I have no problem with it, but generally I'd just leave addForeignFile there and deprecate it. No need to break the API needlessly.

Do you feel that way about the class method qAddForeignFile too? If so, we could just give it a default implementation qAddForeignFile = qAddForeignSource.

Do you feel that way about the class method qAddForeignFile too? If so, we could just give it a default implementation qAddForeignFile = qAddForeignSource.

If it was up to me I'd only leave and deprecate the "main" function, addForeignFile, since we're reasonably sure that there are no users of the other.

  • Add back addForeignFile

If it was up to me I'd only leave and deprecate the "main" function, addForeignFile, since we're reasonably sure that there are no users of the other.

I've added addForeignFile back and marked it as deprecated.

AFAICT, the only breakage then is for th-orphans and singletons which define the following instances of Quasi (thanks @ljli):

-- th-orphans
instance Quasi m => Quasi (StateT s m)
instance (Quasi m, Monoid w) => Quasi (WriterT w m)
instance Quasi m => Quasi (ReaderT * r m)
instance (Quasi m, Monoid w) => Quasi (RWST r w s m)

-- singletons
instance (Quasi q, Monoid m) => Quasi (QWithAux m q)
ljli added a comment.Nov 22 2017, 5:53 PM

I'm quite happy with this. Thanks for doing this @harpocrates.
I tested it like this: https://gist.github.com/ljli/c5aca47ca4eb89bded7e5cd87144bd96
GHCi (with -fobject-code) works and it makes my life a lot easier in general.
Longterm, we probably want to disentangle TH, IO and the compiler internals, but for the time being this solves a real problem.

In D4217#117811, @ljli wrote:

I'm quite happy with this. Thanks for doing this @harpocrates.
I tested it like this: https://gist.github.com/ljli/c5aca47ca4eb89bded7e5cd87144bd96
GHCi (with -fobject-code) works and it makes my life a lot easier in general.
Longterm, we probably want to disentangle TH, IO and the compiler internals, but for the time being this solves a real problem.

I've already got a package inline-rust queued up for this sort of thing. In fact, finishing that properly was what got implementing this GHC feature in the first place. :)

I'm not sure I see how the compiler internals are tangled with TH/IO through this - by construction TH requires some degree of interaction with the compiler. As for entangling IO in, I totally agree (and hope the cross-compilation stuff will force us to untangle things a bit).

In D4217#117811, @ljli wrote:

I'm quite happy with this. Thanks for doing this @harpocrates.
I tested it like this: https://gist.github.com/ljli/c5aca47ca4eb89bded7e5cd87144bd96
GHCi (with -fobject-code) works and it makes my life a lot easier in general.
Longterm, we probably want to disentangle TH, IO and the compiler internals, but for the time being this solves a real problem.

I've already got a package inline-rust queued up for this sort of thing. In fact, finishing that properly was what got implementing this GHC feature in the first place. :)

I'm not sure I see how the compiler internals are tangled with TH/IO through this - by construction TH requires some degree of interaction with the compiler. As for entangling IO in, I totally agree (and hope the cross-compilation stuff will force us to untangle things a bit).

I agree we'll certainly need IO, even though I hate having IO actions at compile time; yet it's a necessity here. Ideally we could sandbox it somehow, but I don't see how just yet.

To take IO and TH apart (as needed for cross compilation), I basically see two extreme approaches:

  • blow up the Quasi monad to make actions more explicit. This is what I basically did in D3608, explicitly exposing File, Directory and Process actions. This would allow to have an instance with a qRunIO that errors out and as such prohibits arbitrary IO.
  • Only teach the Quasi monad about the distinction between where IO is run. (qRunHostIO, qRunBuildIO).

A combination of both is what @simonmar proposed in D3608, which would reduce the direct blow up in the Quasi monad, but still be a bit more explicit about what the actual IO action is.

As I mentioned I hope I'll be able to get around to this over the weekend and then we should know more :-)

I finally got around to finishing my WIP implementation of an inline-rust package using this feature: https://github.com/harpocrates/inline-rust/.

This should clarify precisely my use case for this feature. Hopefully playing that some more I'll continue gaining confidence about the correctness of this differential. So far, everything I've tried has worked as expected.

I finally got around to finishing my WIP implementation of an inline-rust package using this feature: https://github.com/harpocrates/inline-rust/.

Very nice!

This should clarify precisely my use case for this feature. Hopefully playing that some more I'll continue gaining confidence about the correctness of this differential. So far, everything I've tried has worked as expected.

It looks like this should work quite nicely for this. There is a slightly orthogonal configuration issue though: how should one specify where rustc is to be found? Currently it looks like the user has to ensure it is in PATH which I suppose works but is slightly suboptimal.

  • Rename TH foreign file API
  • Typo in comment
  • Add back addForeignFile
  • Get rid of warning

I'm starting to also want a LangCmm constructor.

data ForeignSrcLang
  = LangC | LangCxx | LangObjc | LangObjcxx | LangCmm | RawObject

The implementation seems straightforward and contained to DriverPipeline. This sort of thing would make prototyping ideas using foreign import prim (which is more expressive than foreign import ccall) much more feasible. Does this sound like a bad idea?

I'm starting to also want a LangCmm constructor.

data ForeignSrcLang
  = LangC | LangCxx | LangObjc | LangObjcxx | LangCmm | RawObject

The implementation seems straightforward and contained to DriverPipeline. This sort of thing would make prototyping ideas using foreign import prim (which is more expressive than foreign import ccall) much more feasible. Does this sound like a bad idea?

If we are going down this route, shouldn't we make this parametric?

data ForeignLang a = Lang a | RawObject

or is there any reason why that would not work?

mboes added a subscriber: mboes.Jan 11 2018, 4:37 PM
  • Rename TH foreign file API
  • Typo in comment
  • Add back addForeignFile
  • Get rid of warning

I don't really want this to slip through the cracks, so I've gone ahead and rebased this onto today's HEAD. As I understand, the last time I left this ticket 2 months ago, @angerman had mentioned wanting to test this out in conjunction with D3608: Extend the Quasi Monad but progress on that front has stalled for the moment.

What would be the right way forward from here? Perhaps I should seek more feedback from the community? I really would like to avoid missing another release cycle...

PS. I'm not asking for a review right now, I know that handling 8.4.1 is much more time sensitive - I just want to know what to expect in terms of the process. :)

@harpocrates, I’m terribly sorry for letting this languish. I’m currently bed bound, but will try to give this a try next week. Feel free to ping me next week!

@angerman ping!

(Feel free to delay this if you are too busy - like I said, I'm only worried about this being completely forgotten.)

@angerman ping!

(Feel free to delay this if you are too busy - like I said, I'm only worried about this being completely forgotten.)

Thanks. I'm on it :-) Last week was (unexpectedly) a pretty cabal heavy one...

@angerman ping!

(Again, feel free to delay this if you are too busy.)

harpocrates updated this revision to Diff 15634.Mar 3 2018, 2:25 PM

Rebase

  • Rename TH foreign file API
  • Typo in comment
  • Add back addForeignFile
  • Get rid of warning
hsyl20 awarded a token.Mar 5 2018, 9:11 AM
hsyl20 added a subscriber: hsyl20.
angerman accepted this revision.Mar 5 2018, 7:22 PM

Ok, let's do this in 8.6.

If someone ends up spotting issues throughout 8.6 we may be able to fix them in time. I guess the really tricky ones will be iOS/Android/RPi applications that use inline-rust. I'd love to explore that, but just didn't manage to write up anything substantial.

This revision is now accepted and ready to land.Mar 5 2018, 7:22 PM
harpocrates updated this revision to Diff 15708.Mar 8 2018, 4:14 PM
  • Update deprecation version
harpocrates marked an inline comment as done.Mar 8 2018, 4:22 PM
harpocrates added inline comments.
libraries/template-haskell/Language/Haskell/TH/Syntax.hs
469

If you were referring to the "deprecated in 8.4" as opposed to "8.6", good catch!

Other that that, the deprecation of addForeignFile in favour of the more nicely named addForeignSource is intentional. Given that we are adding addTempFile (which actually involves files), addForeignFile is likely to just cause confusion - despite its name, its API is completely free of any files.

bgamari accepted this revision.Mar 25 2018, 12:58 PM

Alright, indeed the implementation looks reasonable to me. I'm still a bit worried about the implications on cross-compilation but if it has @angerman's approval then I suppose we can just go ahead and merge.

This revision was automatically updated to reflect the committed changes.