Expect tests with (implicit_transitive_deps false)

[ddickstein]

When I try setting (implicit_transitive_deps false) and try to use expect tests I get the error Unbound module Expect_test_common. I assume there's some module I'm missing but I'm not sure which - expect_test_common.ml is in ppx_expect, but explicitly adding ppx_expect to my pps list doesn't seem to help (I already have ppx_jane there). My library deps are just core, core_kernel, async, async_kernel. When I change (implicit_transitive_deps true) the expect test works properly.

[ghost]

That's indeed a know issue. We have the same problem in the dune testsuite itself. The problem is that (implicit_transitive_deps false) isn't backed by proper compiler support at the moment.

There is however a plan to add the necessary compiler support, by allowing to let the compiler see some modules but hide them from the user.

[rgrinberg]

That's indeed a know issue. We have the same problem in the dune testsuite itself. The problem is that (implicit_transitive_deps false) isn't backed by proper compiler support at the moment.

Actually, I think that the expect test issue can be fixed with re_exports. I think the runtime dependencies of ppx_expect should just re-export the libraries that we are manually specifying.

[ghost]

That seems reasonable yeah. I'll have a look at adding the necessary re_exports into ppx_expect next week.

[ghost]

I had a look at this, and it's not as straightforward as I thought. First, our setup to export our code to github currently generates dune 1.x files and (re_export ...) is only available since dune 2.x, so we have to update our system to generate dune 2.x files first. No big deal, but this hasn't been done yet.

Secondly, we'd have to make the runtime library of ppx_expect re-export Base because ppx_expect generates codes that looks like this:

  { name = Expect_test_common.File.Name.of_string "foo"
  ... }

where file.ml in expect_test_common starts with open Base. This means that the above of_string expect Base.string rather than string. Without access to Base, the compiler cannot see that Base.string is the same as string. Having ppx_expect re-export Base doesn't feel right to me. Base is something you should opt-in, not something you get implicitly.

It is possible to re-work a bit ppx_expect to avoid having to re-export Base. If someone is interested to work on that, I can provide some pointers.

[rgrinberg]

It is possible to re-work a bit ppx_expect to avoid having to re-export Base. If someone is interested to work on that, I can provide some pointers.

How about we just get rid of base in ppx_expect? I recall we discussed this before and people were in favor. On Jun 29, 2020, 4:34 AM -0700, Jérémie Dimino notifications@github.com, wrote:

I had a look at this, and it's not as straightforward as I thought. First, our setup to export our code to github currently generates dune 1.x files and (re_export ...) is only available since dune 2.x, so we have to update our system to generate dune 2.x files first. No big deal, but this hasn't been done yet. Secondly, we'd have to make the runtime library of ppx_expect re-export Base because ppx_expect generates codes that looks like this: { name = Expect_test_common.File.Name.of_string "foo" ... } where file.ml in expect_test_common starts with open Base. This means that the above of_string expect Base.string rather than string. Without access to Base, the compiler cannot see that Base.string is the same as string. Having ppx_expect re-export Base doesn't feel right to me. Base is something you should opt-in, not something you get implicitly. It is possible to re-work a bit ppx_expect to avoid having to re-export Base. If someone is interested to work on that, I can provide some pointers. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[ghost]

Well, this "just" has quite a bit of weight... So let's look first at the overall picture. There are quite a few components involved here:

• the ppx rewriter
• the runtime library
• the "runner" library, i.e. the one that get linked when you link an inline test runner

and we have two packages involved: ppx_inline_test and ppx_expect. At the moment:

• the runtime library of ppx_expect does not use Base directly, however it depends on a "common" library that is shared between the ppx rewriter, the runtime and the runner and this "common" library uses Base
• the runner library of ppx_expect uses Base
• the runtime library of ppx_inline_test does not use Base directly, however it depends on time_now which uses Base
• the runner library of ppx_inline_test does not uses Base

And ppx_expect depends on ppx_inline_test.

IMO, trying to entirely get rid of Base would be quite a lot of work and it's not clear it would be that necessary. In particular, using Base in the runner library seems fine to me.

However, it seems reasonable to expect the runtime libraries to be as light as possible given that these always end up in the final executable. One thing we could do is make the runtime libraries be trivial registrars and move all the logic to the runner libraries. Another option would be to rely on virtual libraries. i.e. the runtime library would be virtual with a default dummy implementation and the runner library would pull-in the more complex real implementation. We can't do it at the moment as we don't support virtual libraries inside Jane Street.

/cc other people who might be interested in this dicussion: @ceastlund @NathanReb

[MattWindsor91]

This (or at least a similar issue) has also broken the test runs of a few of my projects when migrating from Jane Street packages v0.13.0 to v0.14.0; the dune stanzas for the tests used to read

(libraries ppx_inline_test.config ppx_expect.config ppx_expect.common base ; ... )

which previously worked fine, but now gives me the cryptic error

Error: Signature mismatch:
       Modules do not match:
         sig
           module IO_run = Expect_test_config.IO_run
           module IO_flush = Expect_test_config.IO_flush
           val flush : unit -> unit
           val run : (unit -> unit) -> unit
           val flushed : unit -> bool
           val upon_unreleasable_issue :
             Expect_test_config_types__.Expect_test_config_types_intf.Upon_unreleasable_issue.t
         end
       is not included in
         Expect_test_config_types.S

Is the easiest thing to do for now to disable (implicit_transitive_deps false)? I've had issues with this flag in the past (especially having to pull in dependencies like sexplib0 and base.caml).

[ghost]

Thanks @MattWindsor91 ! I have been trying to understand why we were getting this error in the Dune CI but without luck. Now that you pointed out the connection with (implicit_transitive_deps false), I realise that this is because ppx_expect.config was split between ppx_expect.config and ppx_expect.config_types. Adding the latter to the list of libraries fixes the issue.

To answer your question, for now you can either disable (implicit_transitive_deps false) or add ppx_expect.config_types.

BTW, until we have proper compiler support for this we are going to continue running into such troubles. I know that @nojb was looking at adding the feature we need in the compiler, so TBH I would rather wait than add re_export everywhere.