fix: no more detached properties

[erights]

Addresses ljharb/object.assign#79 . Attn @ljharb . Please feel free to leave review comments. Question for you at the bottom of this note.

This reverts the enable-property-override mechanism to stop using the detached-properties mechanism and instead go back to the original strategy --- putting the original value on an own property of the getter function of the new accessor. However, rather than name this extra property .value as Caja did, we name it with the registered symbol Symbol.for('originalValue'). We export this symbol, but we also make it a registered symbol so it can be used by reconstructing it rather than importing it. By exposing the original value on any own property, harden still finds it and its transitive walk proceeds.

We give the property an obscure but reconstructible name so code like
https://github.com/ljharb/es-abstract/blob/0089955ffac420870f95e517d7fa8592afa17094/GetIntrinsic.js#L266

value = isOwn ? desc.get || desc.value : value[part];

which is the cause of ljharb/object.assign#79 , could be changed to the following. I write it out the long way with nested ifs just to make it clear. Recovering some of the brevity of the original is left as an exercise...

//either
import { originalValueSymbol } from 'something/enable-property-overrides';
//or
const originalValueSymbol = Symbol.for('originalValue');

if (isOwn) {
  const getter = desc.get;
  if (getter) {
    if (originalValueSymbol in getter) {
      // either
      value = getter[originalValueSymbol];
      // or
      value = value[part];
      // or
      value = getter();
    } else {
      value = getter;
    }
  } else {
    value = desc.value;
  }
} else {
  value = value[part];
}

@ljharb, once this PR is merged and deployed, would you be willing to adapt the above pattern in order to solve ljharb/object.assign#79 ?

[erights]

Can't resist the brevity challenge ;)

if (isOwn && 'get' in desc && !(originalValueSymbol in desc.get)) {
  value = desc.get;
} else {
  value = value[part];
}

[ljharb]

This is certainly an elegant way to special-case and detect SES, but having to do so at all still seems troubling. Is there no solution that could avoid relying on implementation details of SES?

[erights]

This is certainly an elegant way to special-case and detect SES, but having to do so at all still seems troubling. Is there no solution that could avoid relying on implementation details of SES?

I don't see one, but I'm open to suggestions. Do you see another approach to suppressing the override mistake other than turning the data property into an accessor before freezing? Note that the issue is not SES per se. It is any suppression of the override mistake, if this accessor trick is the only way to do so.

And I certainly agree that it is troubling! Everything about the override mistake has been troubling!

[ljharb]

In the case of Boolean.prototype.valueOf, are you truly concerned some code is going to need to do Object(true) and stick an own valueOf on it?

[erights]

In the case of Boolean.prototype.valueOf, are you truly concerned some code is going to need to do Object(true) and stick an own valueOf on it?

What? How did you get from our topic to this question? What's the relevance? I am not concerned about this at all as I don't see the relevance.

[kriskowal]

LGTM, notes are recommendations.

I’m suspicious of the benefit of using a registered symbol over merely get.value. It seems only to slightly lessen probability of conflict at expense of ease of use.

[ljharb]

@erights Oh right sorry, my issue is Function apply :-/ similarly tho, how often does code install an own apply property on functions?

The relevance is that I'm suggesting SES doesn't need to "fix" the override mistake so aggressively/optimistically - only where it would otherwise break code.

[erights]

@erights Oh right sorry, my issue is Function apply :-/ similarly tho, how often does code install an own apply property on functions?

The relevance is that I'm suggesting SES doesn't need to "fix" the override mistake so aggressively/optimistically - only where it would otherwise break code.

That's exactly what we're doing! We add cases to https://github.com/Agoric/SES-shim/blob/9a7a0975036d8d938263ba265f747876d7d91599/packages/ses/src/enablements.js#L67 based on what problems we encounter. As you see on that line, we found that tape overrides apply by assignment, and so was broken until we added apply to this whitelist.

[ljharb]

hmm, i maintain tape and am not aware of or able to locate any assignment of apply in it, only invocation (but even if that was a mistake and was corrected, it wouldn't fix the root issue here, ofc)

[erights]

(but even if that was a mistake and was corrected, it wouldn't fix the root issue here, ofc)

Can you explain what you see as the root issue? Thanks.

[erights]

hmm, i maintain tape and am not aware of or able to locate any assignment of apply in it, only invocation.

Filed #474 . Will remove if warranted, which now seems likely. Thanks!

[ljharb]

The root issue is that SES's fix to the override mistake breaks code like es-abstract (which, even if i update, is still deployed relatively widely and might not be updated for awhile) that assumes that data properties in the spec will always be data properties on first run.

[erights]

LGTM, notes are recommendations.

I’m suspicious of the benefit of using a registered symbol over merely get.value. It seems only to slightly lessen probability of conflict at expense of ease of use.

It does do exactly that. But for this purpose, ease of use is not a feature ;)

[erights]

The root issue is that SES's fix to the override mistake breaks code like es-abstract (which, even if i update, is still deployed relatively widely and might not be updated for awhile) that assumes that data properties in the spec will always be data properties on first run.

My policy is to only solve solvable problems ;). The harden proposal will propose that future engines have a way to directly suppress the override mistake. But all we can do with present engines is to suppress the override mistake using the elements of today's JS. Do you see any way to do so that does not violate this root issue? If not, what choice is there?

Remember, the affects lots of stakeholders which are already using this accessor technique to suppress the override mistake in many production systems.