Fix issues with parametricity. Collapse successive NonNulls.

[ajnsit]

This PR fixes the issues raised in #7, by NOT collapsing a (Nullable (Nullable a)) into Nullable a. This is done by have a different representation for nested nulls (called WrappedNull).

However, there is no issue with collapsing successive notNull into a single value, as the type system protects us from that. So this PR also represents a deeply nested non null value as the value itself. This allows us to continue to pass a Nullable (Nullable a) (and so on to any depth) directly to a JS function which expects an a, as long as the JS function correctly handles the case of a deeply nested null. This I think is an acceptable tradeoff, since we recover parametricity with only a small usecase requiring additional work on the JS side.

I have added a new test case that checks that the example of breaking parametricity in #7 now behaves correctly.

NOTE: This is how things are handled in Bucklescript (https://github.com/BuckleScript/bucklescript/blob/master/lib/js/caml_option.js). And that directly inspired this PR.

[garyb]

This seems like it would somewhat defeat the purpose of Nullable as I understand it? In that it's supposed to be a convenient way to handle JS nulls without having to handle it in the FFI.

[ajnsit]

I wouldn’t say that. This still handles the null/non-null values the same way. null :: Nullable Int => null null :: Nullable (Nullable Int) => null ... notNull(1) => 1 notNull(notNull(1)) => 1 ... I would argue that that covers 99% of the use cases. The only case that’s different is - notNull(null) => customRepresentation. notNull(notNull(null)) => customRepresentation ... The documentation already warns against wrapping a Nullable value into Nullable again. If one follows that then this case will never occur. So we are already better off than the current implementation. Except now, we are actually able to nest Nullable values and have - 1. Guaranteed safety on the Purescript side no matter what we do. 2. Ability to work seamlessly in JS with wrapped nonNull values. 3. Ability to handle wrapped null values in JS manually if needed.

…

-- Sent from my hyper-communicator

[garyb]

Ah, I see! I was thinking the wrapped representation was always present.

[hdgarrood]

I'm not really keen on this, as I think it departs from what Nullable is for. It's also a breaking change, because the runtime representation of Nullable is part of its public API. The purpose of Nullable is really just for marshalling on API boundaries, it's not intended as a general-purpose data type for representing possible absence of a value. I think this change risks expanding the remit of Nullable to overlap with what Maybe is, because with this change Nullable gets the ability to do everything Maybe does. To me, that's potentially a problem, because if people start using Nullable as a general purpose "this might be absent" data type, it hurts code reusability across the ecosystem, as then we'll find that we have to convert between Nullable and Maybe all the time at library boundaries if we are ever using libraries which use both Nullable and Maybe together.

[ajnsit]

If JS were the only target for Purescript then I would have said that this module should replace Maybe. Because as you said, this covers everything that Maybe can do. Also agree about this change breaking the public interface. However, I do believe that losing parametricity is too big a price to pay for compatibility. Purescript code should never be unsafe.

I'm not sure what's the way out here. Should I create a new library? But then we're looking at three ways to construct optional types.

[hdgarrood]

Create a separate library if you want, but I would rather we didn't have something which tried to replace Maybe - I don't think it would be good for the ecosystem, for the compatibility reasons I mentioned.

Also, I think the idea that PureScript code should never be unsafe is nice, but not really workable in practice. This library is only intended for use with the FFI, which is always very unsafe to begin with - and this is why I think it's worth paying the price of this parametricity violation in exchange for a simple API and simple JS runtime representation. Also, library authors will occasionally need to resort to things like unsafeCoerce, unsafePerformEffect, unsafePartial, or similar, for performance reasons or otherwise. Saying that unsafe code should never occur in app code is perhaps more realistic, I think, but that still should be left up to the people maintaining those apps.

[garyb]

My 2p: I feel like Nullable and Foreign were a mistake now. I'm much more of a fan of just dealing with FFI stuff like this directly in FFI bindings - in theory it's nice to not have to write JS to handle these things, but in practice writing it in PS isn't all that much better.

Additionally, the types appearing in PS means the temptation is there to use them anywhere other than in a very thin marshalling layer means that they can have unintended consequences since they don't behave like normal PS data, etc.

[ajnsit]

@hdgarrood I agree that unsafe code is sometimes necessary. However, in this case it's not necessary, and I don't think the right way to prevent adoption of something is to make it unsafe.

There is a lot of user code using Nullable in their data definitions, easily found using a github search. I think the adoption battle is already lost.

However, if the official stance is that this library should go away, and usage is discouraged, then I will go ahead and create a new repo, since it seems to fill a need. There are already libraries like https://github.com/rightfold/purescript-nullable-safe which aim to make nullable type-safe, and which are also being used in user code, however which provide only 50% of the functionality of this PR with zero additional benefits and force you to use artificial constraints on your functions.

[hdgarrood]

That’s fine - please go ahead and create any library you wish to - but I think you have misunderstood my stance. I am saying that the current design is the most appropriate for satisfying the need which this library aims to satisfy: handing nulls on JS FFI boundaries. To give a concrete example: if you do happen to end up with a nested Nullable for whatever reason, and the value does turn out to be null, and you want to pass that value into a JS library, this library currently will do what you want, but with the nested-null representation of this PR it will almost certainly not do what you want.

As it happens I’m not entirely convinced by @garyb’s view, I have found these kinds of libraries useful. I personally am also skeptical of Foreign, but imo libraries like Nullable, Data.Function.Uncurried, Effect.Uncurried, and whichever one it is that safely converts between Aff and Promise are useful and should continue to exist.

The fact that people are using Nullable in data definitions does not mean the battle has been lost; if those data definitions are being used for FFI imports then that’s how it was intended to be used! Yes there’s a temptation to use Nullable in more of your PureScript code than you perhaps should, but to me, this is a fault of the FFI, not of Nullable: using the FFI safely always requires effort and care.

I would prefer not to see widespread adoption of a general purpose “this might be absent” data type based on your design here, but that’s a fundamentally different thing to what this library is currently, and it isn’t something which exists in the core libraries currently. So really I’m saying that the core libraries should not provide this thing; I don’t think the characterisation of ‘preventing adoption by making it unsafe’ is entirely fair.