Change type validators to use predicate instead of TypeRep #191

Avaq · 2016-04-23T13:24:32Z

Commit description:

This commit will close #149 by changing all functions
that have built-in type validation to use predicates
instead of TypeReps to perform their validations.

The functions affected are:

get()

gets()

parseJson()

The only function untouched is is(), for it is used
to turn a TypeRep into a predicate.

Avaq · 2016-04-23T13:29:44Z

index.js

@@ -2341,25 +2341,24 @@
  //. ```
  S.lastIndexOf = sanctifyIndexOf('lastIndexOf');

-  //# pluck :: Accessible a => TypeRep b -> String -> [a] -> [Maybe b]
+  //# pluck :: Accessible a => (b -> Boolean) -> String -> [a] -> [Maybe c]


I'm not super happy with the type signatures. Both b and c are "unique type variables", so they may as well be Any. I wish there was some way to indicate inside the type signature that c is the set of values that pass the predicate. Perhaps a Predicate a pseudotype?

Perhaps a Predicate a pseudotype?

I'm not sure how this would work. Could you provide the type signature you have in mind?

The idea would be that Predicate a somehow constrains a to be the set of values for which the predicate function of type Predicate a returned true.. It doesn't quite work like that I think, but I wish it could.

Avaq · 2016-04-23T13:48:09Z

A major concern I have about this change is the ease of upgrading. Since many TypeReps are actually also Functions, the type checker will not complain. Take the following example:

S.get(Array, 'x', {x: 1})

Before this commit, this would return Nothing(), which is what the developer intended to have happen. After this commit, this will in fact return Just(1), because Array is being used as a predicate to filter and Boolean(Array(1)) === true.

A half-solution would be to change filter to be more strict about the output of predicates:

  //  filter :: (Monad m, Monoid m) => ((a -> Boolean), m a) -> m a
  var filter = function(pred, m) {
    return m.chain(function(x) {
-      return pred(x) ? m.of(x) : m.empty();
+      return pred(x) === true ? m.of(x) : m.empty();
    });
  };

But this would just make the function always return Nothing() when given an "invalid predicate" like that. Still quite difficult for a developer to figure out what's going wrong (especially since it will probably happen in lots of places; get and gets are some of my most used Sanctuary functions).

It would be somewhat better if we asserted the output of the predicate to be a Boolean, at least. How is your function checking feature coming along? :)

One idea is to use the Predicate a pseudotype I mentioned earlier which asserts that:

The function is unary
The function returns a Boolean when called with anything

davidchambers · 2016-04-30T18:23:45Z

@Avaq, could you point to the dc-higher-order-functions branch of sanctuary-def and try the $._UnaryFunction type constructor? It doesn't yet support type variables, but you could use $._UnaryFunction($.Any, $.Boolean) for now. :)

Avaq · 2016-05-20T16:20:23Z

I rebased this on sanctuary#master, but sanctuary-def#dc-higher-order-functions is still at 0.5, whereas master wants 0.6.x. Would it be a good idea to rebase sanctuary-def#dc-higher-order-functions as well? Otherwise I feel like I'm working in the past. ^^

davidchambers · 2016-05-20T16:53:03Z

I just rebased that branch for you. :)

Avaq · 2016-05-20T17:31:50Z

index.js

@@ -279,6 +279,9 @@
  var l = $.TypeVariable('l');
  var r = $.TypeVariable('r');

+  //  Predicate :: Type
+  var Predicate = $._UnaryFunction($.Any, $.Boolean);


Not sure where to put this, nor how to signature it, but I like it being separated because;

It shortens the type-signatures it's used in to below 79 characters.

Less repitition.

Sounds good to me, since in each case the predicate must accept any value. For a function such as filter, though, we'd want a -> Boolean rather than Any -> Boolean.

Avaq · 2016-05-24T10:07:21Z

Seen that Error: Cannot find module 'to-iso-string' before?

davidchambers · 2016-05-26T23:42:22Z

Seen that Error: Cannot find module 'to-iso-string' before?

No. That's new to me.

davidchambers · 2016-05-27T00:33:02Z

It was an intermittent (network?) error, apparently. Tests are now passing (after rebuilding). :)

Avaq · 2016-12-24T17:37:43Z

I'm in the process of rebasing this because it's on the to-do list for the next release. But it may take a while; a lot has changed.

davidchambers · 2016-12-24T17:44:49Z

That's exciting news, @Avaq! Thank you. :)

Avaq · 2016-12-25T11:36:14Z

I've made the rebase. Several things have changed:

pluck Is no longer affected.
I dropped the changes which depended on a specific commit hash of sanctuary-def. I think it's better to merge this first and change it once sanctuary-def is updated. Otherwise we keep running in circles and stuff will never be merged.

Avaq · 2016-12-25T11:45:51Z

Doc tests didn't run in 7.0. Fixing docs.

Avaq · 2016-12-25T11:54:19Z

I think it's better to merge this first

Allow me to extend upon this statement: The reason we didn't merge just came back to me. It was this concern I had about S.get(Array, 'x', {x: 1}) returning Just(1), making upgrading a pain.

Perhaps we should hold off until #232 is merged. Or; knowing that it will be merged before the next release, it should be safe to merge this and upgrade the $.Functions to $.UnaryFunctions afterwards.

Avaq · 2016-12-26T09:28:39Z

test/get.js

-  eq(S.get(String, 'x', {x: 0, y: 42}), S.Nothing);
+  eq(S.get(S.K(true), 'x', {x: 0, y: 42}), S.Just(0));
+  eq(S.get(S.K(true), 'y', {x: 0, y: 42}), S.Just(42));
+  eq(S.get(S.K(true), 'z', {x: 0, y: 42}), S.Just(undefined));


This behavior was discussed with @davidchambers on Gitter, and he noted that he would prefer this to return Nothing (by determining beforehand whether the object property even exists). This has several implications:

The behavior will change from how it currently works. Currently; if we were to provide a TypeRep Nil, we would correctly get Just(undefined). The only reason we don't come across this is because TypeRep Nil does not exist.

We will have to make a decision on what constitutes existence of an object property, eg. do we walk the prototype chain? Currently we delegate all of the decision making to the type identifier.

I'm concerned that behavior might be unexpected. If I provide a type identifier which includes undefined as one of its members (eg. Nil | String) then it would behave the same as the type excluding undefined (eg. String) for JSON parsed data. My get would not resolve to Just(undefined), despite undefined being an expected type.

What are your thoughts?

What about this?

function get(pred, key, obj) { var x = obj[key]; return key in obj && pred(x) ? Just(x) : Nothing; }

I draw a distinction between {x: undefined} and {}. In the Sanctuary world we should never think the result of ({}).x is undefined. Instead, we should think the result of indiscriminately getting the value of the x property of {} is Nothing.

Understood. Your reasoning makes sense, yet my concern still stands. I will make the change, and I'll have to see how it plays out in practice once it's released. It's a minor concern anyway. :)

Avaq · 2016-12-27T08:51:30Z

index.js

    var maybe = keys.reduce(function(m, k) {
-      return m.chain(function(x) { return x == null ? Nothing : Just(x[k]); });
+      return m.chain(function(x) {
+        return x != null && k in x ? Just(x[k]) : Nothing;


Decided to negate the null check rather than negating the k in x check for aesthetic reasons. Means that the false case in now on the right hand side of the ternary.

davidchambers · 2016-12-27T09:04:26Z

test/get.js

 var S = require('..');

+var vm = require('vm');


Revert this change, please. :)

⚡ (merge error I missed)

davidchambers · 2016-12-27T09:45:26Z

test/gets.js


-  eq(S.gets(RegExp, ['x'], {x: vm.runInNewContext('/.*/')}), S.Just(/.*/));
-  eq(S.gets(vm.runInNewContext('RegExp'), ['x'], {x: /.*/}), S.Just(/.*/));
+  eq(S.gets(S.K(true), ['x'], {x: vm.runInNewContext('/.*/')}), S.Just(/.*/));


Could we use S.is(RegExp) here rather than S.K(true), and reinstate the second assertion?

I figured S would be undefined in a new context. But I haven't asserted that. Let me check. :)

Indeed; ReferenceError: S is not defined.

This is what I had in mind:

- eq(S.gets(RegExp, ['x'], {x: vm.runInNewContext('/.*/')}), S.Just(/.*/)); - eq(S.gets(vm.runInNewContext('RegExp'), ['x'], {x: /.*/}), S.Just(/.*/)); + eq(S.gets(S.is(RegExp), ['x'], {x: vm.runInNewContext('/.*/')}), S.Just(/.*/)); + eq(S.gets(S.is(vm.runInNewContext('RegExp')), ['x'], {x: /.*/}), S.Just(/.*/));

This way we'd continue to assert that a regexp from one environment is a member of the type represented by RegExp from another environment. Admittedly this is really testing is rather than gets, but that's okay.

Ahhh. Right. ⚡

davidchambers · 2016-12-27T09:46:51Z

test/gets.js


-  throws(function() { S.gets(Number, null); },
+  throws(function() { S.gets(S.K(true), null); },


Let's use S.is(Number) here (and below).

Same for the get tests? Should I change all S.K(Bool) functions to appropriate S.is(TypeRep) functions?

Yes, please. It's nice for the diff to act as an upgrade guide as much as possible. :)

davidchambers · 2016-12-27T09:48:09Z

index.js

-    return filter(function(x) { return is(typeRep, x); },
-                  encase(JSON.parse, s));
+  function parseJson(pred, s) {
+    return filter(function(x) { return pred(x); }, encase(JSON.parse, s));


function(x) { return pred(x); } is better written pred. 😜

Very good point. I'll change it in both places.

davidchambers · 2016-12-27T09:52:07Z

index.js

    var x = obj[key];
-    return is(typeRep, x) ? Just(x) : Nothing;
+    return key in obj && pred(x) ? Just(x) : Nothing;


We shouldn't attempt obj[key] before the key in obj check. Let's replace the two lines above with the following:

return key in obj && pred(obj[key]) ? Just(obj[key]) : Nothing;

if we want to optimise property access and repetition we can do this:

var x = null return key in obj && (x = obj[key], pred(x) ? Just(x) : Nothing)

I suppose it's cleaner that way. Though perhaps it would then be best to wrap the whole statement in an if. When implementing Promise/A+ I had to access the .then property once and only once, because property access in JavaScript is impure due to custom getters.

EDIT: Or the suggestion by Irakli over an if statement.

I don't mind too much. I'll leave it to you, Aldwin. If you go with Irakli's approach, we could avoid using the comma operator:

var x; return key in obj && pred(x = obj[key]) ? Just(x) : Nothing;

This approach seems okay too:

if (key in obj) { var x = obj[key]; if (pred(x)) return Just(x); } return Nothing;

@safareli Your statement returns false rather than Nothing for non-existent properties.

@Avaq you are right.

// this key in obj && (x = obj[key], true) && pred(x) ? Just(x) : Nothing // and this will work key in obj && (x = obj[key], 1) && pred(x) ? Just(x) : Nothing

but it's a bit cryptic

Scroll up for a more elegant solution, @safareli. ;)

Avaq · 2016-12-27T10:21:11Z

test/parseJson.js

-  eq(S.parseJson(Array, '["foo","bar"]'), S.Just(['foo', 'bar']));
+  eq(S.parseJson(S.K(true), '[Invalid JSON]'), S.Nothing);
+  eq(S.parseJson(S.K(false), '{"foo":"bar"}'), S.Nothing);
+  eq(S.parseJson(S.K(true), '["foo","bar"]'), S.Just(['foo', 'bar']));


Fixing these.

davidchambers · 2016-12-27T10:30:35Z

test/parseJson.js

-  eq(S.parseJson(Array, '["foo","bar"]'), S.Just(['foo', 'bar']));
+  eq(S.parseJson(S.is(Object), '[Invalid JSON]'), S.Nothing);
+  eq(S.parseJson(S.is(Array), '{"foo":"bar"}'), S.Nothing);
+  eq(S.parseJson(S.is(Array), '["foo","bar"]'), S.Just(['foo', 'bar']));


I'd love to replace the two assertions above with the following:

eq(S.parseJson($.test([], $.Array($.String)), '{"foo":"bar"}'), S.Nothing); eq(S.parseJson($.test([], $.Array($.String)), '[1,2,3,4,5,6]'), S.Nothing); eq(S.parseJson($.test([], $.Array($.String)), '["foo","bar"]'), S.Just(['foo', 'bar']));

Very aeson-like!

This requires a recent version of sanctuary-def, though. I'm currently working on the pull request to upgrade our sanctuary-def dependency. :)

davidchambers · 2016-12-27T10:33:54Z

index.js

-    var x = obj[key];
-    return is(typeRep, x) ? Just(x) : Nothing;
+  function get(pred, key, obj) {
+    var x = null;


Thoughts on var x = null; versus var x; here?

Let's make it consistent with similar code.

I realized there is no similar case in the entire code base.

davidchambers · 2016-12-27T10:40:04Z

index.js

-  //. name, and an object and returns Just the value of the specified object
-  //. property if it is of the specified type (according to [`is`](#is));
+  //. Takes a predicate, a property name, and an object and returns Just the
+  //. value of the specified object property if it passes the given predicate;
  //. Nothing otherwise.


We should state that Nothing is returned if the property does not exist:

Takes a predicate, a property name, and an object and returns Just the value of the specified object property if ~~it passes~~ the property exists and the value satisfies the given predicate; Nothing otherwise.

davidchambers · 2016-12-27T10:41:32Z

index.js

-  //. the value is of the specified type; Nothing otherwise.
+  //. Takes a predicate, an array of property names, and an object and returns
+  //. Just the value at the path specified by the array of property names if
+  //. such a path exists and the value passes the given predicate; Nothing


I'd prefer "satisfies" in place of "passes".

Avaq · 2016-12-27T10:51:13Z

index.js

-  //. The `Object` type representative may be used as a catch-all since most
-  //. values have `Object.prototype` in their prototype chains.
+  //. Takes a predicate, a property name, and an object and returns Just the
+  //. value of the specified object property if it exists and the value


Went for "it" over your suggested "the property".

This seems slightly ambiguous to me. I see two valid interpretations:

return Just (the value of the specified object property) if it exists; or

return Just the value of (the specified object property) if it exists.

"It" can refer to either parenthesized term, depending on where one's brain decides the parens should go. For some reason my brain wants to include "the value of" in the parens.

Hm, I found that it read more fluently. Referring to the existence of the subject clears the ambiguity to me - it doesn't matter whether we talk about the existence of the property or it's value. Non-existent applies to the whole pair. ;)
I guess you should decide in the end. What will it be?

I find your argument persuasive. Let's stick with "it". 😀

Avaq · 2016-12-28T09:49:14Z

So what do you think? Merge this before or after #232?

davidchambers · 2016-12-28T10:41:49Z

Go ahead and squash, rebase on master, and update this pull request. Once the tests pass I will merge it. :)

This commit will close #149 by changing all functions that have built-in type validation to use predicates instead of TypeReps to perform their validations. The functions affected are: * get() * gets() * parseJson() The only function untouched is `is()`, for it is used to turn a TypeRep into a predicate.

Avaq · 2016-12-28T11:29:55Z

⚡

Avaq · 2016-12-28T11:40:58Z

IE 11.0.0 (Windows 8.1 0.0.0)  gets FAILED
	Error: Can't execute code from a freed script
	   at _toString (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:23393:17)
	   at toString (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:23720:9)
	   at f1 (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:16255:17)
	   at Maybe$prototype$toString (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:977:5)
	   at Anonymous function (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:26072:11)
	   at Anonymous function (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:25352:11)
	   at Anonymous function (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:453:18)
	   at Anonymous function (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:26072:11)
	   at Anonymous function (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:25348:11)
	   at _toString (/tmp/6bd55503b2760cbee13c9bdb8672c6c7.browserify:23393:17)

Avaq · 2016-12-28T11:42:00Z

Seems like a test-runner related error. Has to do with running code in child windows like iframes. Shall we retry?

Shard · 2016-12-29T02:07:35Z

I re-ran the tests and they worked 😕

davidchambers · 2016-12-29T04:00:10Z

I've hit that error once too. It seems to be nondeterministic.

davidchambers · 2016-12-29T04:03:46Z

🌳

Avaq reviewed Apr 23, 2016
View reviewed changes

Avaq force-pushed the av-predicate-type-validation branch from 1331577 to c30cbbd Compare April 23, 2016 13:33

davidchambers mentioned this pull request May 6, 2016

Check signature of function arguments sanctuary-js/sanctuary-def#63

Closed

Avaq force-pushed the av-predicate-type-validation branch from c30cbbd to 46ba190 Compare May 20, 2016 16:20

Avaq reviewed May 20, 2016
View reviewed changes

davidchambers mentioned this pull request Jun 13, 2016

sanctuary-def@0.8.0 #232

Merged

davidchambers mentioned this pull request Aug 29, 2016

encase return Left/Nothing when null / undefined / NaN #241

Closed

davidchambers mentioned this pull request Sep 14, 2016

v0.12.0 checklist #255

Closed

33 tasks

Avaq force-pushed the av-predicate-type-validation branch from b591bb2 to 938f8e2 Compare December 25, 2016 11:32

Avaq force-pushed the av-predicate-type-validation branch from 938f8e2 to 9a90a49 Compare December 25, 2016 11:38

Avaq force-pushed the av-predicate-type-validation branch 3 times, most recently from 3a95cc0 to e3ea415 Compare December 25, 2016 11:50

Avaq commented Dec 26, 2016

View reviewed changes

Avaq commented Dec 27, 2016

View reviewed changes

davidchambers reviewed Dec 27, 2016

View reviewed changes

Avaq commented Dec 27, 2016

View reviewed changes

davidchambers reviewed Dec 27, 2016

View reviewed changes

Avaq commented Dec 27, 2016

View reviewed changes

Avaq force-pushed the av-predicate-type-validation branch from 18eb8af to ddc5f6e Compare December 28, 2016 11:28

davidchambers merged commit 01f053e into master Dec 29, 2016

davidchambers deleted the av-predicate-type-validation branch December 29, 2016 04:03

davidchambers mentioned this pull request Feb 13, 2017

v0.12.0 release notes #332

Closed


		throws(function() { S.gets(Number, null); },
		throws(function() { S.gets(S.K(true), null); },

Change type validators to use predicate instead of TypeRep #191

Change type validators to use predicate instead of TypeRep #191

Conversation

Avaq commented Apr 23, 2016 • edited Loading

Avaq Apr 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq commented Apr 23, 2016 • edited Loading

davidchambers commented Apr 30, 2016

Avaq commented May 20, 2016

davidchambers commented May 20, 2016

Avaq May 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq commented May 24, 2016

davidchambers commented May 26, 2016

davidchambers commented May 27, 2016

Avaq commented Dec 24, 2016 • edited Loading

davidchambers commented Dec 24, 2016

Avaq commented Dec 25, 2016 • edited Loading

Avaq commented Dec 25, 2016

Avaq commented Dec 25, 2016 • edited Loading

Avaq Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

safareli Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safareli Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Avaq Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safareli Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq commented Dec 28, 2016

davidchambers commented Dec 28, 2016

Avaq commented Dec 28, 2016

Avaq commented Dec 28, 2016

Avaq commented Dec 28, 2016 • edited Loading

Shard commented Dec 29, 2016

davidchambers commented Dec 29, 2016

davidchambers commented Dec 29, 2016

Avaq commented Apr 23, 2016 •

edited

Loading

Avaq Apr 23, 2016 •

edited

Loading

Avaq commented Apr 23, 2016 •

edited

Loading

Avaq May 20, 2016 •

edited

Loading

Avaq commented Dec 24, 2016 •

edited

Loading

Avaq commented Dec 25, 2016 •

edited

Loading

Avaq commented Dec 25, 2016 •

edited

Loading

Avaq Dec 26, 2016 •

edited

Loading

safareli Dec 26, 2016 •

edited

Loading

Avaq Dec 27, 2016 •

edited

Loading

Avaq Dec 27, 2016 •

edited

Loading

safareli Dec 27, 2016 •

edited

Loading

Avaq Dec 27, 2016 •

edited

Loading

safareli Dec 27, 2016 •

edited

Loading

Avaq Dec 27, 2016 •

edited

Loading

Avaq commented Dec 28, 2016 •

edited

Loading