Membranes and chai assertions

[LucaFranceschini]

So I'm playing a bit with this module, and I noticed unexpected behavior when using membranes in chai assertions. Here's the code:

const { Membrane } = require('es7-membrane')
const chai = require('chai')

// Establish the Membrane.
const dryWetMB = new Membrane()

// Establish "wet" ObjectGraphHandler.
const wetHandler = dryWetMB.getHandlerByName('wet', { mustCreate: true })

// Establish "dry" ObjectGraphHandler.
const dryHandler = dryWetMB.getHandlerByName('dry', { mustCreate: true })

const wet = { }

// Establish "wet" view of an object.
// Get a "dry" view of the same object.
const dry = dryWetMB.convertArgumentToProxy(
  wetHandler,
  dryHandler,
  wet
)

// enable should-style assertions
chai.should()

chai.expect(wet).to.equal(wet)  // ok
chai.expect(dry).to.equal(dry)  // ok

wet.should.equal(wet)  // ok
dry.should.equal(dry)  // error

(the file can be run as it is with node)

While everything works fine with expect assertions, the last should test fails:

[...]/node_modules/es7-membrane/docs/dist/node/es7-membrane.js:2279
      throw ex;
      ^
AssertionError: expected {} to equal {}
    at [...]

My guess is that something weird is happening with the this binding and/or with the function call argument, so that in the end they're not the same object anymore. But I didn't go too deep into how membranes work.

Is this (pun not intended) expected? Am I missing something here?

P.S.: my understanding is that one of the goals of this module (when not using traps) is to produce an object as much indistinguishable as possible from the original "wet" one, is this correct?

[ajvincent]

I'm not familiar with mocha or chai. That said, I do use Jasmine-style tests, and they do pass when running make: https://github.com/ajvincent/es-membrane/blob/master/spec/concepts.js

The fact that your second test (chai.expect(dry).to.equal(dry)) passes is a strong hint that something strange is going on indeed, especially with the should property that appears out of nowhere courtesy of the chai.should() call.

I can't investigate this today or tomorrow - but fortunately there's a three-day weekend coming up. Here's what I need:

• A link to the mocha / chai frameworks, including installation instructions for local builds
• Documentation on how mocha / chai work.
• if possible, a contact with someone who works on mocha or chai, as there's a decent chance here the bug is in their code, not mine

If practical, I'd love to arrange a live debugging session over video chat somewhere. Of course, time zones are an issue. (I live in Pacific Standard Time.)

[ajvincent]

On another note: const { Membrane } = require('es7-membrane') That shouldn't work anymore: I recently renamed the project to es-membrane. But I also had trouble verifying that deprecating es7-membrane works...

[LucaFranceschini]

I'm not familiar with mocha or chai. That said, I do use Jasmine-style tests, and they do pass when running make: https://github.com/ajvincent/es-membrane/blob/master/spec/concepts.js

First, I'm not sure why I talked about mocha at all: that's just the testing framework I was using, but indeed the problem is all about chai assertions. I edited the OP so that mocha is not needed anymore. Sorry for confusion.

About chai: it is an assertion library, much like the assertions provided by Jasmine. It is not a testing framework.

The Jasmine test suite you linked seems to use the expect-style, and nothing strange happens with that since it does not modify the objects. In other words, the following two are basically equal:

expect(x).to.equal(y)
x === y

The fact that your second test (chai.expect(dry).to.equal(dry)) passes is a strong hint that something strange is going on indeed, especially with the should property that appears out of nowhere courtesy of the chai.should() call.

As you can imagine, when doing chai.should() the property should is added to Object.prototype (http://chaijs.com/guide/styles/#should). Without getting too much into it, I assume it captures the target object when looking up the should property, and later calls to equal compare the argument to it.

I can't investigate this today or tomorrow - but fortunately there's a three-day weekend coming up. Here's what I need:

• A link to the mocha / chai frameworks, including installation instructions for local builds
• Documentation on how mocha / chai work.

Chai framework: http://chaijs.com/ Local installation is really as simple as npm i chai: http://chaijs.com/guide/installation/ Chai should assertion style: http://chaijs.com/guide/styles/#should

• if possible, a contact with someone who works on mocha or chai, as there's a decent chance here the bug is in their code, not mine

I have no contacts with the chai team (never had an issue with that). FWIW, I successfully used chai should-style assertions with proxies. On the other hand, I assume your handler traps are more complex than mines.

If practical, I'd love to arrange a live debugging session over video chat somewhere. Of course, time zones are an issue. (I live in Pacific Standard Time.)

Yeah, a bit of a problem, UTC+1 here. Will think about it.

On another note: const { Membrane } = require('es7-membrane') That shouldn't work anymore: I recently renamed the project to es-membrane. But I also had trouble verifying that deprecating es7-membrane works...

Yeah I noticed your renaming. I tried the new name and it works, that's all I can say :P

Thank you for your support, much appreciated.

[LucaFranceschini]

I investigated a bit. It looks like should captures the receiver of the property lookup process (the value of this when getters are encountered), not the target (I'm talking about this). Depending on what the membrane proxy does, this may be useful to understand the problem. I can imagine it has to do with the property access trap of the proxy handler.

Code:

const chai = require('chai')

chai.should()

const target = { }
const receiver = { }

Reflect.get(target, 'should', receiver).equal(receiver)  // ok
Reflect.get(target, 'should', receiver).equal(target)    // error

[ajvincent]

I'm going to bet that the line: dry.should.equal(wet)

passes. Could you quickly run that test?

[LucaFranceschini]

At this point I expected it to pass as well, but I got the very same error.

[ajvincent]

Ok, that target-versus-receiver part is very interesting, and admittedly one of the more obscure parts of the specification. I honestly don't know if I get this part right.

Reading my own code, I pass receiver through, without doing wrap/unwrap operations. That could be the bug here.

[LucaFranceschini]

Glad to hear you have a path to follow. More info (not so much admittedly) on receivers here: https://stackoverflow.com/questions/37563495/what-is-a-receiver-in-javascript https://github.com/getify/You-Dont-Know-JS/blob/master/es6%20%26%20beyond/ch7.md#proxies

As far as I know, the only cases in which the receiver is not the object in which we're looking for the property are

• proxy.prop, where the target is the proxied object but the receiver is the proxy itself
• explicitly using Reflect.get to set a value for this

Note that proxies, Reflect and receivers all joined the party with ES6: before that, the internal [[Get]] method did not have a receiver as an argument.

Basically, receivers only make sense with proxies.

Edit: oh, also, receivers are only relevant for getters and setters, of course.

[ajvincent]

Apologies for not getting back to you on this: another bug I was working on took much longer than I expected.

[ajvincent]

@LucaFranceschini Please review commit f2224e5.

1. spec/non-membrane/receiver.js appears to be a simple testcase for Reflect.get() and Reflect.set() using a receiver.
2. I added two new tests to the end of spec/concepts.js which I believe effectively runs the same test across a membrane, and the code as-is passed.

If these tests are valid, then I need either a non-chai testcase demonstrating what I did wrong... or we need to go yell at some chai developers. :-)

[LucaFranceschini]

Thanks.

Here is a non-chai test that does not do what I expect. This is a really naive implementation of should, which is very similar to what chai does (this is my understanding after giving a look to their code, but I'm in no way involved in the development of chai so I may be wrong).

const { Membrane } = require('./docs/dist/node/es-membrane')
const assert = require('assert')

const wet = { }

const dryWetMB = new Membrane()
const wetHandler = dryWetMB.getHandlerByName('wet', { mustCreate: true })
const dryHandler = dryWetMB.getHandlerByName('dry', { mustCreate: true })
const dry = dryWetMB.convertArgumentToProxy(wetHandler, dryHandler, wet)

// objects returned by `should`
function Assertion(obj) {
    this._obj = obj
}

Assertion.prototype.equal = function equal(other) {
    assert(this._obj === other)
}

Object.defineProperty(Object.prototype, 'should', {
    configurable: true,
    get: function () {
        return new Assertion(this)
    }
})

wet.should.equal(wet) // ok
dry.should.equal(dry) // error

AssertionError [ERR_ASSERTION]: false == true

Even with this naive implementation dry.should.equal(wet) fails as well. Furthermore, the receiver seems to be correctly forwarded by the membrane implementation, the following succeeds:

assert(dry.should._obj === dry)

This seems to be more about the function invocation. So I tried to see whether the function actually receives the correct this:

Assertion.prototype.equalDry = function equal() {
    assert(this._obj === dry)
}

dry.should.equalDry()

And it succeeds. So it looks to me like some weird wrapping/unwrapping on the argument is happening, does it make sense?

[ajvincent]

OK, here is your testcase rewritten for Jasmine:

const wet = { }

const dryWetMB = new Membrane()
const wetHandler = dryWetMB.getHandlerByName('wet', { mustCreate: true })
const dryHandler = dryWetMB.getHandlerByName('dry', { mustCreate: true })
const dry = dryWetMB.convertArgumentToProxy(wetHandler, dryHandler, wet);

// objects returned by `should`
function ObjectWrapper(obj) {
    this._obj = obj
}

ObjectWrapper.prototype.equal = function equal(other) {
    return (this._obj === other);
}

Object.defineProperty(Object.prototype, 'should', {
    configurable: true,
    get: function () {
        return new ObjectWrapper(this);
    }
})

describe("should tests", function() {
  beforeEach(function() {
    jasmine.DEFAULT_TIMEOUT_INTERVAL = 120000;
  });
  it("for wet", function() {
    const rv = wet.should.equal(wet);
    expect(rv).toBe(true);
  });
  it("for dry", function() {
    debugger;
    const part1 = dry.should;
    const part2 = part1.equal(dry);
    expect(part2).toBe(true);
  });
});

Using my Membrane Laboratory with settings:

• Script environment: Ordinary JavaScript
• Test environment: Jasmine 2.8.0
• code above inserted via the Add Script button, which opens up a CodeMirror editing area

[ajvincent]

I think I understand the root cause of this "bug", but not exactly how to explain it...

The problem starts with ObjectWrapper - the constructor function , specifically - not belonging to any particular object graph. That means when new ObjectWrapper is invoked from the dry side, it's going to invoke a membrane-agnostic function and return a non-wrapped object. But it's handed a wrapped object as its first argument. Right there, we've introduced a cross-graph operation, which has to be handled very carefully - and here, it isn't.

The non-wrapped instance of ObjectWrapper then gets wrapped with an origin on the dry side, with its obj property having its origin on the wet side. That's always, always going to be a tricky situation.

Later, inside the equal() function, we're once again on the dry side, where I noticed two truths:

other === wet
this._obj === dry

Since dry is a proxy for wet, dry cannot equal wet, and the test fails.

Now, you might fairly ask, why does the Membrane library handle things this way? It operates under the assumption that any unwrapped object it's never seen before comes from the object graph it was created under. In other words, if dry.should internally returns a new value, that new value belongs to the dry object graph. Each object graph should see only objects it generates and proxies to objects from other object graphs.

The intent of the ObjectWrapper, however, violates this assumption. Object.prototype.should, as defined here, is completely unaware of the membrane that it depends on. So is ObjectWrapper.

I'll talk about solutions in a later post.

[ajvincent]

Perhaps the right solution, ultimately, is to have the Membrane scan the properties of the newly constructed object for cross-graph contamination just as the object is being wrapped in a proxy.

[ajvincent]

No, that's not going to work: the membrane cannot alter any object it's received under normal conditions.

The truly best solution would be to run the wet and dry code in different "realms" or "sandboxes", which the Membrane is supposed to help you construct. Really, code from two object graphs should never mix, and I went to a lot of trouble in my testcases to avoid that.

The second-best solution is to write a membrane-aware variant of ObjectWrapper:

// objects returned by `should`
function ObjectWrapper(obj) {
  if (dryWetMB.hasProxyForValue(wetHandler.fieldName, obj)
    this._obj = dryWetMB.convertArgumentToProxy(dryHandler, wetHandler, obj);
  else
    this._obj = obj;
}

You could try doing something similar with Object.prototype.should...

The third-best solution would be to create a proxy for ObjectWrapper to be used on the dry side. But this gets into name-collision issues, because you don't want this proxy to be seen on the wet side. In a sense, we're back to closures with this approach.

I'm sorry, but I just don't see any way for a Membrane to provide a wholly-native solution for this, not without causing some other side-effect (like exposing an unwrapped object to a wrapped object).

[ajvincent]

Earlier I wrote,

No, that's not going to work: the membrane cannot alter any object it's received under normal conditions.

Maybe there's some wiggle room - specifically, with a configuration option.

The code that matters is in ObjectGraphHandler.apply (and equivalent code in construct).

      _this = this.membrane.convertArgumentToProxy(
        this,
        argHandler,
        thisArg,
        Object.create(optionsBase, { "isThis": new DataDescriptor(true) })
      );

      for (let i = 0; i < argumentsList.length; i++) {
        let nextArg = argumentsList[i];
        nextArg = this.membrane.convertArgumentToProxy(
          this,
          argHandler,
          nextArg,
          Object.create(optionsBase, { "argIndex": new DataDescriptor(i) })
        );
        args.push(nextArg);
      }

If the Membrane is explicitly given permission to replace properties of _this and nextArg which are from the proxy's object graph, then it can do the search-and-replace safely before executing the actual target function.

Suppose there was a "fixProperties": true setting in the Membrane configuration. Then in convertArgumentToProxy, I could do something like this:

// this is pseudocode
let keys = Reflect.ownKeys(arg);
keys.forEach(function(key) {
  const desc = Reflect.getOwnPropertyDescriptor(arg, key);
  if (!isDataDescriptor(desc) || (valueType(desc.value) == "primitive"))
    return;
  const [found, prop] = this.getMembraneProxy(originHandler.fieldName, desc.value);
  if (!found || (prop !== desc.value))
    return;
  desc.value = this.convertArgumentToProxy(originHandler, targetHandler, desc.value);
  Reflect.defineProperty(arg, key, desc);
}, this);

This is of course conditional on whether arg is frozen/sealed or not, or if the particular key is configurable or not, or some other weird setting. But that's also something we can work around, since the convertArgumentToProxy code is likely to be creating the proxy in this case anyway...

There's also the recursive case to consider (a.b.c === a).

What configuration API I should define for this is very unclear right now. I welcome suggestions.

[erights]

Attn @tvcutsem

Tom, please take a good look at this. Thanks.

[ajvincent]

For what it's worth, there's another possibility, an inconsistency that actually bothers me a bit. In Membrane.js, I have this block of code:

    if (keys.includes("get")) {
      wrappedDesc.get = function wrappedGetter () {
        const wrappedThis = membrane.convertArgumentToProxy(originHandler, targetHandler, this);
        return membrane.convertArgumentToProxy(
          originHandler,
          targetHandler,
          desc.get.call(wrappedThis)
        );
      };
    }

    if (keys.includes("set") && (typeof desc.set === "function")) {
      const wrappedSetter = function(value) {
        const wrappedThis  = membrane.convertArgumentToProxy(originHandler, targetHandler, this);
        const wrappedValue = membrane.convertArgumentToProxy(originHandler, targetHandler, value);
        return membrane.convertArgumentToProxy(
          targetHandler,
          originHandler,
          desc.set.call(wrappedThis, wrappedValue)
        );
      };
      this.buildMapping(targetHandler, wrappedSetter);
      wrappedDesc.set = wrappedSetter;
}

Note the return call for the getter. The wrapping of the return value has the originHandler first and the targetHandler second. No matter what, the return wrapping should be in the reverse order of the argument wrapping.

Luca, your testcase passes when I rewrite this block as:

    if (keys.includes("get")) {
      wrappedDesc.get = function wrappedGetter () {
        const wrappedThis = membrane.convertArgumentToProxy(targetHandler, originHandler, this);
        return membrane.convertArgumentToProxy(
          originHandler,
          targetHandler,
          desc.get.call(wrappedThis)
        );
      };
    }

    if (keys.includes("set") && (typeof desc.set === "function")) {
      const wrappedSetter = function(value) {
        const wrappedThis  = membrane.convertArgumentToProxy(targetHandler, originHandler, this);
        const wrappedValue = membrane.convertArgumentToProxy(targetHandler, originHandler, value);
        return membrane.convertArgumentToProxy(
          originHandler,
          targetHandler,
          desc.set.call(wrappedThis, wrappedValue)
        );
      };
      this.buildMapping(targetHandler, wrappedSetter);
      wrappedDesc.set = wrappedSetter;
    }

But it causes another testcase of mine to fail:

  it("Setters should wrap and unwrap values correctly", function () {
    var extraHolder;
    const desc = {
      get: function() { return extraHolder; },
      set: function(val) { 
        extraHolder = val;
        return val;
      },
      enumerable: true,
      configurable: true
    };

    Reflect.defineProperty(dryDocument, "extra", desc);

    var dryExtra = {};
    dryDocument.extra = dryExtra;

    expect(typeof extraHolder).toBe("object");
    expect(extraHolder).not.toBe(dryExtra); // fails here
    expect(wetDocument.extra).toBe(extraHolder); // fails here

    expect(dryDocument.extra).toBe(dryExtra);
  });

I think this is telling me that the test itself might also be incorrect, for reasons I don't fully understand yet.

One of my oldest sayings is "The first step in confirming there is a bug in someone else's work is confirming there are none in your own." It's really a warning to myself to be willing to recheck my own work.

[ajvincent]

It turns out this really was just a bug in the membrane implementation. Luca, thank you very much for finding this!! This has now been fixed, and a new npm build will be released soon.

[LucaFranceschini]

Thanks to you for prompt support. Didn't really follow all the details in the last posts but I'm glad you sorted it out! :tada:

[ajvincent]

Feel free to join the releases mailing list at https://groups.google.com/forum/#!forum/es-membrane

[LucaFranceschini]

@ajvincent one last thing: is what I said in the OP P.S. true?

P.S.: my understanding is that one of the goals of this module (when not using traps) is to produce an object as much indistinguishable as possible from the original "wet" one, is this correct?

I know your goal is to provide support for distortions, revocation and possibly other things, I'm just wondering about transparency.

[ajvincent]

Almost correct: the wrapped "dry" object should be indistinguishable from the "wet" object, with the obvious exception that dry !== wet.

[tvcutsem]

Even though the bug is now fixed, since @erights asked me to review, I'll just chime in with a few observations.

First, I'm glad you didn't need to pursue the 'fixProperties' config option. A membrane proxy 'finding and fixing' properties when a value is passed through would not be a good idea IMHO. If you find yourself needing this, it probably means the 'initial conditions' of the membrane were not set up in the right way.

Second, in observing the bugfix commit, it seems the root cause of the bug was the switching of 'originHandler' and 'targetHandler', which is very subtle indeed. I remember when I wrote my membrane implementation (far less comprehensive and well-tested as yours), one thing that really helped was to consistently give all variables a 'dry' or 'wet' prefix (like here: https://github.com/tvcutsem/harmony-reflect/blob/master/examples/generic_membrane.js#L189). I also named my transformation functions 'wet2dry' and 'dry2wet'. Like that, my variable names acted like a (manually checked) type system for tracking the flow of values through the membrane. Applying a similar idiom might help to avoid bugs like this going forward.