freeze Array.empty, closes #979

[gcanti]

No description provided.

[gcanti]

would be a breaking change (breaks fp-ts itself)

[raveclassic]

But freezing is also a breaking change: empty.push() now throws

[gcanti]

That's the very purpose of @sutarmin's suggestion, I guess. empty is not supposed to be modified

[gkamperis]

@gcanti But theoretically should not all returned arrays by fp-ts be frozen to avoid mutation?

Is 'empty' to be considered a global constant like 'null'?

[gcanti]

But theoretically should not all returned arrays by fp-ts be frozen to avoid mutation?

@gkamperis I don't think so, why you say that? Personally I consider all arrays (readonly or not) immutable (except when I need local mutation, of course), but nothing stops you to do

import * as A from 'fp-ts/lib/Array'

const as = A.takeLeft(2)([1, 2, 3, 4])
as.push(5)

Is 'empty' to be considered a global constant like 'null'?

Kind of. Maybe the best analogy is with undefined: you can modify it, but you are not advised to.

[gkamperis]

@gcanti The question is a bit philosophical... I am saying this because immutability and purity go hand in hand.

Theoretically any array (any value really) returned by an FP function should be immutable by the system and not left as a discipline on the developer. If the developer wants to mutate it they should explicitly take the step and in this case they can easily spread it.

Once again, a theoretical/philosophical question...

Kind of. Maybe the best analogy is with undefined: you can modify it, but you are not advised to.

So in the new "empty" implementation how can you modify it? it is frozen.

By freezing it, it has become a global constant reference that any fp-ts function that returns an empty array can re-use safely (and a bit optimally as well).

Don't get me wrong, I am not arguing against this change, I am just trying to figure out what the meaning of "empty" is.

[gcanti]

empty is a shared value so it shouldn't be modified

[any value] should be immutable by the system and not left as a discipline on the developer

I agree, we could ban

• Array in favor of ReadonlyArray
• Set in favor of ReadonlySet
• Map in favor of ReadonlyMap
• NonEmptyArray in favor of ReadonlyNonEmptyArray
• Record in favor of ReadonlyRecord (to be defined)
• Tuple in favor of ReadonlyTuple (to be defined)
• other?

across the whole codebase.

My only concern is about ergonomics, does it worth it?

[gkamperis]

@gcanti again as a theoretical conversation

Immutability always introduces more difficult ergonomics.
And the Readonly prefix is long-winded...
I do not think you need to prefix everything with ReadonlyXXXXX.
(I'd do that only if I wanted to support both modes Array and ReadonlyArray.)

Since you are talking about removing the mutable version then you just need to state in the docs that all these structures are read-only.

Similar to what you did for stack safety.

It becomes a working assumption of the lib.

[gkamperis]

@gcanti but having said all that I do also think that the lib is fine as it is. :)

[raveclassic]

That's the very purpose of @sutarmin's suggestion

I get it by I doubt it should be the responsibility of the lib instead of the developer.

empty is not supposed to be modified

It should be restricted by its interface then, not by fragile and inconsistent convention. By inconsistent I mean exactly what @gkamperis suggests - we should freeze everything, not only arrays, and always use Readonly* interfaces. Yes, DX is terrible but it follows the interface. If I get a mutable value I expect all methods to work properly without exceptions.

[gkamperis]

@raveclassic I kind of disagree... in this case empty is more like rxjs's empty().

maybe it should become a function similar to the rxjs empty?

rxjs empty

but you do not need to prefix everything with Readonly if the lib has the default assumption that everything is read-only/frozen.

I can see @gcanti point that "it doesn't matter if I return a mutable structure"...
if fp-ts internals do not mutate then you allow the developer some flexibility.
After all fp-ts does not need to be involved with state management...

On the other hand if everything turns frozen then you might avoid dev code that mutates structures within a pipe() like I saw one question asked here...

[raveclassic]

empty is more like rxjs's empty()

Hardly, because rxjs empty is still an Observable.
I'd say that now it's more like a Subject with next method throwing suddenly because "rxjs-based code is assumed to be declarative instead of imperative by convention".

if the lib has the default assumption

Personally I'd like to avoid any kind of assumptions and/or conventions and to achieve promised correctness through strict interfaces not producing any side effects like unexpected exceptions. If we want an immutable structure then let's define immutability in its interface.

After all fp-ts does not need to be involved with state management...

Not sure I'm following. What kind of state management are you talking about? The discussion is about a broken contract. Before it things went bad if someone suddenly mutated empty constant. But that was up to developer, not to the lib. The lib provided Array interfaces that allowed mutation wether you like it or not. And now it's still the same interface but with all allowed mutating methods now throwing unexpectedly. IMO that's a broken contract as well as a breaking change.

[steida]

@gcanti There is some serious performance issue with freeze. Check automerge/automerge#177

But maybe it's not an issue for fp-ts case.

[gkamperis]

@raveclassic I think you are taking my comparison the wrong way. I am comparing concepts not behaviour.

I get what you are saying. But the instigating issue is valid. Someone can get empty mutate it by mistake and then everywhere where empty is used suddenly is blown up.

So changing empty from an exported const to a function empty() that returns a brand new empty array every time will maintain the concept and avoid the freeze. That is why I mentioned that maybe it should become a function similar to rxjs as it returns an always empty observable no matter what the caller did to the previous empty.

RE the state management, I think I used the wrong words. I meant managing the immutability state. I meant that maybe fp-ts should not trouble itself on weather the data structures used with it are immutable or not as it is not really required.

[gcanti]

It should be restricted by its interface then, not by fragile and inconsistent convention

I agree but we can't, as I said above changing the type of empty is a breaking change.

This PR is just a way to prevent unwanted occasional mutations, but I'm fine to revert the change if you all disagree (moreover personally I've never experienced problems because I just don't mutate things and don't use unsafe libraries)

we should freeze everything, not only arrays, and always use Readonly* interfaces

If we switch to Readonly* interfaces there's no need for freezing (which is not an option, cc @steida)

Yes, DX is terrible but it follows the interface.

I'm mostly interested in this ^, that's my main concern.

Admittedly after the introduction of the readonly modifier the DX should be way better than the past.
Is still awkward working with Readonly* interfaces?
Maybe we should try them again, for example we could add the following modules

• ReadonlyArray
• ReadonlySet
• ReadonlyMap
• ReadonlyNonEmptyArray
• ReadonlyRecord
• ReadonlyTuple
• ReadonlyTupleT
• other?

and see how it goes. And if it ends up well we can remove the mutable counterparts in v3

[mlegenhausen]

+1 for the readonly modules.

If we can prevent programming errors at compile time we should go this direction or we run in the problem that the interface behavior does not match the runtime behavior.

[gkamperis]

is there any reason why empty is a const value?

if it is required to be a value then freeze is the right way to go.

if it is not required to be a value converting it to a function resolves the mutation/freeze issue.

Suppose everything works out, fast fwd to v3: prefixing everything with Readonly when everything is read-only is just boilerplate and superfluous in my opinion.

[raveclassic]

@gkamperis

changing empty from an exported const to a function empty() that returns a brand new empty array every time

Ahhh I see now, thanks. Yeah, that would fit however with a light performance penalty. Still a breaking change though.

I meant that maybe fp-ts should not trouble itself on weather the data structures used with it are immutable or not as it is not really required

Thanks once again for explanation. I'd say fp-ts should only trouble itself on maintaining correctness of its interfaces and not expose such fragile implementation details to the outer world.

@gcanti

Thanks for explanation.

This PR is just a way to prevent unwanted occasional mutations, but I'm fine to revert the change if you all disagree (moreover personally I've never experienced problems because I just don't mutate things and don't use unsafe libraries)

Well I tend to revert it anyway. We faced this issue when we're working with a legacy javascript library that mutated its config built using empty value. Seems like deep-copying that whole initial config should be enough. /cc @sutarmin

If we switch to Readonly* interfaces there's no need for freezing

Agree. I'd avoid any hidden js "optimizations" 😄

Is still awkward working with Readonly* interfaces?

This is related to #987.

Maybe we should try them again, for example we could add the following modules and see how it goes. And if it ends up well we can remove the mutable counterparts in v3

That's a great suggestion actually!

@mlegenhausen

If we can prevent programming errors at compile time we should go this direction or we run in the problem that the interface behavior does not match the runtime behavior.

Can't agree more!

[sutarmin]

OK, this one got controversial, let me add my 5 cents here. As I said in #979 I understand that this is a compromise and I considered ReadonlyArray as an alternative.

I really love the idea about the compile-time contract (the whole TS thing is about that, right?) and I'm looking forward to trying Readonly* implementations, but there is a problem right now. And it looks to me like nobody here has faced it yet, but I have. Believe me, the last thing that you would expect is that empty is not actually empty. So the whole compromise is about having either a tough time with debugging or immediate runtime exception in such cases. And the latter looks like a lesser evil. I know that evil is evil, lesser, greater..., but it won't even touch you if you don't deal with mutable libraries, but you can never be sure you don't. I think that empty is too fundamental to be silently mutated.

TL;DR I would consider moving from Object.freeze to Readonly* interfaces whenever it's possible rather than reverting this PR.

Seems like deep-copying that whole initial config should be enough.

It's not the only case where we had problems, so I would prefer a more universal solution.

[raveclassic]

@sutarmin You are talking about a problem in a javascript library. Nobody forbids an arbitrary 3rd party javascript code to mess everything everywhere including fp-ts itself, that's javascript in the end. Since fp-ts is a TypeScript-first library then IMO we should focus only on TypeScript parts. This means fp-ts should not bother about trying to fix javascript but to provide reliable and correct interfaces for TypeScript code.

It's not the only case where we had problems, so I would prefer a more universal solution.

Looks like a room for improvement in the end-user code, not in fp-ts core.

Summing up

I understand that this is a compromise

This is not a compromise actually, current implementation (before this change) fully satisfies the interface. You are trying to solve a problem which is out of scope of the lib because 1) if we want immutability, we can use freeze and change interface to ReadonlyArray 2) since we're typescript-first and use ReadonlyArray we don't actually need freeze.

[sutarmin]

You are talking about a problem in a javascript library

Nope, before this change, one could easily do it in any library, even written in Typescript.

Nobody forbids an arbitrary 3rd party javascript code to mess everything everywhere

Actually, freeze does 😄

Looks like a room for improvement in the end-user code, not in fp-ts core

Yes, this was considered in #979 as well, but it's not too reliable because it's easy to forget.

And again: I think empty is too fundamental for fp-ts to be occasionally mutated "just because it's javascript in the end".

[raveclassic]

Nope, before this change, one could easily do it in any library, even written in Typescript.

Then it's even more not the responsibility of fp-ts to track such cases.

Actually, freeze does

Breaking a valid contract with exceptions. That's not fp-ts way.

but it's not too reliable because it's easy to forget.

But it's still up to developer to think about it, not for fp-ts core. The primary goal is to provide reliable, consistent and correct interfaces and not to try to solve any particular "forgotten" problem on the developer side.

I think empty is too fundamental for fp-ts to be occasionally mutated "just because it's javascript in the end".

Should we also freeze Monoids? Alternatives? All available instances in the whole lib?
The problem is fundamental indeed but not for fp-ts. empty is no more "fundamental" than any other constant.

Anyway I still don't see any strong reasoning from your side which would make breaking the contract justified but some highly specific edge case without any details. Could you provide an example of such case where it's impossible to fix it on your side so that such change should be made to fp-ts core?

[sutarmin]

Could you provide an example of such case where it's impossible to fix it on your side

It's always possible, but the idea was to improve overall DX in this case, to provide developer information right away that something unexpected just happened.

Should we also freeze Monoids? Alternatives? All available instances in the whole lib?

It's not quite the same, much more difficult to occasionally mutate such instances.

The primary goal is to provide reliable, consistent and correct interfaces

Agreed. The question is how reliable is the interface called empty that could not be an actual empty array? All I'm saying is that if you hit such a situation, a runtime exception is your least problem.

Of course, explicit ReadonlyArray is better, let's move that way, but without removing freeze (to be still safe in cases of interoperability with js libraries)

[raveclassic]

It's always possible, but the idea was to improve overall DX in this case, to provide developer information right away that something unexpected just happened.

So no example?

It's not quite the same, much more difficult to occasionally mutate such instances.

It is. There's no "occasionally" or "intentionally". Either you can break things or not. Either consistently or not.

The question is how reliable is the interface called empty that could not be an actual empty array

That's not the interface but an implementation detail. The interface says I am able to push to empty and that's it.

Of course, explicit ReadonlyArray is better, let's move that way, but without removing freeze (to be still safe in cases of interoperability with js libraries)

When we can define such const empty: readonly never[] = Object.freeze([]) without casts and not break the entire ecosystem then we should add it to the core. At the moment it's a partial solution and a broken contract. If you ever need an immutable empty array in your code just create a constant: const immutableEmpty: readonly never[] = Object.freeze(array.empty)
Please let's keep focused on this and not delve into meaningless debate.

[sutarmin]

There's no "occasionally" or "intentionally".

I beg to differ, but I'm stepping back at the moment, all my arguments are already in the thread.

[gcanti]

The interface says I am able to push to empty and that's it

@raveclassic just an observation: I'm ok to revert this change (will send a PR), especially if we add Readonly* modules which hopefully will make Array and NonEmptyArray obsolete, but let's be honest and a bit more flexible: not everything can be encoded in types (I wish...) and often the context is important too.

Consider Semigroup for example, the way is implemented (in TypeScript, Haskell, PureScript, etc...) is just ridiculous from a math point of view:

export interface Semigroup<A> {
  readonly concat: (x: A, y: A) => A
}

^ this says nothing about the most important thing: associativity.

[gcanti]

• revert 1082 revert #1082 #1085
• readonly modules (draft) add readonly modules #1086