Reconsider unsafePartialBecause

[hdgarrood]

I really like the idea of unsafePartialBecause, but I think it makes an erroneous assumption that the try-catch block it uses will always catch errors arising from incorrect use of whatever thing it is you're passing to it. This is often the case but not always:

module Main where

import Prelude
import Control.Monad.Eff (Eff)
import Control.Monad.Eff.Console (log)
import Control.Monad.Eff.Exception (EXCEPTION, catchException, message)
import Partial.Unsafe (unsafePartialBecause)
import Unsafe.Coerce (unsafeCoerce)

data Foo
  = Foo
  | Bar

f :: Partial => Foo -> String
f Foo = "Foo"

main = do
  log "Trying for the first time:"
  catchAndPrint (\_ -> unsafePartialBecause "1" (f Bar))
  log "Trying for the second time:"
  catchAndPrint (\_ -> (unsafePartialBecause "2" f) Bar)

catchAndPrint act = do
  catchException (\err -> log ("Caught: " <> message err)) (evaluate act >>= log)

evaluate :: forall eff a. (Unit -> a) -> Eff (err :: EXCEPTION | eff) a
evaluate = unsafeCoerce

In general it's not safe to assume that the try-catch block will be invoked if the provided function is used incorrectly. Note that the example above is not the only way it can bite you: since functions with a Partial constraint are not required to throw on invalid inputs, these functions will cause unsafePartialBecause to not work as expected either. Take e.g. unsafeIndex from arrays, which returns undefined on an invalid input.

Another issue is that unsafePartialBecause will not interact well with code that uses e.g. catchException.

At the very least we need to update the docs, but I think this behaviour is unintuitive and potentially confusing enough that we should consider removing it altogether.

Thoughts? /cc @chexxor @sharkdp

[hdgarrood]

Oops, sorry, the output of the above program is:

Trying for the first time:
Caught: unsafePartial failed. The following assumption was incorrect: '1'.
Trying for the second time:
Caught: Failed pattern match at Main line 15, column 1 - line 17, column 1: Bar

[sharkdp]

Those are some valid concerns.

The only counter-argument I can provide is that unsafePartialBecause still has an unsafe prefix, i.e. one should always be very careful when using such functions. This does not mean that I don't agree with you. Adding a warning to the docs would be a good solution for me, but I'm also fine with removing the function alltogether if there is no real fix for the mentioned problems.

[hdgarrood]

I'm also fine with removing the function alltogether if there is no real fix for the mentioned problems.

Sadly I don't think there is, as I think the current implementation of unsafePartialBecause makes two assumptions which we can't make valid — those being: 1) a partial function will always throw on an invalid input, 2) exceptions are never thrown during 'normal' (correct) operation of a program.

[sharkdp]

Agreed.

There is a rather silly solution:

unsafePartialBecause :: forall a. String -> (Partial => a) -> a
unsafePartialBecause _ = unsafePartial

which would still serve the main purpose (documentation at the call site)... but then again I can also use a comment :-)

[paf31]

There is another approach which is to change the evidence for Partial to be the reason string itself. Then we can use the type system to plumb those from the call site to the place where we call crash. Then crash can include the explanation for why we thought partiality was okay.

[safareli]

I was stuck in this for a while, I had a function like this:

mustBeMounted :: Maybe Unit -> Unit
mustBeMounted = unsafePartialBecause "children must be mounted" fromJust

And was using it in couple places, to get error with the message, if I made some mistake during development. Then once I made the mistake, I got an error without expected message, I was confused and spent some time trying to identify the issue.

Then changing that code to:

mustBeMounted a = unsafePartialBecause "children must be mounted" (fromJust a)

And with this change, I got expected message in error.

So we definitely need to add a comment or something about this behaviour until this is fixed in some other way

[hdgarrood]

@paf31 That suggestion sounds interesting, but I suppose it would require changes to the language and compiler; do you want to investigate that and/or add an issue on the compiler issue tracker?

[hdgarrood]

Shall we change it to this and deprecate it for now?

unsafePartialBecause :: forall a. String -> (Partial => a) -> a
unsafePartialBecause _ = unsafePartial

[paf31]

Yes I think that's best.

[hdgarrood]

That code doesn't typecheck and I'm not sure why:

  15  unsafePartialBecause _ = unsafePartial
                               ^^^^^^^^^^^^^
  
  Could not match constrained type
  
    Partial => a0
  
  with type
  
    Partial => t1
  
  while trying to match type Partial => a0
    with type Partial => t1
  while checking that expression unsafePartial
    has type (Partial => a0) -> a0
  in value declaration unsafePartialBecause
  
  where a0 is a rigid type variable
          bound at line 15, column 1 - line 15, column 39
        t1 is an unknown type

Can you see what's going on here?

[paf31]

I think you need to eta reduce unfortunately:

unsafePartialBecause _ x = unsafePartial x