This is for discussion for now, particularly the second patch.

(I also still need to get a full test harness run to validate
the claim in the patch message, but the unit tests and a fair
proportion of the shell tests are definitely ok)

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

patch 2343f0cab3c9f7a13702d83fc465eb49bde2c7ba
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Aug 18 15:34:26 BST 2019
  * remove unused functions from D.P.W.Sealed

patch ff075a27c24e92768b0402d95841df9b366f9705
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Aug 27 13:11:42 BST 2019
  * get rid of unsafe unsealing
  
  An unsafe unseal is really just an unsafe coerce of
  an existentially quantified witness, so make that explicit
  and reduce the number of kinds of "unsafe" operations
  we have to understand.
  
  The laziness property of 'seal' doesn't seem to be actually
  needed by any test and is unlikely to have any performance
  impact.

Attachments

• patch-preview.txt
• remove-unused-functions-from-d_p_w_sealed.dpatch
• unnamed

> patch ff075a27c24e92768b0402d95841df9b366f9705
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Tue Aug 27 13:11:42 BST 2019
>   * get rid of unsafe unsealing
>   
>   An unsafe unseal is really just an unsafe coerce of
>   an existentially quantified witness, so make that explicit
>   and reduce the number of kinds of "unsafe" operations
>   we have to understand.
>   
>   The laziness property of 'seal' doesn't seem to be actually
>   needed by any test and is unlikely to have any performance
>   impact.

I disagree. We do rely on the laziness of seal to ensure we don't read
inventories too eagerly. I stumbled over this when I tried to remove the
(apparently useless) "unseal seal <$>" calls in
readRepoFromInventoryList. It turned out they are there to avoid forcing
an inventory due to the pattern matching for Sealed:

  do
    ...
    Sealed ... <- unseal seal <$> ...

IIRC the failure was detected by tests/issue2293-laziness-amend.sh.

>>   The laziness property of 'seal' doesn't seem to be actually
>>   needed by any test and is unlikely to have any performance
>>   impact.
> 
> I disagree. We do rely on the laziness of seal to ensure we don't read
> inventories too eagerly. I stumbled over this when I tried to remove the
> (apparently useless) "unseal seal <$>" calls in
> readRepoFromInventoryList. It turned out they are there to avoid forcing
> an inventory due to the pattern matching for Sealed:
> 
>   do
>     ...
>     Sealed ... <- unseal seal <$> ...
> 
> IIRC the failure was detected by tests/issue2293-laziness-amend.sh.

I see, thanks. That's horrible :-) I shall have a look for a better option.

In the meantime let me know if you have any objections to screening the
first patch, or to the principle of removing unsafeUnseal at least from
the API of D.P.W.Sealed.

> In the meantime let me know if you have any objections to screening the
> first patch, or to the principle of removing unsafeUnseal at least from
> the API of D.P.W.Sealed.

The first patch is fine.

Removing unsafeUnseal is a good move. (unseal unsafeCoerceXXX) is almost
as concise and gives us more fine-grained control over which witness we
coerce. It conflicts with my coalesce cleanup patches (which I sent but
didn't screen yet) because they remove the need for the unsafeUnseal
there. Not a big deal though, just one line, I can rebase them or we
leave the conflict in there and resolve it.

New version of bundle, which I'll screen now.

This version passes the tests and preserves the laziness
of unseal. As discussed it does stop exporting
unsafeUnseal.

Also tried to document the current situation.

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

patch 2343f0cab3c9f7a13702d83fc465eb49bde2c7ba
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Aug 18 15:34:26 BST 2019
  * remove unused functions from D.P.W.Sealed

patch aeeaff120da10d9800f260d01ec8f859b1e39e24
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Sep  2 18:08:29 BST 2019
  * don't export unsafe unseal operations
  
  An unsafe unseal is really just an unsafe coerce of
  an existentially quantified witness, so make that explicit
  and reduce the number of kinds of "unsafe" operations
  we have to understand.
  
  unsafeUnseal[2] is still needed internally to implement
  a suitably lazy unseal/unseal2.
  

patch 61f57743142c4a8e6b51e25d7d590428aa8fc0ea
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Sep  2 20:26:31 BST 2019
  * document the laziness of unseal in more detail

Attachments

• patch-preview.txt
• remove-unused-functions-from-d_p_w_sealed.dpatch
• unnamed

We can follow up on the laziness of unseal in a new patch at some point.
Also just for reference there was extensive discussion of the current
behaviour in patch1880.

> patch 2343f0cab3c9f7a13702d83fc465eb49bde2c7ba
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Sun Aug 18 15:34:26 BST 2019
>   * remove unused functions from D.P.W.Sealed

Nice cleanup.

> patch aeeaff120da10d9800f260d01ec8f859b1e39e24
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Mon Sep  2 18:08:29 BST 2019
>   * don't export unsafe unseal operations
>   
>   An unsafe unseal is really just an unsafe coerce of
>   an existentially quantified witness, so make that explicit
>   and reduce the number of kinds of "unsafe" operations
>   we have to understand.
>   
>   unsafeUnseal[2] is still needed internally to implement
>   a suitably lazy unseal/unseal2.

This is definitely an improvement. It makes some expressions that
formerly used unsafeUnseal superficially more complex but that is just
as well, since we want to avoid unsafeX anyway whenever we can.

> patch 61f57743142c4a8e6b51e25d7d590428aa8fc0ea
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Mon Sep  2 20:26:31 BST 2019
>   * document the laziness of unseal in more detail

Thanks for doing that, much appreciated.

All accepted.