doctest icon indicating copy to clipboard operation
doctest copied to clipboard

doctest doesn't discover intra-module code when passed -fobject-code and given multiple targets

Open RyanGlScott opened this issue 7 years ago • 13 comments

I originally discovered this issue when trying to make doctest work with the structs library. Here is a minimized example that exhibits the problem using two Haskell modules:

module Internal () where
{-# LANGUAGE UnboxedTuples #-}
module Label where

-- $setup
-- >>> :set -XUnboxedTuples

-- |
-- >>> case foo 42 of (# x #) -> x == 42
-- True
foo :: Int ->  (# Int #)
foo x = (# x #)

Since Label uses UnboxedTuples, you have to pass -fobject-code to doctest to make its tests work. Running doctest on Label alone works:

$ doctest -fobject-code Label.hs 
Examples: 2  Tried: 2  Errors: 0  Failures: 0

However, if you also pass in Internal as an argument to doctest, then something strange happens:

$ doctest -fobject-code Internal.hs Label.hs
### Failure in Label.hs:8: expression `case foo 42 of (# x #) -> x == 42'
expected: True
 but got: 
          <interactive>:31:6: error:
              Variable not in scope: foo :: Integer -> (# Integer #)
Examples: 2  Tried: 2  Errors: 0  Failures: 1

For some reason, doctest fails to discover the foo function when testing Label, despite the fact that foo is defined in Label itself! Even stranger, the order of modules matters here, since passing Label then Internal works:

$ doctest -fobject-code Label.hs Internal.hs
Examples: 2  Tried: 2  Errors: 0  Failures: 0

Unfortunately, the test suite in structs happens to always put Internal before Label when running doctest, so this issue is hard to avoid. A workaround is to manually import Label itself, like so:

{-# LANGUAGE UnboxedTuples #-}
module Label where

-- $setup
-- >>> :set -XUnboxedTuples
-- >>> import Label

-- |
-- >>> case foo 42 of (# x #) -> x == 42
-- True
foo :: Int ->  (# Int #)
foo x = (# x #)
$ doctest -fobject-code Internal.hs Label.hs
Examples: 3  Tried: 3  Errors: 0  Failures: 0

RyanGlScott avatar May 05 '17 14:05 RyanGlScott

Hm, I wonder if this is GHC's fault. I tried loading these modules into GHCi and seeing if foo was in scope:

$ ghci -fobject-code Label.hs
GHCi, version 8.0.2: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/ryanglscott/.ghci
[1 of 1] Compiling Label            ( Label.hs, Label.o )
Ok, modules loaded: Label (Label.o).
λ> :t foo
foo :: Int -> (# Int #)
$ ghci -fobject-code Label.hs Internal.hs 
GHCi, version 8.0.2: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/ryanglscott/.ghci
[1 of 2] Compiling Label            ( Label.hs, Label.o )
[2 of 2] Compiling Internal         ( Internal.hs, Internal.o )
Ok, modules loaded: Internal (Internal.o), Label (Label.o).
λ> :t foo
foo :: Int -> (# Int #)
$ ghci -fobject-code Internal.hs Label.hs
GHCi, version 8.0.2: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/ryanglscott/.ghci
[1 of 2] Compiling Label            ( Label.hs, Label.o )
[2 of 2] Compiling Internal         ( Internal.hs, Internal.o )
Ok, modules loaded: Internal (Internal.o), Label (Label.o).
λ> :t foo

<interactive>:1:1: error: Variable not in scope: foo

Sure enough, foo was out of scope when I gave it Internal.hs Label.hs, but it worked with other combinations.

RyanGlScott avatar May 05 '17 15:05 RyanGlScott

But then again, I don't think GHCi alone can explain the odd behavior in doctest. If I define another module which doesn't require -fobject-code to interpret, e.g.,

module Bar where

-- |
-- >>> bar == bar
-- True
bar :: Int
bar = 42

Then I can run doctest on it in whatever order I want without issue:

$ doctest Bar.hs
Examples: 1  Tried: 1  Errors: 0  Failures: 0
$ doctest Bar.hs Internal.hs
Examples: 1  Tried: 1  Errors: 0  Failures: 0
$ doctest Internal.hs Bar.hs
Examples: 1  Tried: 1  Errors: 0  Failures: 0

But at the same time, I also observe the same scoping issue if I try to load different combinations of Internal and Bar into GHCi:

$ ghci Bar.hs
GHCi, version 8.0.2: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/ryanglscott/.ghci
[1 of 1] Compiling Bar              ( Bar.hs, interpreted )
Ok, modules loaded: Bar.
λ> :t bar
bar :: Int
$ ghci Bar.hs Internal.hs 
GHCi, version 8.0.2: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/ryanglscott/.ghci
[1 of 2] Compiling Internal         ( Internal.hs, interpreted )
[2 of 2] Compiling Bar              ( Bar.hs, interpreted )
Ok, modules loaded: Bar, Internal.
λ> :t bar
bar :: Int
$ ghci Internal.hs Bar.hs
GHCi, version 8.0.2: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/ryanglscott/.ghci
[1 of 2] Compiling Internal         ( Internal.hs, interpreted )
[2 of 2] Compiling Bar              ( Bar.hs, interpreted )
Ok, modules loaded: Bar, Internal.
λ> :t bar

<interactive>:1:1: error: Variable not in scope: bar

We have the same issue of bar being out of scope when running ghci Internal.hs Bar.hs. So something must be going on in doctest to explain why this sort of thing works fine without -fobject-code, but doesn't otherwise.

RyanGlScott avatar May 05 '17 15:05 RyanGlScott

I believe https://github.com/sol/doctest/issues/104#issuecomment-99483481 is another occurrence of this bug in the wild.

RyanGlScott avatar May 05 '17 15:05 RyanGlScott

OK, I think I know what is going on here. doctest loads modules into GHCi with this code:

      void $ Interpreter.safeEval repl $ ":m *" ++ module_

But the * prefix tries to load the module in interpreted mode. If do this directly in GHCi, you'll discover that it actually emits an error when -fobject-code is enabled:

$ ghci -fobject-code Internal.hs Label.hs
GHCi, version 8.0.2: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/ryanglscott/.ghci
Ok, modules loaded: Internal (Internal.o), Label (Label.o).
λ> :m *Label
module 'Label' is not interpreted; try ':add *Label' first
λ> :t foo

<interactive>:1:1: error: Variable not in scope: foo

It appears that doctest was swallowing the "not interpreted" error, which explains why foo is never brought into scope.

RyanGlScott avatar May 05 '17 17:05 RyanGlScott

The question now becomes: is it possible to fix this? You could use :m Label instead of :m *Label when -fobject-code is enabled, which would bring foo into scope:

$ ghci -fobject-code Internal.hs Label.hs
GHCi, version 8.0.2: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/ryanglscott/.ghci
Ok, modules loaded: Internal (Internal.o), Label (Label.o).
λ> :m Label
λ> :t foo
foo :: Int -> (# Int #)

That's a much better situation than the present. But it's still not an ideal situation, because one of the things that doctest can do is refer to internal definitions in doctests, e.g.,

module Bar (bar) where

-- |
-- >>> bar == baz
-- True
bar :: Int
bar = baz

baz :: Int
baz = 42
$ doctest Bar.hs
Examples: 1  Tried: 1  Errors: 0  Failures: 0

But trying to do something similar with -fobject-code won't work:

{-# LANGUAGE UnboxedTuples #-}
module Label (foo) where

-- $setup
-- >>> :set -XUnboxedTuples

-- |
-- >>> case bar 42 of (# x #) -> x == 42
-- True
foo :: Int -> (# Int #)
foo = bar

bar :: Int ->  (# Int #)
bar x = (# x #)
$ doctest -fobject-code Label.hs
### Failure in Label.hs:8: expression `case bar 42 of (# x #) -> x == 42'
expected: True
 but got: 
          <interactive>:31:6: error:
              Variable not in scope: bar :: Integer -> (# Integer #)
Examples: 2  Tried: 2  Errors: 0  Failures: 1

This makes sense, since the object code only contains definitions which are exported, and Label.bar isn't exported. But due to the presence of UnboxedTuples, you can't just load Label in interpreted mode, so as far as I can tell, there's no way around this (unless you were to somehow compile a variant of Label which exports all definitions...)

What should we do here? Should we use :m instead of :m * when -fobject-code is used, and put a note in the documentation warning that internal definitions can't be used in combination with -fobject-code? Or can you see a better solution?

RyanGlScott avatar May 05 '17 17:05 RyanGlScott

I think the correct resolution is to use :m with -fobject-code but not support referring to internal definitions if you have to use it.

mpickering avatar Jun 28 '19 10:06 mpickering

I didn't comment at the time, but when I tried prototyping this idea about two years ago, I ran into serious difficulties trying to make it work with modules that enable TemplateHaskell which, for some reason, the GHC API believed to require the use of -fobject-code. Unfortunately, I no longer have the patch in front of me, so I'd have to write it again from scratch to produce the exact error, which likely won't happen until next week at the earliest. (Unless you can do so before then, for some value of "you".)

RyanGlScott avatar Jun 28 '19 11:06 RyanGlScott

-fobject-code having automatic non-obvious effects that can break code spooks me.

As noted, auto-switching to :m isn't a Pareto improvement: it'll break some things even as it fixes others. Unless it's very clear that the fixage is bigger than the breakage, I'm very hesitant to do this.

AIUI, this whole issue's caused by a non-fundamental GHC limitation, that unboxed * don't work in GHCi, right? That makes me doubly hesitant about hacking around it in doctest user-visibly.

Detecting -fobject-code and, instead of auto-switching, issuing a warning is a possibility. Detecting uses of non-exported variables is probably rather harder.

#217 implements manual switching as a flag. I think if that gets merged, plus an intelligible warning about -fobject-code, users are being as best served by doctest as they could be.

quasicomputational avatar Jun 28 '19 12:06 quasicomputational

@quasicomputational What "automatic non-obvious effects" are you precisely referring to?

mpickering avatar Jun 28 '19 12:06 mpickering

Sorry, maybe I've misinterpreted. I thought that it was being suggested that doctest would toggle between :m and :m * depending on if -fobject-code was in play or not. Then, the addition of that flag somewhere that gets propagated to doctest will break any tests that use non-exported definitions, and likely break them with an error message that makes it hard to link cause and effect.

quasicomputational avatar Jun 28 '19 13:06 quasicomputational

I thought that it was being suggested that doctest would toggle between :m and :m * depending on if -fobject-code was in play or not.

That is what I was suggesting, yes.

Then, the addition of that flag somewhere that gets propagated to doctest will break any tests that use non-exported definitions, and likely break them with an error message that makes it hard to link cause and effect.

This is true, but at the same time, I don't think #217 is a better solution. The problem with #217 is that is uses :m + globally, which imposes a compilation performance penalty on the entire project. I'd prefer to only use :m + in those places that absolutely need them, and nowhere else.

This will become even more relevant in a future version of GHC that ships this commit, which automatically enables -fobject-code (on a module-by-module basis) whenever UnboxedTuples is enabled.

RyanGlScott avatar Jun 28 '19 13:06 RyanGlScott

Won't passing -fobject-code on the command line also affect every module that doctest's invoked on? That is, I don't see why doctest -fobject-code with auto-switching would be any faster than doctest --no-interpret.

I appreciate that -fobject-code can be enabled in ways that doctest can't detect, but (for me) that makes it more important to give the user some way of controlling :m + versus :m *, since an automatic solution won't work reliably. (And, unfortunately, that also means that a warning can't be reliably issued. Unless we do something ugly like try to parse GHCi's output - hmm!)

At the same time, I do appreciate that a global switch is a big hammer - much bigger than ideal. So, another straw proposal: teaching doctest about a pragma, say {-# DOCTEST_MODE (interpreted|object-code) #-}, for per-module toggling.

quasicomputational avatar Jun 28 '19 13:06 quasicomputational

Having something like DOCTEST_MODE would definitely be nice as an analog to OPTIONS_GHC.

RyanGlScott avatar Jun 28 '19 14:06 RyanGlScott