sanctuary-js / sanctuary-def

Run-time type system for JavaScript
MIT License
294 stars 23 forks source link

Set, WeakSet, Map and WeakMap should be in the default env #183

Closed mfidemraizer closed 4 years ago

mfidemraizer commented 6 years ago

I believe that these built-in collection types should be included in the default env

davidchambers commented 6 years ago

What about WeakSet, @mfidemraizer?

mfidemraizer commented 6 years ago

@davidchambers all of them ;-)

davidchambers commented 6 years ago

@mfidemraizer, could you provide implementations of the following functions?

//  isJsSet :: Any -> Boolean
var isJsSet = function(x) { ... };

//  jsSetValues :: JsSet a -> Array a
var jsSetValues = function(jsSet) { ... };
//  isJsMap :: Any -> Boolean
var isJsMap = function(x) { ... };

//  jsMapKeys :: JsMap k v -> Array k
var jsMapKeys = function(jsMap) { ... };

//  jsMapValues :: JsMap k v -> Array v
var jsMapValues = function(jsMap) { ... };
mfidemraizer commented 6 years ago

Wouldn't look them as follows?

//  isJsSet :: Any -> Boolean
var isJsSet = function(x) { return Set.prototype.isPrototypeOf(x) };

//  jsSetValues :: JsSet a -> Array a
var jsSetValues = function(jsSet) { return [...jsSet.values()] };
//  isJsMap :: Any -> Boolean
var isJsMap = function(x) { return Map.prototype.isPrototypeOf(x) };

//  jsMapKeys :: JsMap k v -> Array k
var jsMapKeys = function(jsMap) { return [...jsMap.keys()] };

//  jsMapValues :: JsMap k v -> Array v
var jsMapValues = function(jsMap) { return [...jsMap.values()] };

There's an issue here:

It seems like we would need to place some notice regarding the usage of these collection types: you need native ES2015+ support.

davidchambers commented 6 years ago

It seems like we would need to place some notice regarding the usage of these collection types: you need native ES2015+ support.

The problem cannot be solved with documentation. If we use ES6 syntax (... or of) in the source file, the library will no longer work at all in older environments (due to parsing errors).

Perhaps these types should be defined in a separate module (e.g. require('sanctuary-def/es6')).

mfidemraizer commented 6 years ago

I thought I already sent some message (it seems like I missed to send it and now it's lost).

You're right. In my case, I would call that module as sanctuary-def/esnext as ES is a living standard now.

davidchambers commented 6 years ago

I would call that module as sanctuary-def/esnext as ES is a living standard now.

:thumbsup:

Avaq commented 6 years ago

The problem cannot be solved with documentation. If we use ES6 syntax (... or of) in the source file

All of this syntax is just sugar over specific method calls, so you can just implement it without sugar and have it throw in older environments only when called.

//  isJsSet :: Any -> Boolean
var isJsSet = function(x) {
  return Object.prototype.toString.call(x) === '[object Set]';
};

//  jsSetValues :: JsSet a -> Array a
var jsSetValues = function(jsSet) {
  return Array.from(jsSet.values());
};
//  isJsMap :: Any -> Boolean
var isJsMap = function(x) { 
  return Object.prototype.toString.call(x) === '[object Map]';
};

//  jsMapKeys :: JsMap k v -> Array k
var jsMapKeys = function(jsMap) { 
  return Array.from(jsMap.keys());
};

//  jsMapValues :: JsMap k v -> Array v
var jsMapValues = function(jsMap) {
  return Array.from(jsMap.values());
};
davidchambers commented 6 years ago

Thank you, Aldwin. This is good news!

We can define $.JsSet :: Type -> Type and $.JsMap :: Type -> Type -> Type (along with their “weak” equivalents). The question then becomes whether it is safe to include $.JsSet ($.Unknown) and $.JsMap ($.Unknown) ($.Unknown) in $.env. I think it is safe to do so, because an extractor function (such as jsSetValues) will only be applied if and when the corresponding predicate returns true, which should only happen in environments which support iterators. Is this reasoning sound?

gabejohnson commented 6 years ago

While we're at it, should we add an Iterable or Iterator typeclass as well?

davidchambers commented 6 years ago

Good idea, Gabe. :)

Avaq commented 6 years ago

Is this [the] reasoning sound?

I can confirm that this is the sound of reasoning.

(just kidding, yes it's sound)

mfidemraizer commented 6 years ago

@Avaq There's something that it's not just syntactic sugar:

var isJsSet = function(x) {
  return Object.prototype.toString.call(x) === '[object Set]';
}

const obj = {
    get [Symbol.toStringTag]() {
    return 'Set'
  }
}

isJsSet ( obj )  // true

It would be strange that someone would override an object string tag with Set, but anyway, it seems unsafe to rely on strings to determine if a given value is of some prototype.

What about this?

var isJsSet = function(x) { return Set ? Set.prototype.isPrototypeOf(x) : false };
davidchambers commented 6 years ago

There's a problem with your suggestion, @mfidemraizer:

> var vm = require('vm')

> Set.prototype.isPrototypeOf(vm.runInNewContext('new Set([1, 2, 3])'))
false

it seems unsafe to rely on strings to determine if a given value is of some prototype

As explained in sanctuary-js/sanctuary#100, Object.prototype.toString is the most reliable approach.

mfidemraizer commented 6 years ago

@davidchambers it's a circular argument, because I've demonstrated how you may impersonate what you expect.

I understand that regular prototype checking couldn't work across sandbox boundaries. BTW I wouldn't be able to argue that testing type string representations would be the most reliable approach because the common denominator is manual sandboxing is niche instead of the mainstream use case.

Indeed I'm not arguing that niche use cases should be ditched. But I still think that compromising idiomatic prototype checking because of niche use cases - at least for me - isn't a good design decision.

In fact, there's a way to get true on your proposed case (based on this issue from 2011):

// sets true
const result = Set.prototype.isPrototypeOf( 
        vm.runInNewContext( 
            "new Set( [ 1, 2, 3 ] )", 
            { Set }
        ) 
    )

And for me sounds absolutely logical: if you're willing to verify that a certain Set object inside an vm context is of parent Set's prototype is because you're sharing the same global object.

Probably there're more complex scenarios where you would need to inject too many globals to get the expected behavior....

Thus, I still conclude that relying on strings is a bad idea and my previous comment demonstrated why.

ivenmarquardt commented 6 years ago

Thus, I still conclude that relying on strings is a bad idea

@mfidemraizer Just to give you another perspective: Haskell's ADTs are essentially tagged unions where the tags don't belong to the type system but are values and hence aren't erased after compilation so that you can pattern match on them. I don't know in detail how tags are implemented but I guess pretty much like Strings.

Apart from that, relying on object identity may be also considered harmful, because it invalidates referential transparency.

davidchambers commented 6 years ago

I agree that Node's vm module is only required in a few specific circumstances, @mfidemraizer. I use vm.runInNewContext in examples not because I consider this particular function important, but because it provides a convenient means of demonstrating the broader issue. Dealing with multiple frames in a browser context is something many front-end developers find themselves doing at some point. The limitation applies here too, though it takes a bit more code to demonstrate:

var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
var script = iframe.contentWindow.document.createElement('script');
script.textContent = 'window.myArray = [1, 2, 3]';
iframe.contentWindow.document.body.appendChild(script);
var myArray = window.frames[window.frames.length - 1].myArray;
myArray instanceof Array;  // => false
Object.prototype.toString.call(myArray) === '[object Array]';  // => true
Array.isArray(myArray);  // => true

If and when we release a version of sanctuary-def which provides $.JsSet and $.JsMap, you're free to define and use your own type constructors instead. Perhaps you're working in a context in which you can be sure there will only ever be one environment, so you'd prefer to use type constructors which perform instanceof checks and thus won't be fooled by sneaky uses of Symbol.toStringTag. ;)

mfidemraizer commented 6 years ago

@ivenmarquardt About string, I was arguing that relying on toStringTag output is a bad idea. I should have been more clear on this, sorry.

mfidemraizer commented 6 years ago

@davidchambers The frontend example seems more meaningful (because there's no apparent workaround like when creating vm contextes).

Anyway, isn't sharing state across process boundaries a bad idea?

See how web workers solve that scenario: serialization.

const myArray = JSON.parse(JSON.stringify(window.frames[window.frames.length - 1].myArray))

...and then:

Array.prototype.isPrototypeOf(myArray) // => true 

If I'm not mistaken, every most JS values can be serialized. These corner cases can be easily solved with serialization/deserialization. It's a small overhead to cover a niche case.

Anyway, it's clear we own different points of views about how to solve some issues.

davidchambers commented 6 years ago

If I'm not mistaken, every JS value can be serialized.

Out of interest, how would one serialize new Date (0)? How about S.Just (0)?

mfidemraizer commented 6 years ago

@davidchambers

And about S.Just, I'm not sure if it's fine to serialize monads as they're computational results, aren't they? Would you span a computation across process boundaries?

It seems that I would've to produce some kind of DTO and continue the computation from a raw value. I wouldn't rely on serializing monads because I can easily find a case where serialization isn't possible: futures (unless you fork them, of course).

mfidemraizer commented 6 years ago

@davidchambers I would add that I made a wrong statement: not all values are serializable. For example, functions can't be serialized.

davidchambers commented 6 years ago

not all values are serializable

We're in agreement. :)

Avaq commented 6 years ago

@mfidemraizer Symbol.toStringTag is a plague. Just like Proxy, and other behaviour-altering features that have been, and are being, added to JavaScript. As time progresses, JavaScript becomes less and less reliable. Sanctuary makes a best effort to identify what you give it, but a user will always be able to forge a value that impersonates some other value.

ivenmarquardt commented 6 years ago

Symbol.toStringTag is a plague.

@Avaq

Can you elaborate on this? Are you referring to interoperability?

Btw, I use Proxys to transform normal into homogeneous Arrays without having to transform the actual data structure, which would be pretty expensive. I consider them quite useful for meta-programming like this.

EDIT: I guess I got it. You mean that people can use these tools in a wrong manner. Yes, that's true. However, it isn't the responsibility of a lib to anticipate all these errors. If you make a mistake you have to deal with the consequences.

Avaq commented 6 years ago

However, it isn't the responsibility of a lib to anticipate all these errors. If you make a mistake you have to deal with the consequences

That's the point I was trying to make, but sprinkled with some of my personal pains with regards to these new features. :P