sinon.restore cannot restore spies

[runspired]

Version: 15.0.1

Assuming a class instance like so

class Thing {
  aMethod() {}
}
const thing = new Thing();

and a spy like so

sinon.spy(thing, 'aMethod');

a global restore will error

sinon.restore();

This is because sinon attempts to directly set target[property] back to the original method, which is caught by the spy proxy.

Example Error:

TypeError: Cannot assign to read only property 'setRequestReviewers' of object '[object Object]'
         at Function.restore 

[runspired]

hrm, actually this runs deeper. I changed this to restoring the spy directly and it still errors in this way. So I dug a bit and I think sinon isn't accounting for decorated methods.

[fatso83]

sinon isn't accounting for decorated methods.

To be honest, I am not sure what you mean by this. Do you mean the not-yet-standardized (?) annotations employed by some frameworks and applied by Babel and such, or something else?

Would be great with a minimal reproduction if someone was to look into fixing this, such as a repo with 2-3 files

[fatso83]

@runspired Would you be able to provide us with a reproduction case? As in with full setup and transpilation, etc. I assume that TC39 will be accepted in the near future, and so it would be nice if we already had support in place for whatever that might throw at us.

[runspired]

All you have to do is define with defineProperty with the config seen above in the screenshots. Doesn't even take a decorator, decorator is just the common case. Happy next week to give you a gist or some such.

[fatso83]

Great. Looking forward to it, as even with those hints I am not totally sure what the bug actually is, so a repro is definitely warranted :)

[stevenhair]

I also ran into this problem - it seems that it was introduced in 15.0.2 (I'm guessing in #2499). My code uses decorators, but I haven't been able to isolate the issue yet.

@runspired, did you get a chance to put a reproduction together? I could take a stab at it if you haven't.

[fatso83]

@stevenhair please take a stab at it. would be interesting to know if 15.0.3 fixed anything. See CHANGES.

[runspired]

The fail scenario here can be triggered by doing the following.

/*
class TestedObject {
  @aDecorator
  myMethod;
}
*/

var _class, _descriptor;
function _initializerDefineProperty(target, property, descriptor, context) { if (!descriptor) return; Object.defineProperty(target, property, { enumerable: descriptor.enumerable, configurable: descriptor.configurable, writable: descriptor.writable, value: descriptor.initializer ? descriptor.initializer.call(context) : void 0 }); }
function _defineProperty(obj, key, value) { key = _toPropertyKey(key); if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }
function _toPropertyKey(arg) { var key = _toPrimitive(arg, "string"); return typeof key === "symbol" ? key : String(key); }
function _toPrimitive(input, hint) { if (typeof input !== "object" || input === null) return input; var prim = input[Symbol.toPrimitive]; if (prim !== undefined) { var res = prim.call(input, hint || "default"); if (typeof res !== "object") return res; throw new TypeError("@@toPrimitive must return a primitive value."); } return (hint === "string" ? String : Number)(input); }
function _applyDecoratedDescriptor(target, property, decorators, descriptor, context) { var desc = {}; Object.keys(descriptor).forEach(function (key) { desc[key] = descriptor[key]; }); desc.enumerable = !!desc.enumerable; desc.configurable = !!desc.configurable; if ('value' in desc || desc.initializer) { desc.writable = true; } desc = decorators.slice().reverse().reduce(function (desc, decorator) { return decorator(target, property, desc) || desc; }, desc); if (context && desc.initializer !== void 0) { desc.value = desc.initializer ? desc.initializer.call(context) : void 0; desc.initializer = undefined; } if (desc.initializer === void 0) { Object.defineProperty(target, property, desc); desc = null; } return desc; }
function _initializerWarningHelper(descriptor, context) { throw new Error('Decorating class property failed. Please ensure that ' + 'proposal-class-properties is enabled and runs after the decorators transform.'); }
let TestedObject = (_class = class TestedObject {
  constructor() {
    _initializerDefineProperty(this, "myMethod", _descriptor, this);
  }
}, (_descriptor = _applyDecoratedDescriptor(_class.prototype, "myMethod", [aDecorator], {
  configurable: true,
  enumerable: true,
  writable: true,
  initializer: null
})), _class);

function aDecorator() {
  const desc = {}
  // uncomment these to make sinon work
  // desc.writable = true;
  // desc.configurable = false;
  desc.value = function() { console.log("decorated method works"); }
  return desc;
}

const testObject = new TestedObject();
let spy = sinon.spy(testObject, 'myMethod');
testObject.myMethod();
sinon.restore();

The above is the common case for how this might happen in an app using decorators. A very simple reproduction is the following:

class TestedObject {}

Object.defineProperty(TestedObject.prototype, 'myMethod', {
  value: function() { console.log("decorated method works"); }
});

const testObject = new TestedObject();
let spy = sinon.spy(testObject, 'myMethod');
testObject.myMethod();
sinon.restore();

[runspired]

can also confirm that this is NOT fixed by 15.0.3

[fatso83]

@runspired Thanks for providing the reproduction. This is exactly what was needed.

Bear in mind that the simplified version will never work for any library trying to modify the prop. When you just specify value in the property descriptor you will get the default values for configurable and writable, which is false. If a value is not configurable, we (Sinon) cannot do anything about it once it is set. We cannot repurpose it or do anything. So Sinon cannot do something about that bit of code in any case. Nothing to fix there.

As to the more advanced version, you can (again) see that Sinon is not to blame here, as we need a configurable property descriptor and if you just add this bit of code after your aDescriptor function definition you will see that it is not:

console.log(
    Object.getOwnPropertyDescriptor(TestedObject.prototype, "myMethod")
);

This prints

$ node repro2.test.cjs 
{
  value: [Function (anonymous)],
  writable: false,
  enumerable: false,
  configurable: false
}

If this is the production code running, then the problem is your decorator implementation. This is easy to spot in the _applyDecoratedDescriptor when it runs its reduction pipeline:

        .reduce(function (desc, decorator) {
            return decorator(target, property, desc) || desc;
        }, desc);

Even if this decorator was originally configurable it will be replaced by what your decorator returns. And since your decorator just returns a decorator with the value field set, you will get an uconfigurable property descriptor.

Not a bug in Sinon, in other words. We don't do magic 🪄 so nothing for us to do.

[fatso83]

Closing was potentially a bit premature, as there might be other details I can have missed. I will leave this open for a while so that all details are covered.

[fatso83]

AFAIK, it seems (I might be wrong) that whatever implementation/transpilation step you are using to produce the above seems to implement descriptors incorrectly. This is, of course, just me casually browsing the TC39 proposal for descriptors, but it seems that unless the field or method is a Private Name (as in #myMethod) it should have configurable: true, which is not the case for the above code.

I also had a look at the text in the GitHub proposal and it also did not seem to imply that this was a thing that was intended. There's nothing there that Sinon should not be fine handling, essentially.

[runspired]

So Sinon cannot do something about that bit of code in any case. Nothing to fix there.

Ah, but Sinon does do something. That something is that it still installs the spy, because remember this is on the prototype, not the instance, but we spy the instance. The trouble is when sinon then tries to restore the spy at which point it attempts to assign in a manner that affects the prototype.

If sinon wants to add a check and blow up earlier here with a helpful warning that would be very understandable :) However, I also suspect that setting up and tearing down the spy on the instance only would work, considering it was installable in the first place.

Fwiw I did indeed work around this in our app by making it configurable once I realized what the issue was.

AFAIK, it seems (I might be wrong) that whatever implementation/transpilation step you are using to produce the above seems to implement descriptors incorrectly.

Yes and No - we're using legacy decorators at the moment, but they themselves don't set any values for the descriptor that your decorator doesn't set, so this means we get the browser defaults which as you noted above default to being fairly restrictive.

[fatso83]

Thanks for that extra input. That is valuable as I missed the part about Sinon modifying the instance. I now modified the case to add step-by-step info on what happens and can see something funky happening here in this section (see details below for more):

running TestedObject constructor:post definition: myMethod lives on prototype? true
testObject:pre spy: myMethod lives on prototype? true
testObject:pre spy undefined
testObject:post spy: myMethod lives on prototype? false
testObject:post spy {
  value: [Function (anonymous)],
  writable: false,
  enumerable: false,
  configurable: false
}

So myMethod lives on the prototype before Sinon comes around and then Sinon creates a new object descriptor that is more or less a copy of the one on the prototype and assigns its own value to the instance, which it then cannot remove (as it is non-configurable). That seems off... 😸 Feel free to look into the details of what happens internally 🥵

One thing I don't get about this is what happens when the constructor calls _initializerDefineProperty. To me, this looks like it assigns the object descriptor for myMethod to the instance (this), but for some reason, it is not present on the instance when I check it later. I am missing something. Do you have an explanation? 👀
EDIT: got it. I stepped through it. The descriptor passed in is null, so the constructor step does nothing.

repro case

"use strict";
const sinon = require("./lib/sinon");
const { assert } = require("@sinonjs/referee");
/* eslint-disable */

var _class, _descriptor;
function _initializerDefineProperty(target, property, descriptor, context) {
    if (!descriptor) {
        return;
    }
    Object.defineProperty(target, property, {
        enumerable: descriptor.enumerable,
        configurable: descriptor.configurable,
        writable: descriptor.writable,
        value: descriptor.initializer
            ? descriptor.initializer.call(context)
            : void 0,
    });
}
function _defineProperty(obj, key, value) {
    key = _toPropertyKey(key);
    if (key in obj) {
        Object.defineProperty(obj, key, {
            value: value,
            enumerable: true,
            configurable: true,
            writable: true,
        });
    } else {
        obj[key] = value;
    }
    return obj;
}
function _toPropertyKey(arg) {
    var key = _toPrimitive(arg, "string");
    return typeof key === "symbol" ? key : String(key);
}
function _toPrimitive(input, hint) {
    if (typeof input !== "object" || input === null) {
        return input;
    }
    var prim = input[Symbol.toPrimitive];
    if (prim !== undefined) {
        var res = prim.call(input, hint || "default");
        if (typeof res !== "object") {
            return res;
        }
        throw new TypeError("@@toPrimitive must return a primitive value.");
    }
    return (hint === "string" ? String : Number)(input);
}
function _applyDecoratedDescriptor(
    target,
    property,
    decorators,
    descriptor,
    context
) {
    console.log(
        "prototype pre decorator",
        Object.getOwnPropertyDescriptor(target, "myMethod")
    );

    var desc = {};
    Object.keys(descriptor).forEach(function (key) {
        desc[key] = descriptor[key];
    });
    desc.enumerable = Boolean(desc.enumerable);
    desc.configurable = Boolean(desc.configurable);
    if ("value" in desc || desc.initializer) {
        desc.writable = true;
    }
    desc = decorators
        .slice()
        .reverse()
        .reduce(function (desc, decorator) {
            return decorator(target, property, desc) || desc;
        }, desc);
    if (context && desc.initializer !== void 0) {
        desc.value = desc.initializer ? desc.initializer.call(context) : void 0;
        desc.initializer = undefined;
    }
    if (desc.initializer === void 0) {
        Object.defineProperty(target, property, desc);
        desc = null;
    }
    return desc;
}
function _initializerWarningHelper(descriptor, context) {
    throw new Error(
        "Decorating class property failed. Please ensure that " +
            "proposal-class-properties is enabled and runs after the decorators transform."
    );
}
const TestedObject =
    ((_class = class TestedObject {
        constructor() {
            console.log(
                "running TestedObject constructor:prototype definition of myMethod:pre",
                Object.getOwnPropertyDescriptor(
                    TestedObject.prototype,
                    "myMethod"
                )
            );
            console.log(
                "running TestedObject constructor:instance definition of myMethod:pre",
                Object.getOwnPropertyDescriptor(this, "myMethod")
            );
            _initializerDefineProperty(this, "myMethod", _descriptor, this);
            console.log(
                "running TestedObject constructor:prototype definition of myMethod:post",
                Object.getOwnPropertyDescriptor(
                    TestedObject.prototype,
                    "myMethod"
                )
            );
            console.log(
                "running TestedObject constructor:instance definition of myMethod:post",
                Object.getOwnPropertyDescriptor(this, "myMethod")
            );
            console.log(
                "running TestedObject constructor:descriptors:post definition",
                Object.getOwnPropertyDescriptors(this)
            );
            console.log(
                "running TestedObject constructor:post definition: myMethod lives on prototype?",
                this.myMethod === TestedObject.prototype.myMethod
            );
        }
    }),
    (_descriptor = _applyDecoratedDescriptor(
        _class.prototype,
        "myMethod",
        [aDecorator],
        {
            configurable: true,
            enumerable: true,
            writable: true,
            initializer: null,
        }
    )),
    _class);

function aDecorator() {
    const desc = {};
    // uncomment these to make sinon work
    // desc.writable = true;
    // desc.configurable = false;
    desc.value = function () {
        console.log("decorated method works");
    };
    return desc;
}

console.log(
    "prototype post decorator",
    Object.getOwnPropertyDescriptor(TestedObject.prototype, "myMethod")
);

const testObject = new TestedObject();
console.log(
    "testObject:pre spy: myMethod lives on prototype?",
    testObject.myMethod === TestedObject.prototype.myMethod
);
console.log(
    "testObject:pre spy",
    Object.getOwnPropertyDescriptor(testObject, "myMethod")
);
let spy = sinon.spy(testObject, "myMethod");
console.log(
    "testObject:post spy: myMethod lives on prototype?",
    testObject.myMethod === TestedObject.prototype.myMethod
);
console.log(
    "testObject:post spy",
    Object.getOwnPropertyDescriptor(testObject, "myMethod")
);
console.log(
    "prototype",
    Object.getOwnPropertyDescriptor(TestedObject.prototype, "myMethod")
);
testObject.myMethod();
sinon.restore();

output

$ node repro2.test.cjs 
prototype pre decorator undefined
prototype post decorator {
  value: [Function (anonymous)],
  writable: false,
  enumerable: false,
  configurable: false
}
running TestedObject constructor:prototype definition of myMethod:pre {
  value: [Function (anonymous)],
  writable: false,
  enumerable: false,
  configurable: false
}
running TestedObject constructor:instance definition of myMethod:pre undefined
running TestedObject constructor:prototype definition of myMethod:post {
  value: [Function (anonymous)],
  writable: false,
  enumerable: false,
  configurable: false
}
running TestedObject constructor:instance definition of myMethod:post undefined
running TestedObject constructor:descriptors:post definition {}
running TestedObject constructor:post definition: myMethod lives on prototype? true
testObject:pre spy: myMethod lives on prototype? true
testObject:pre spy undefined
testObject:post spy: myMethod lives on prototype? false
testObject:post spy {
  value: [Function (anonymous)],
  writable: false,
  enumerable: false,
  configurable: false
}
prototype {
  value: [Function (anonymous)],
  writable: false,
  enumerable: false,
  configurable: false
}
decorated method works
/home/carlerik/code/sinon/lib/sinon/util/core/wrap-method.js:222
                    object[property] = this.wrappedMethod;
                                     ^

TypeError: Cannot assign to read only property 'myMethod' of object '#<TestedObject>'
    at Function.restore (/home/carlerik/code/sinon/lib/sinon/util/core/wrap-method.js:222:38)
    at /home/carlerik/code/sinon/lib/sinon/sandbox.js:34:21
    at Array.forEach (<anonymous>)
    at applyOnEach (/home/carlerik/code/sinon/lib/sinon/sandbox.js:33:5)
    at Sandbox.restore (/home/carlerik/code/sinon/lib/sinon/sandbox.js:188:9)
    at Object.<anonymous> (/home/carlerik/code/sinon/repro2.test.cjs:183:7)
    at Module._compile (node:internal/modules/cjs/loader:1165:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1219:10)
    at Module.load (node:internal/modules/cjs/loader:1043:32)
    at Function.Module._load (node:internal/modules/cjs/loader:878:12)

[fatso83]

Think you can spot the issue in wrap-method.js (when mirroring the descriptor):

We cannot mirror the bit about it being non-configurable if we intend to restore it.

@mantoni / @mroderick : inputs? I do not want to break anything, but I cannot see why it makes sense to inject spies we cannot remove afterwards. Seems to make sense to ensure it is configurable: true.

[fatso83]

So ... this git diff makes the reproduction case pass:

diff --git a/lib/sinon/util/core/wrap-method.js b/lib/sinon/util/core/wrap-method.js
index 64127721..0da3b531 100644
--- a/lib/sinon/util/core/wrap-method.js
+++ b/lib/sinon/util/core/wrap-method.js
@@ -137,6 +137,7 @@ module.exports = function wrapMethod(object, property, method) {
         for (i = 0; i < types.length; i++) {
             mirrorProperties(methodDesc[types[i]], wrappedMethodDesc[types[i]]);
         }
+        methodDesc.configurable = true
         Object.defineProperty(object, property, methodDesc);
 
         // catch failing assignment

All other tests also pass, including cloud tests, so I am inclined towards merging this as a fix, unless I get some hands in the air saying no :)

[fatso83]

I created a fix in #2508, but it unraveled a can of worms when trying to make sure we have consistent behavior for spies, stubs, mocks and fakes.

[fatso83]

Fix published as 15.0.4

[runspired]

@fatso83 thank you very much! <3