tc39 / proposal-error-stacks

ECMAScript Proposal, specs, and reference implementation for Error.prototype.stack / System.getStack
https://tc39.github.io/proposal-error-stacks/
MIT License
168 stars 13 forks source link

Need to define what happens when the .stack setter is invoked #7

Closed bzbarsky closed 7 years ago

bzbarsky commented 7 years ago

In practice, this ends up defining an own value property on the receiver in Firefox, and of course defines an own value property on the thing the set is happening on in browsers where .stack is a value property to start with. I fully expect the web to depend on this behavior.

See description of the Firefox behavior in https://mail.mozilla.org/pipermail/es-discuss/2017-January/047586.html

ljharb commented 7 years ago

Making Error.prototype.stack have a setter that throws on no arguments, and defines an own property with one, seems reasonable. It wouldn't be important imo in that setter to throw if the this value is not an Error object; and the current behavior seems to only define an own property when the receiver is not an Error object. Am I summarizing the thread correctly?

In any case, that should only be done if every browser (where stack is not an own data property) has a setter that does this.

However, I don't see how any part of the web could depend on a setter existing, since it doesn't exist in every browser - merely it might depend on "writing .stack is a noop on Error objects", which the current proposal ensures.

bzbarsky commented 7 years ago

It wouldn't be important imo in that setter to throw if the this value is not an Error object;

It would be very important to NOT throw.

and the current behavior seems to only define an own property when the receiver is not an Error object.

That's not correct, and I have no idea where you got the idea that that's the behavior. It always defines an own property on the receiver in Firefox. Simple testcase:

var err = new Error();
err.stack = 5;
alert(err.stack);

This alerts 5 in Firefox, Safari, Chrome, and Edge.

that should only be done if every browser (where stack is not an own data property) has a setter that does this.

You should really sit down and test what browser behavior is, sure. I told you what the Firefox behavior is, so that's a start.

However, I don't see how any part of the web could depend on a setter existing

Where do I start...

  1. In strict mode, a setter not existing would cause .stack assignments to throw. If no browser currently throws in that situation in strict mode, then this would be very likely to break sites.
  2. In non-strict mode, a setter not existing would cause .stack assignments to no-op instead of actually assigning the value. Since every browser currently assigns the value, that would be very likely be not web-compatible.

merely it might depend on "writing .stack is a noop on Error objects"

Not a single browser has that behavior.

which the current proposal ensures.

Yes, which is why the current proposal is not web-compatible.

ljharb commented 7 years ago

@bzbarsky you're being quite hostile in your tone, both here and in the thread. At the time I wrote this text, i did sit down and test the behavior, which is what I wrote out in the readme under "Compatibility". I didn't test Edge, and a number of browser versions have been released since that time, so it's entirely possible that it's inaccurate.

Note that these are all stage 2 concerns, so they're not urgent in any way right now, since this proposal isn't yet at any stage.

What I meant by "depend on a setter existing" was, "getting the property descriptor and extracting the set function". Quite obviously, if a getter existed without a setter, throwing where all browsers did not previously throw would be incompatible, which is why my comment said that adding a setter seems reasonable.

In browsers where you can extract the setter, .calling it on an Error object in fact is a noop - the "compatibility" section of the readme documents which browser have that behavior. Note that that is entirely different from assigning to stack directly on the error object.

bzbarsky commented 7 years ago

you're being quite hostile in your tone, both here and in the thread

Quite honestly, that's because I feel like you're wasting my time, what with asking me to file issues that are already filed, repeatedly making incorrect statements that you could verify with a trivial test, etc... I should probably just disengage from this whole discussion entirely; my main interest here is in not ending up with yet another TC39 spec that's not web-compatible because people couldn't be bothered to actually test things. :(

At the time I wrote this text, i did sit down and test the behavior

Then I have no idea how you ended up with the Firefox behavior you ended up with. The Firefox behavior hasn't changed since at least Firefox 39.... Did you test before that? But before that it wasn't an accessor property either...

getting the property descriptor and extracting the set function

Sure, no one depends on that. But if we posit that we have an accessor descriptor (which we are), then it sounds like we agree that the setter needs to exist.

In browsers where you can extract the setter, .calling it on an Error object in fact is a noop

No, it is not, as you can trivially check. This testcase:

var err = new Error;
var desc = Object.getOwnPropertyDescriptor(err, "stack"); // Edge
if (!desc) {
  desc = Object.getOwnPropertyDescriptor(Error.prototype, "stack"); // Firefox
}
desc.set.call(err, 5);
alert(err.stack);

alerts 5 in Firefox and Edge. Here's a live test: https://jsfiddle.net/acg4txLu/

Again, I'd really like to understand why you think it's a no-op!

Note that that is entirely different from assigning to stack directly on the error object.

It... actually can't be, if the object is an ordinary object: direct assignment will call the setter with the object as "this", just like .call() does.

ljharb commented 7 years ago

I agree they have the same effect; the difference is in how web code might be depending on it. It's possible I was checking in Nightly and that it behaves differently; but of course it's also possible that I made a mistake (although I feel confident I got the behavior from somewhere).

At any rate, thank you for the issue, I've put up #8 to resolve this, which adds a setter.