darcs

Patch 1850 introduce TestOnly class

Title introduce TestOnly class
Superseder Nosy List ganesh
Related Issues
Status accepted Assigned To
Milestone

Created on 2019-07-14.18:11:55 by ganesh, last changed 2019-08-20.15:09:16 by bf.

Files
File name Status Uploaded Type Edit Remove
introduce-testonly-class.dpatch ganesh, 2019-07-14.18:11:55 application/x-darcs-patch
introduce-testonly-class.dpatch ganesh, 2019-08-09.18:20:34 application/x-darcs-patch
patch-preview.txt ganesh, 2019-07-14.18:11:55 text/x-darcs-patch
patch-preview.txt ganesh, 2019-07-16.10:35:31 text/x-darcs-patch
patch-preview.txt ganesh, 2019-08-09.18:20:34 text/x-darcs-patch
unnamed ganesh, 2019-07-14.18:11:55 text/plain
unnamed ganesh, 2019-07-16.10:35:31 text/plain
unnamed ganesh, 2019-08-09.18:20:34 text/plain
use-testonly-for-the-constructors-of-repopatchv3.dpatch dead ganesh, 2019-07-16.10:35:31 application/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg20937 (view) Author: ganesh Date: 2019-07-14.18:11:55
I won't screen this immediately.

This is a proof of concept for the idea of flagging TestOnly
code we've recently discussed.

Comments on the general concept as well as the specific
names/structure welcome.

1 patch for repository /home/ganesh/darcs/screened-temp:

patch 1c1bc79b9f3d5a1edb7c04dfd12c75db64ac6ca2
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Jul 14 19:05:07 BST 2019
  * introduce TestOnly class
  
  This allows us to explicitly flag test-only code in the
  main darcs library.
Attachments
msg20943 (view) Author: bf Date: 2019-07-15.16:51:50
(I was a bit astonished that defining a nullary type class requires MultiParamTypeClasses, though there is a certain logic to that...)

My only comment wrt the general idea is that I see no way to use this technique to flag export of data constructors: it works only if we add dedicated functions that aren't used internally. This can become a bit heavy-weight especially for GADTs.

Here is a completely different idea: we add WARNING pragmas. I just tested this for RepoPatchV3: In src/Darcs/Patch/V3/Core.hs I added 

{-# WARNING Prim, Conflictor, Rotcilfnoc "Data constructors of RepoPatchV3 are only exported for testing" #-}

Then I get warnings like this:

harness/Darcs/Test/Patch/Properties/RepoPatchV3.hs:49:28: warning: [-Wdeprecations]
    In the use of data constructor ‘Conflictor’
    (imported from Darcs.Patch.V3.Core):
    "Data constructors of RepoPatchV3 are only exported for testing"
   |
49 | prop_consistentConflictor (Conflictor _ x p)
   |                            ^^^^^^^^^^

If I add

  ghc-options:      -Wno-warnings-deprecations

in darcs.cabal in the test-suite section, then all is fine.

The same trick could be used for PrimPatchId instead of adding unsafePrimPatchId. We could streamline the warning message to e.g. "TEST CODE ONLY", since ghc already gives us the source location and highlights the identifier for us.
msg20944 (view) Author: bf Date: 2019-07-15.16:55:31
Sorry, looks like editing messages in firefox no longer breaks long lines. Strange, always worked for me before.
msg20945 (view) Author: ganesh Date: 2019-07-15.18:15:41
On 15/07/2019 17:51, Ben Franksen wrote:

> My only comment wrt the general idea is that I see no way to use
> this technique to flag export of data constructors: it works only if
> we add dedicated functions that aren't used internally. This can
> become a bit heavy-weight especially for GADTs

pattern synonyms seem to work (I guess bidirectional ones would too), e.g.:

  pattern PrimP
    :: TestOnly => NamedPrim prim wX wY -> RepoPatchV3 prim wX wY
  pattern PrimP prim <- Prim prim

But it is still boilerplate.

> Here is a completely different idea: we add WARNING pragmas.

The disadvantage of these is that they aren't transitive, and that
disabling them means disabling deprecations from other imports too.

Perhaps I also find TestOnly a bit more elegant, but I'm probably biased
by having come up with it recently.
msg20947 (view) Author: bf Date: 2019-07-16.08:58:23
> pattern synonyms seem to work (I guess bidirectional ones would too), e.g.:
> 
>   pattern PrimP
>     :: TestOnly => NamedPrim prim wX wY -> RepoPatchV3 prim wX wY
>   pattern PrimP prim <- Prim prim
> 
> But it is still boilerplate.

Could you extend this example (unidirectional) to cover all
constructors, including the single use site in the harness, ideally as a
patch? It may be the (syntactic) overhead isn't as bad as I thought. The
main difficulty may be to come up with a suitable naming convention.

>> Here is a completely different idea: we add WARNING pragmas.
> 
> The disadvantage of these is that they aren't transitive,

True. Though I guess we would fix any such warnings soon and not wait
until other code depends on it.

> and that
> disabling them means disabling deprecations from other imports too.

This is really a disadvantage. I thought we could live with it for the
test-suite, but it is clearly not ideal. We could lobby ghc HQ to add a
more fine-grained disabling of warnings coming from WARNING pragmas...

> Perhaps I also find TestOnly a bit more elegant, but I'm probably biased
> by having come up with it recently.

I am not and I agree that it is more elegant.
msg20949 (view) Author: ganesh Date: 2019-07-16.10:35:31
Here's a full patch showing how to use TestOnly for
the constructors of RepoPatchV3

1 patch for repository darcs-unstable@darcs.net:screened:

patch 7365e0511a860bbbbd7b40a55992fc12dda4650d
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Jul 15 20:45:18 BST 2019
  * use TestOnly for the constructors of RepoPatchV3
Attachments
msg21047 (view) Author: ganesh Date: 2019-08-06.20:14:38
Shall we go ahead with TestOnly?
msg21052 (view) Author: bf Date: 2019-08-08.17:10:34
> Shall we go ahead with TestOnly?

Yes, go ahead. It may not be perfect (what ever is?) but it's better
than the pragma which I agree is overkill. I also like that it is quite
unintrusive.
msg21067 (view) Author: ganesh Date: 2019-08-09.18:20:34
Slightly updated version of the second patch to reflect
recent changes in the code. Will screen now.

2 patches for repository darcs-unstable@darcs.net:screened:

patch 1c1bc79b9f3d5a1edb7c04dfd12c75db64ac6ca2
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Jul 14 19:05:07 BST 2019
  * introduce TestOnly class
  
  This allows us to explicitly flag test-only code in the
  main darcs library.

patch 40cbe0f55c03ba686d9371d8d08fc6073deb28f2
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Aug  9 19:22:48 BST 2019
  * use TestOnly for the constructors of RepoPatchV3
Attachments
History
Date User Action Args
2019-07-14 18:11:55ganeshcreate
2019-07-15 16:51:51bfsetmessages: + msg20943
2019-07-15 16:55:31bfsetmessages: + msg20944
2019-07-15 18:15:41ganeshsetmessages: + msg20945
2019-07-16 08:58:23bfsetmessages: + msg20947
2019-07-16 10:35:31ganeshsetfiles: + patch-preview.txt, use-testonly-for-the-constructors-of-repopatchv3.dpatch, unnamed
messages: + msg20949
2019-07-29 16:50:29bfsetstatus: needs-screening -> in-discussion
2019-08-06 20:14:38ganeshsetmessages: + msg21047
2019-08-08 17:10:34bfsetmessages: + msg21052
2019-08-09 12:04:18ganeshsetstatus: in-discussion -> needs-screening
2019-08-09 18:20:34ganeshsetfiles: + patch-preview.txt, introduce-testonly-class.dpatch, unnamed
messages: + msg21067
2019-08-09 20:43:05ganeshsetstatus: needs-screening -> needs-review
2019-08-20 15:09:16bfsetstatus: needs-review -> accepted