Repository: plaid/sanctuary
Pull request #161 (2016-03-04 22:24:31)

Add prop function
As discussed in #159. I've a few reservations about this, firstly if I define `S.prop` as `var prop = S.prop = def(...` then I get lots of errors in the tests when I replace instances of `R.prop` in `index.js`:

>TypeError: prop is not a function
    at createSanctuary (/home/stefano/git/sanctuary/index.js:9:16296)
    at /home/stefano/git/sanctuary/index.js:9:45767
    at _ (/home/stefano/git/sanctuary/index.js:9:268)
    at Object.<anonymous> (/home/stefano/git/sanctuary/index.js:9:779)

Another thing is that `Accessible` is a very permissive type for this function, it allows you to pass numbers or booleans in even though they don't support property access so you end up getting error messages that leak implementation details to the user:

```js
S.prop('a', true)
// => TypeError: Cannot use 'in' operator to search for 'a' in true
```

Also, I think it's strange that a function that operates on `Records` should consider keys that are present in the prototype chain. What are people's thoughts?

davidchambers: > I think it's strange that a function that operates on `Records` should consider keys that are present in the prototype chain.

It's not clear to me which is the most appropriate membership check.

I can, though, provide what I believe is a compelling example in favour of `key in obj`:

```javascript
//  Pair :: (a, b) -> Pair a b
function Pair(fst, snd) {
  this[0] = fst;
  this[1] = snd;
}

Pair.prototype.length = 2;
```

Should `S.prop('length', new Pair(0, 0))` evaluate to `2`? I argue that it should. Otherwise we'd be drawing a distinction between two equivalent implementations (`length` as a prototype property or as an instance property).

Also, the usual traps with inherited properties affect object-as-dictionary rather than object-as-record. When using an object to keep track of claimed usernames, `name in usernames` would erroneously evaluate `true` for `name` values such as `'toString'` and `'valueOf'`. When using an object as a record, though, one asks for a known property. Presumably if one writes `S.prop('toString', obj)` one *wants* to reference the `toString` method of `obj` (regardless of whether it's attached to `obj` directly).

kedashoe: So kind of a more type-dangerous [get](https://github.com/plaid/sanctuary#get)? Might be nice to mention `get` in the description (and this in the description for `get`.. do we have `@see`s?), how they're different, when you might use one vs the other.

Should we include `props` in this PR?


davidchambers: `prop` and `get` are certainly related, but should be used in different contexts. `get` and `gets` perform a role similar to that of Haskell's Aeson library: providing a safe way to interact with uncertain data.

We don't use `@see` (as I detest JSDoc), but we should certainly include reciprocal links and a sentence or two describing the differences.

kedashoe: > prop and get are certainly related, but should be used in different contexts. get and gets perform a role similar to that of Haskell's Aeson library: providing a safe way to interact with uncertain data.

Ya, I was thinking something along those lines.

> as I detest JSDoc

ha!
svozza: You make a convincing case for `key in obj`, David. Regarding my second point, do you think we should tighten up the definition of `Accessible` to something like this:

```js
function(x) { return x != null && R.contains(_type(x), ['Array', 'Function', 'Object', 'String']); }
```

I still have no idea why I can't use `S.prop` in the file before it's defined even though I declare it as a var (I assumed hoisting would take care of that). It would be nice for use to eat our own dogfood rather than relying on `R.prop`.
davidchambers: > [D]o you think we should tighten up the definition of `Accessible`[?]

What's the motivation for doing so?

> I still have no idea why I can't use `S.prop` in the file before it's defined even though I declare it as a var (I assumed hoisting would take care of that).

There are two different concepts in JavaScript referred to as "hoisting": [`var` hoisting][1], and [function declaration hoisting][2].

When one writes…

```javascript
(function() {
  console.log(x);
  var x = 42;
}());
```

`var` hoisting means one has effectively written…

```javascript
(function() {
  var x;
  console.log(x);
  x = 42;
}());
```

This will log `undefined` rather than throw a `ReferenceError` as one would expect without `var` hoisting. Note, though, that it does not log `42`.

> It would be nice for use to eat our own dogfood rather than relying on `R.prop`.

We could do so by defining `prop` near the top of the file…

```javascript
var prop =
def(…);
```

then assigning `prop` to `S.prop` at the appropriate point in the file.


[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var#var_hoisting
[2]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function#Function_declaration_hoisting

svozza: > What's the motivation for doing so?

Because `R.prop` as it is currently defined will not throw the correct type error if we pass a primitive value in and instead we'll get something like `TypeError: Cannot use 'in' operator to search for 'a' in true`. If `Accessible` is supposed to represent types that allow property access then why does it allow primitive types that don't?
davidchambers: Thanks for the explanation, Stefano! I now understand the motivation. :)

> If `Accessible` is supposed to represent types that allow property access then why does it allow primitive types that don't?

Primitive values other than `null` and `undefined` support property access:

```javascript
> true.a
undefined

> 'foo'.toUpperCase()
'FOO'
```

So Accessible isn't quite right, in this case. We could remove the constraint and determine whether the second argument is valid in the function body. This would enable consistent error messages:

```javascript
S.prop('x', null);
// ! No ‘x’ property in null

S.prop('x', {});
// ! No ‘x’ property in {}
```

svozza: > Primitive values other than null and undefined support property access:

Ah yes, through the magic of autoboxing. Although something like `1.toString()` is a syntax error so you have to do `1['toString']()`

> We could remove the constraint and determine whether the second argument is valid in the function body. 

Sounds good, I should be able to work on this later on today.
davidchambers: > Although something like `1.toString()` is a syntax error so you have to do `1['toString']()`

… or `(1).toString()` or `1..toString()`. ;)

svozza: >1..toString()

Ha. That's a new one, how does that work?
davidchambers: `1.` is a valid representation of one. The double dot removes the "decimal" ambiguity, I assume.

svozza: This implementation got really hairy in the end. To support `S.prop('length')` for strings I've had to manually autobox the final argument; given that I had to override the linter to do so this all seems a bit wrong.
davidchambers: I don't see a reason to support `S.prop('length', 'abc')` but not `S.prop('toFixed', 42)`.

We should be able to use the `Object` function to simplify matters:

```diff
-        if (typeof obj === 'string') {
-          /*jshint -W053 */
-          obj = new String(obj);
-          /*jshint +W053 */
-        }
-        if (obj != null && typeof obj !== 'number' &&
-           typeof obj !== 'boolean' && key in obj) {
+        if (obj != null && key in Object(obj)) {
```

svozza: Ah yes, that's _much_ better.

:zap: 
svozza: Actually, can we remove the `null` check and reinstate the `Accessible` type constraint now?
davidchambers: > Actually, can we remove the `null` check and reinstate the `Accessible` type constraint now?

Wouldn't that send us back [here][1]?


[1]: https://github.com/plaid/sanctuary/pull/161#issuecomment-192885727

svozza: > Wouldn't that send us back here?

Not if we do something like this:

```js
var boxed = Object(obj);
if (key in boxed) {
  return boxed[key];
...
```
davidchambers: Ah, I see what you're saying. Sounds great to me!

svozza: This is shaping up a lot better now. :zap:
svozza: It's a shame we can't leave custom reactions to comments like :zap: 
davidchambers: > It's a shame we can't leave custom reactions to comments like :zap:

Haha! With `:zap:`, though, the fact that a comment generates an email notification is usually desirable.

svozza: Actually, that's a good point.
svozza: Is there any more that needs to be done on this?
davidchambers: > Is there any more that needs to be done on this?

No, but I'd like to do a few other things before merging this pull request:

1.  Update and merge plaid/sanctuary-def#38
2.  Publish `sanctuary-def@0.5.0`
3.  Submit and merge a pull request to update our sanctuary-def dependency (to address #165)
4.  Publish `sanctuary@0.9.2`

I plan to take care of these things this evening. :)

davidchambers: > I plan to take care of these things this evening. :)

Actually, I did my tax return this evening instead. :\

svozza: Haha. Rather you than me!
davidchambers: Could you rebase this branch, Stefano?

svozza: Yes, of course. :zap:
davidchambers: You'll need to update the expected error messages. ;)

While you're at it, you can take advantage of the `assert.throws` :alien:.

svozza: Yup, on it!

Patch:
diff --git a/index.js b/index.js
index 6a74cac..a2b7a64 100644
--- a/index.js
+++ b/index.js
@@ -353,6 +353,22 @@
                function() { return R.apply(f, R.prepend(this, arguments)); });
   };
 
+  //  prop :: Accessible a => String -> a -> b
+  var prop =
+  def('prop',
+      {a: [Accessible]},
+      [$.String, a, b],
+      function(key, obj) {
+        var boxed = Object(obj);
+        if (key in boxed) {
+          return boxed[key];
+        } else {
+          throw new TypeError('‘prop’ expected object to have a property ' +
+                              'named ‘' + key + '’; ' +
+                              R.toString(obj) + ' does not');
+        }
+      });
+
   //. ### Classify
 
   //# type :: a -> String
@@ -1024,7 +1040,7 @@
   method('Maybe#toBoolean',
          {},
          [$Maybe(a), $.Boolean],
-         R.prop('isJust'));
+         prop('isJust'));
 
   //# Maybe#toString :: Maybe a ~> String
   //.
@@ -1111,7 +1127,7 @@
   def('isNothing',
       {},
       [$Maybe(a), $.Boolean],
-      R.prop('isNothing'));
+      prop('isNothing'));
 
   //# isJust :: Maybe a -> Boolean
   //.
@@ -1128,7 +1144,7 @@
   def('isJust',
       {},
       [$Maybe(a), $.Boolean],
-      R.prop('isJust'));
+      prop('isJust'));
 
   //# fromMaybe :: a -> Maybe a -> a
   //.
@@ -1532,7 +1548,7 @@
   method('Either#toBoolean',
          {},
          [$Either(a, b), $.Boolean],
-         R.prop('isRight'));
+         prop('isRight'));
 
   //# Either#toString :: Either a b ~> String
   //.
@@ -1621,7 +1637,7 @@
   def('isLeft',
       {},
       [$Either(a, b), $.Boolean],
-      R.prop('isLeft'));
+      prop('isLeft'));
 
   //# isRight :: Either a b -> Boolean
   //.
@@ -1638,7 +1654,7 @@
   def('isRight',
       {},
       [$Either(a, b), $.Boolean],
-      R.prop('isRight'));
+      prop('isRight'));
 
   //# either :: (a -> c) -> (b -> c) -> Either a b -> c
   //.
@@ -1715,7 +1731,7 @@
   //. > S.encaseEither(S.I, JSON.parse, '[')
   //. Left(new SyntaxError('Unexpected end of input'))
   //.
-  //. > S.encaseEither(R.prop('message'), JSON.parse, '[')
+  //. > S.encaseEither(S.prop('message'), JSON.parse, '[')
   //. Left('Unexpected end of input')
   //. ```
   S.encaseEither =
@@ -2407,6 +2423,20 @@
 
   //. ### Object
 
+  //# prop :: Accessible a => String -> a -> b
+  //.
+  //. Takes a property name and an object with known properties and returns
+  //. the value of the specified property. If for some reason the object
+  //. lacks the specified property, a type error is thrown.
+  //.
+  //. For accessing properties of uncertain objects, use [`get`](#get) instead.
+  //.
+  //. ```javascript
+  //. > S.prop('a', {a: 1, b: 2})
+  //. 1
+  //. ```
+  S.prop = prop;
+
   //# get :: Accessible a => TypeRep b -> String -> a -> Maybe b
   //.
   //. Takes a [type representative](#type-representatives), a property
@@ -2417,7 +2447,7 @@
   //. The `Object` type representative may be used as a catch-all since most
   //. values have `Object.prototype` in their prototype chains.
   //.
-  //. See also [`gets`](#gets).
+  //. See also [`gets`](#gets) and [`prop`](#prop).
   //.
   //. ```javascript
   //. > S.get(Number, 'x', {x: 1, y: 2})


Test patch:
diff --git a/test/index.js b/test/index.js
index 5927dec..217239a 100644
--- a/test/index.js
+++ b/test/index.js
@@ -2452,7 +2452,7 @@ describe('either', function() {
     });
 
     it('applies the first argument to the Error', function() {
-      eq(S.encaseEither(R.prop('message'), factorial, -1),
+      eq(S.encaseEither(S.prop('message'), factorial, -1),
          S.Left('Cannot determine factorial of negative number'));
     });
 
@@ -2512,7 +2512,7 @@ describe('either', function() {
     });
 
     it('applies the first argument to the Error', function() {
-      eq(S.encaseEither2(R.prop('message'), rem, 42, 0),
+      eq(S.encaseEither2(S.prop('message'), rem, 42, 0),
          S.Left('Cannot divide by zero'));
     });
 
@@ -2573,7 +2573,7 @@ describe('either', function() {
     });
 
     it('applies the first argument to the Error', function() {
-      eq(S.encaseEither3(R.prop('message'), area, 2, 2, 5),
+      eq(S.encaseEither3(S.prop('message'), area, 2, 2, 5),
          S.Left('Impossible triangle'));
     });
 
@@ -4105,6 +4105,76 @@ describe('list', function() {
 
 describe('object', function() {
 
+  describe('prop', function() {
+
+    it('is a binary function', function() {
+      eq(typeof S.prop, 'function');
+      eq(S.prop.length, 2);
+    });
+
+    it('type checks its arguments', function() {
+      throws(function() { S.prop(1); },
+             errorEq(TypeError,
+                     'Invalid value\n' +
+                     '\n' +
+                     'prop :: Accessible a => String -> a -> b\n' +
+                     '                        ^^^^^^\n' +
+                     '                          1\n' +
+                     '\n' +
+                     '1)  1 :: Number, FiniteNumber, NonZeroFiniteNumber, Integer, ValidNumber\n' +
+                     '\n' +
+                     'The value at position 1 is not a member of ‘String’.\n'));
+
+      throws(function() { S.prop('a', null); },
+             errorEq(TypeError,
+                     'Type-class constraint violation\n' +
+                     '\n' +
+                     'prop :: Accessible a => String -> a -> b\n' +
+                     '        ^^^^^^^^^^^^              ^\n' +
+                     '                                  1\n' +
+                     '\n' +
+                     '1)  null :: Null\n' +
+                     '\n' +
+                     '‘prop’ requires ‘a’ to satisfy the Accessible type-class constraint; the value at position 1 does not.\n'));
+
+      throws(function() { S.prop('a', true); },
+             errorEq(TypeError,
+                     '‘prop’ expected object to have a property named ‘a’; true does not'));
+
+      throws(function() { S.prop('a', 1); },
+             errorEq(TypeError,
+                     '‘prop’ expected object to have a property named ‘a’; 1 does not'));
+    });
+
+    it('throws when the property is not present', function() {
+      throws(function() { S.prop('map', 'abcd'); },
+             errorEq(TypeError,
+                     '‘prop’ expected object to have a property named ‘map’; "abcd" does not'));
+
+      throws(function() { S.prop('c', {a: 0, b: 1}); },
+             errorEq(TypeError,
+                     '‘prop’ expected object to have a property named ‘c’; {"a": 0, "b": 1} does not'));
+
+      throws(function() { S.prop('xxx', [1, 2, 3]); },
+             errorEq(TypeError,
+                     '‘prop’ expected object to have a property named ‘xxx’; [1, 2, 3] does not'));
+    });
+
+    it('it returns the value of the specified object property', function() {
+      eq(S.prop('a', {a: 0, b: 1}), 0);
+      eq(S.prop('0', [1, 2, 3]), 1);
+      eq(S.prop('length', 'abc'), 3);
+      eq(S.prop('x', Object.create({x: 1, y: 2})), 1);
+      eq(S.prop('global', /x/g), true);
+    });
+
+    it('is curried', function() {
+      eq(S.prop('a').length, 1);
+      eq(S.prop('a')({a: 0, b: 1}), 0);
+    });
+
+  });
+
   describe('get', function() {
 
     it('is a ternary function', function() {