Public fields "Define" semantics and subclasses

[rbuckton]

I believe we may need to revisit the Set vs. Define debate of public fields in light of this scenario:

class Base {
  x = 1;
}
class Sub extends Base {
  set x(value) { console.log(value); }
}

// (a)
const obj = new Sub(); 

// (b)
obj.x = 2;

Without an in-depth understanding of the spec, a naïve developer might assume that (a) would print 1 and (b) would print 2, however this is not the case. According to the proposal, x will be Defined on the instance of obj during construction. This means that the field x on Base will override the accessor x on Sub, which seems to violate developer intuition that members on Sub should override members on Base. In fact, the only way for Sub to override x with an accessor would be to use Object.defineProperty in the constructor of Sub to replace the field with an accessor on the instance.

I only see two solutions to solve this issue:

1. Split a field into two operations:
   1. Use Define on the prototype for Base to add a property with the descriptor: { enumerable: true, configurable: true, writable: true, value: undefined }
   2. Use Set in the constructor of Base to evaluate the initializer.
2. Use Set semantics for fields.

Option 1 is the most consistent with the current semantics that use Define (a field on a subclass overrides a member on the superclass), but solves the above issue.

Option 2 is consistent with the state of existing compile-to-javascript languages over the last 6 years.

[nicolo-ribaudo]

Using define+set, how would it be possible to create a decorator to make a field non-writable?

This would throw, or define a writable property after tc39/ecma262#1320:

class Foo {
  @readonly x = 2;
}

function readonly(desc) {
  desc.descriptor.writable = false;
}

[littledan]

I don't see what's new about this scenario. I agree that it's possible to write a code snippet which will be locally surprising, but that's true about all semantic choices. I think TC39 was thinking about this sort of case when it made the Define call.

[mbrowne]

Does anyone have an equally compelling code example where [[Set]] behavior is "locally surprising"? (I fully realize that "equally compelling" is somewhat subjective, so interpret that however you like.)

[mbrowne]

If we must live with [[Define]] as the default (however good or bad that turns out to be), then I think a decorator would be the best workaround for this case. Example:

class Sub extends Base {
  @instance
  set x(value) { console.log(value); }
}

[rbuckton]

@nicolo-ribaudo possibly with an "initializer" (or whatever we call it)?

function readonly(desc) {
  desc.extras = [{ kind: "initializer", placement: desc.placement, initializer: function() {
    // set writeability _after_ field is set, but only if it is an "own" property.
    const prop = Object.getOwnPropertyDescriptor(this, desc.key);
    if (!prop || prop.get || prop.set) throw new Error(`'${desc.key}' is not a field`);
    prop.writable = false;
    Object.defineProperty(this, desc.key, prop);
  }];
  return desc;
}

[nicolo-ribaudo]

Does anyone have an equally compelling code example where [[Set]] behavior is "locally surprising"? (I fully realize that "equally compelling" is somewhat subjective, so interpret that however you like.)

This comment convinced me that [[Set]] can't be worst than [[Define]]. I tried really hard to give an example, but I couldn't find an example as realistic as the arguments against [[Define]].
The only example I found is this:

// Library code
class MyComponent extends React.Component {
  state = { secret: "secret" };
}

// Attacker code
React.Component.prototype.__proto__ = new Proxy(React.Component.prototype.__proto__, new Proxy({}, {
  get(target, name) {
    return function (...args) {
      console.log(name, args);
      return Reflect[name](...args);
    };
  }
}));

// User code
<MyComponent />

// with [[Set]] it logs "set", MyComponent{}, "state", { secret: "secret" }

which can have potential security implications, but

1. It only works if the attacker has access to the class or to a super class
2. If you want your data to be hidden, you can use private fields. Note that with [[Set]] not leaking the class to an extraneous scope isn't enough.

[panlina]

I just took a day to look at this proposal yesterday. I have to say my vote goes to [[Set]]. This example is a good revelation of the problem of [[Define]] here.

[hax]

@rbuckton

I think your proposal could solve half of the problem. But still leave another half:

// base v1.0
class Base {
  constructor() {
    this.x = 1
  }
}

class Sub extends Base {
  x = 100; 
}

// base v1.1 refactor to getter/setter
class Base {
  get x() {...}
  set x(v) {...}
}

The essential issue is x = 100 syntax just implies [[Set]]. So we also need change the syntax (for example: <keyword> x = 100) to complement your proposal.

[littledan]

Chrome is shipping public fields with Define semantics in its beta branch. As I have explained in other threads, we don't have ecosystem consensus towards one or the other of these alternatives, and TC39 has considered this question and length and settled strongly on the Define side. I'm not sure what benefit we will get by continuing to discuss the issue.

[hax]

@littledan Chrome shipped it because it already stage 3. The problem is why it can forward to stage 3 when there are still several serious issues like this. It is said "TC39 have the consensus" on all issues, but obviously some never agree, or may change their mind later. So how to deal with such situation?

I'm not sure what benefit we will get by continuing to discuss the issue.

Yes. Without fixing process first, we are all wasting our time.

[littledan]

I think the way to address these situations in the future is to discuss the issues at Stage 2 and come to a conclusion before reaching Stage 3. See the explainer for a summary of the Set vs Define debate.

[mbrowne]

@littledan Thanks for the adding the summary to the explainer/readme. For stage 2 proposals in general, where is the possible schedule for advancement to stage 3 announced?

BTW, I doubt that most developers (including myself until recently) realize that stage 3 basically means all syntactic and semantic details are final and not open to reconsideration unless some very major and unforeseen problem crops up. Even with this description of what "Acceptance signifies" from https://tc39.github.io/process-document/:

The solution is complete and no further work is possible without implementation experience, significant usage and external feedback.

...it still sounds like if there were a lot of negative community feedback on a particular issue, that the design decision could be reconsidered even if someone had pointed out the issue already and the committee was aware of it at the time they moved the proposal to stage 3 (especially if the volume of feedback and the negative impact were much greater than anticipated). But what I'm hearing from you and other committee members is that only new and unforeseen issues would have any sway. I actually don't think that there will be some huge negative reaction on this particular issue (for one thing, it's an edge case) and maybe it will even be more positive than negative; I'm just describing a hypothetical situation here to understand the process.

[mbrowne]

P.S. At the very least, I think the process document should be updated to indicate that stage 3 means shipping in browsers and/or node.js (at least 2 implementations), not behind an experimental flag.

[littledan]

@mbrowne This is a really good point. We've discussed many process clarifications like that, but were unable to get consensus on them, unfortunately. We're working on improving our supplementary documentation on how the stage process works in practice, but this documentation is still under internal review and not released yet. I'll be happy to get your feedback on the document when it's released.