Unexpected import behavior

[noughtmare]

I just tried this library out on accelerate. It seems to be much faster than doctest, I think mainly because it doesn't have to recompile everything. However, I'm running into an issue with imports.

For example there is an explicit prelude import (source):

import Prelude                                                      ( ($), (.), Maybe(..), Char )

But I still get a bunch of ambiguous type errors like this one:

/private/tmp/accelerate/src/Data/Array/Accelerate/Language.hs:1380: failure in expression `runExp $ use mat !! 12'
expected: 12
 but got: 
          ^
          <interactive>:574:18: error:
              Ambiguous occurrence ‘!!’
              It could refer to
                 either ‘Data.Array.Accelerate.Language.!!’,
                        imported from ‘Data.Array.Accelerate.Language’
                     or ‘Prelude.!!’,
                        imported from ‘Prelude’ (and originally defined in ‘GHC.List’)

And I also get some errors about missing symbols like this:

/private/tmp/accelerate/src/Data/Array/Accelerate/Sugar/Array.hs:191: failure in expression `fromList (Z:.10) [0..] :: Vector Int'
expected: Vector (Z :. 10) [0,1,2,3,4,5,6,7,8,9]
 but got: 
          ^
          <interactive>:37:11: error: Data constructor not in scope: Z

          <interactive>:37:12: error:
              • Data constructor not in scope:
                  (:.) :: t0 -> t1 -> Data.Array.Accelerate.Sugar.Shape.DIM1
              • Perhaps you meant variable ‘.’ (imported from Prelude)

I have confirmed that the tests do compile if I bind them to a top-level variable in the same module.

[noughtmare]

Okay, a fix for the explicit prelude import is this:

-- $setup
-- >>> :m -Prelude
-- >>> import Prelude (($), (.), Maybe(..), Char )

Would it be possible to do this automatically? Doctest is able to cope with this.

That leaves the out of scope identifier issue. Here are all of them:

/private/tmp/accelerate/src/Data/Array/Accelerate/Sugar/Array.hs:193: failure in expression `fromList (Z:.10) [0..] :: Vector Int'
expected: Vector (Z :. 10) [0,1,2,3,4,5,6,7,8,9]
 but got: 
          ^
          <interactive>:37:11: error: Data constructor not in scope: Z

          <interactive>:37:12: error:
              • Data constructor not in scope:
                  (:.) :: t0 -> t1 -> Data.Array.Accelerate.Sugar.Shape.DIM1
              • Perhaps you meant variable ‘.’ (imported from Prelude)

/private/tmp/accelerate/src/Data/Array/Accelerate/Prelude.hs:1561: failure in expression `run $ concatOn _1 (use m1) (use m2)'
expected: Matrix (Z :. 5 :. 15)
            [  0,  1,  2,  3,  4,  5,  6,  7,  8,  9,  0,  1,  2,  3,  4,
              10, 11, 12, 13, 14, 15, 16, 17, 18, 19,  5,  6,  7,  8,  9,
              20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 10, 11, 12, 13, 14,
              30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 15, 16, 17, 18, 19,
              40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 20, 21, 22, 23, 24]
 but got: 
          ^
          <interactive>:689:16: error:
              • Found hole:
                  _1 :: (Exp Int -> f (Exp Int)) -> Exp DIM2 -> f (Exp DIM2)
                Where: ‘f’ is a rigid type variable bound by
                         a type expected by the context:
                           Lens.Micro.Type.Lens' (Exp DIM2) (Exp Int)
                         at <interactive>:689:16-17
                Or perhaps ‘_1’ is mis-spelled, or not in scope
              • In the first argument of ‘concatOn’, namely ‘_1’
                In the second argument of ‘($)’, namely
                  ‘concatOn _1 (use m1) (use m2)’
                In the expression: run $ concatOn _1 (use m1) (use m2)
              • Relevant bindings include
                  it :: Array DIM2 Int (bound at <interactive>:689:1)
                Constraints include
                  GHC.Base.Functor f (from <interactive>:689:16-17)

/private/tmp/accelerate/src/Data/Array/Accelerate/Prelude.hs:1823: failure in expression `run $ reverseOn _1 (use mat)'
expected: Matrix (Z :. 5 :. 10)
            [  9,  8,  7,  6,  5,  4,  3,  2,  1,  0,
              19, 18, 17, 16, 15, 14, 13, 12, 11, 10,
              29, 28, 27, 26, 25, 24, 23, 22, 21, 20,
              39, 38, 37, 36, 35, 34, 33, 32, 31, 30,
              49, 48, 47, 46, 45, 44, 43, 42, 41, 40]
 but got: 
          ^
          <interactive>:863:17: error:
              • Found hole:
                  _1 :: (Exp Int -> f (Exp Int)) -> Exp DIM2 -> f (Exp DIM2)
                Where: ‘f’ is a rigid type variable bound by
                         a type expected by the context:
                           Lens.Micro.Type.Lens' (Exp DIM2) (Exp Int)
                         at <interactive>:863:17-18
                Or perhaps ‘_1’ is mis-spelled, or not in scope
              • In the first argument of ‘reverseOn’, namely ‘_1’
                In the second argument of ‘($)’, namely ‘reverseOn _1 (use mat)’
                In the expression: run $ reverseOn _1 (use mat)
              • Relevant bindings include
                  it :: Array DIM2 Int (bound at <interactive>:863:1)
                Constraints include
                  GHC.Base.Functor f (from <interactive>:863:17-18)

/private/tmp/accelerate/src/Data/Array/Accelerate/Prelude.hs:1867: failure in expression `run $ transposeOn _1 _2 (use mat)'
expected: Matrix (Z :. 10 :. 5)
            [ 0, 10, 20, 30, 40,
              1, 11, 21, 31, 41,
              2, 12, 22, 32, 42,
              3, 13, 23, 33, 43,
              4, 14, 24, 34, 44,
              5, 15, 25, 35, 45,
              6, 16, 26, 36, 46,
              7, 17, 27, 37, 47,
              8, 18, 28, 38, 48,
              9, 19, 29, 39, 49]
 but got: 
          ^
          <interactive>:890:19: error:
              • Found hole:
                  _1 :: (Exp Int -> f (Exp Int)) -> Exp DIM2 -> f (Exp DIM2)
                Where: ‘f’ is a rigid type variable bound by
                         a type expected by the context:
                           Lens.Micro.Type.Lens' (Exp DIM2) (Exp Int)
                         at <interactive>:890:19-20
                Or perhaps ‘_1’ is mis-spelled, or not in scope
              • In the first argument of ‘transposeOn’, namely ‘_1’
                In the second argument of ‘($)’, namely
                  ‘transposeOn _1 _2 (use mat)’
                In the expression: run $ transposeOn _1 _2 (use mat)
              • Relevant bindings include
                  it :: Array DIM2 Int (bound at <interactive>:890:1)
                Constraints include
                  GHC.Base.Functor f (from <interactive>:890:19-20)

          <interactive>:890:22: error:
              • Found hole:
                  _2 :: (Exp Int -> f (Exp Int)) -> Exp DIM2 -> f (Exp DIM2)
                Where: ‘f’ is a rigid type variable bound by
                         a type expected by the context:
                           Lens.Micro.Type.Lens' (Exp DIM2) (Exp Int)
                         at <interactive>:890:22-23
                Or perhaps ‘_2’ is mis-spelled, or not in scope
              • In the second argument of ‘transposeOn’, namely ‘_2’
                In the second argument of ‘($)’, namely
                  ‘transposeOn _1 _2 (use mat)’
                In the expression: run $ transposeOn _1 _2 (use mat)
              • Relevant bindings include
                  it :: Array DIM2 Int (bound at <interactive>:890:1)
                Constraints include
                  GHC.Base.Functor f (from <interactive>:890:22-23)

[martijnbastiaan]

On the import issue: doctest loads a module including all its imports. doctest-parallel works on compiled code, so it will only bring exported symbols in scope (equivalent to import Some.Module where Some.Module is the module it will test). In theory it could scan the import list from the source and repeat those in GHCi, but I decided against this for two (edit: three) reasons:

1. While it might make sense to have all imported symbols in scope while writing doctests, IMO it does not make sense for readers of doctests. That is, readers shouldn't have to view the source code in order to find out where a symbol used in the doctest is coming from. Instead, writers should be explicit about where symbols are coming from in $setup sections.
2. Some import statements might fail in GHCi, as it can only access exposed modules.
3. Edit: In the future I'd like to move away from scanning source code to discover tests. Soon(tm) Haddock comments will be stored in compiled modules, eliminating the need for doctest-parallel to read source code at all!

I think the second issue is similar: because it's not exported by the module under test, it is not in scope. Is that correct?

In theory it could scan the import list from the source and repeat those in GHCi

I could make this a flag enabling this behavior though, if you're not convinced by my reasoning :-)

[martijnbastiaan]

In any case, it'd probably be a good idea to add a migration guide for people coming from doctest.

[noughtmare]

Thanks for the explanation. Now I understand better what is happening. I have been able to make all the errors go away by importing more modules in the $setup.

I think I mostly agree with the idea that doctests should only test the public interface. There are two nuances:

1. In one of the core libraries, accelerate defines a few basic lenses locally without exporting them. Users are expected to import those lenses from another package. The core package shouldn't depend on that external package.
2. Some doctests from non-exposed modules can still be exposed if they are re-exported in an exposed module. So, the module that contains the doctests does not have to be listed under exposed-modules.

1 can be fixed by duplicating the definitions of those lenses in the $setup block. And for 2 I simply moved the modules to exposed-modules (there were only two), but perhaps a better solution would be to make an internal library and import that in the public library and in the tests separately.

In any case, it'd probably be a good idea to add a migration guide for people coming from doctest.

I think that is a very good idea.

[martijnbastiaan]

Users are expected to import those lenses from another package. The core package shouldn't depend on that external package.

I believe adding lens as a dependency of the doctest, rather than the core library, should work. Could you test that?

[martijnbastiaan]

Some doctests from non-exposed modules can still be exposed if they are re-exported in an exposed module.

That's a good point. I think we should be able to detect whether these are reexported and import from there. I'll have a look after I'm back from Christmas visits :-).

[noughtmare]

I believe adding lens as a dependency of the doctest, rather than the core library, should work. Could you test that?

No, sorry, I didn't explain this very well. It is about these specific lenses _1, _2, and _3, which are not standard lens lenses. Users are expected to import lens-accelerate if they want lenses, but the accelerate package itself can't depend on that because it would lead to cyclic dependencies.

Anyway, happy holidays!

[martijnbastiaan]

I've split this issue up into two: #10 and #11. Can we close this issue @noughtmare? :) Btw, how's the migration to doctest-parallel going for accelerate?

[noughtmare]

Yes, this can be closed.

As for the status, I think I still need to come up with a good solution for the tests in the non-exposed modules. I've now used the quick and dirty solution of just exposing the modules in question, but that probably exposes too much. Other ideas I've had are to move those modules to an internal library or to split those modules into exposed and hidden parts. I'm not a part of the core team, so I will need to discuss those options.

But other than that, I think migration only requires a few small changes such as adding extra imports in $setup blocks. And the performance improvement looks compelling, I measured an improvement from 45 seconds to 9 seconds once.

[martijnbastiaan]

How about something like:

{-# ANN module "doctest-parallel: no implicit import" #-}

Which would make doctest-parallel forgo the implicit import The.Module at the start of a test. You would then be responsible for importing the right symbols in the $setup block.

[noughtmare]

I think that would be a good option if all the functions are exported in the same module, so then I would be able to write that manual import only once in the $setup block. If functions in the same hidden module are exported from different exposed modules then that would require putting manual import statements on all doctests, which doesn't seem worth it.

It seems like the former is actually the case in accelerate, but I'm not certain yet. And I don't even really understand why Data.Array.Accelerate.Prelude is not an exposed module in the first place, so I should really first discuss that with the other developers.

[martijnbastiaan]

@noughtmare I've added an option to stop the implicit imports. It can be set on a per-module basis, see:

https://github.com/martijnbastiaan/doctest-parallel#test-non-exposed-modules

This has landed in main, but is not yet part of a release. I think this should pretty neatly solve your issue :).