tc39 / proposal-decorators

Decorators for ES6 classes
https://arai-a.github.io/ecma262-compare/?pr=2417
2.76k stars 106 forks source link

Intercept class field get/set (without accessor on prototype) #530

Closed trusktr closed 5 months ago

trusktr commented 5 months ago

It would be nice to be able to hook into the get/set of field storage.

The context.access.get/set functions cannot be patched to allow intercepting set/get of a field. I think it would be nice to be able to do that if we provide get/set functions somehow.

Note, this is distinct from accessor properties, and this issue is not the same as

Ideas:

  1. Maybe if there's a way to provide get/set functions for fields, it will convert the value descriptor to a get/set descriptor directly on the instance.
  2. alternatively maybe this does not change the descriptor to a get/set descriptor and instead is a new type of feature that is accessible only via decorators and the descriptor remains as a value descriptor. This might be more work to spec, while idea 1 is may be easier because get/set descriptors already exist.

Would this still uphold the requirement that class shape be statically analyzable up front? Just like accessor properties, this would not expose the prototype and would not allow a decorator author to arbitrarily modify the shape of the class until after instantiation, similar to field initializers.

Possible API shape

What could it look like without breaking stage 3 design?

Currently:

type ClassFieldDecorator = (value: undefined, context: {
  kind: "field";
  name: string | symbol;
  access: { get(): unknown, set(value: unknown): void };
  static: boolean;
  private: boolean;
  addInitializer(initializer: () => void): void;
}) => (initialValue: unknown) => unknown | void;

New shape:

type ClassFieldDecorator = (value: undefined, context: {
  kind: "field";
  name: string | symbol;
  access: {
    get(): unknown,
    set(value: unknown): void,

    // This part is new
    setter?(value: unkown): void,
    getter?(): unknown
  };
  static: boolean;
  private: boolean;
  addInitializer(initializer: () => void): void;
}) => (initialValue: unknown) => unknown | void;

Example:

class Foo {
  @doubleAndLog foo = 1
}

function doubleAndLog(_, context) {
  context.access.setter = (value) => {
    console.log('doubled value:', value * 2)
    context.access.set(value * 2)
  }

  return (value) => {
    console.log('initial value:', value)
  }
}

const f = new Foo()
f.foo = 3
console.log('value:', f.foo)

Depending on the rules, the initial value would either

  1. Pass through the setter
  2. Or not

The output would either be (option 1):

initial value: 2
doubled value: 6
value: 6

Or (option 2):

initial value: 1
doubled value: 6
value: 6

This would also serve as an alternative to accessor without using a prototype getter/setter, and would allow reactivity without help from a class decorator:

class Foo {
  @ref foo = 123 // similar to Vue ref()
}

const f = new Foo

watchEffect(() => { // similar to Vue watchEffect
  console.log(f.foo) // this logs any time f.foo changes
})

(Implementation left to imagination)

mhofman commented 5 months ago

2. alternatively maybe this does not change the descriptor to a get/set descriptor and instead is a new type of feature that is accessible only via decorators and the descriptor remains as a value descriptor

That is definitely a non-starter. "data" properties with behavior is the realm of exotic objects. There definitely should be no more ways than Proxy to enable JS programs to have such exotic behavior.

  1. Maybe if there's a way to provide get/set functions for fields, it will convert the value descriptor to a get/set descriptor directly on the instance.

I'm confused, how is this different from an auto-accessor besides the implicit nature of the transformation?

pzuraq commented 5 months ago

This has been discussed previously and has been ultimately determined to be a major performance hazard. See this article for details about how important this is, prior to various class field optimizations both public and private class fields were a major regression from simple named properties: https://v8.dev/blog/faster-class-features

Beyond that, getters and setters on instances are not optimizable, so it would ultimately result in worse performance across the board for these types of fields.

I'm closing this issue since it has been addressed previously.

trusktr commented 5 months ago

There definitely should be no more ways than Proxy to enable JS programs to have such exotic behavior.

What's the reasoning for that?

We can already get and set the storage for a field, which is new behavior we've introduced. It isn't far-fetched to then be able to hook into that.

Proxy is by far more inconvenient, with various issues and foot guns, and extremely cumbersome to implement properly; most likely a Proxy would waste many hours of a developer's time compared to the feature ask besides potentially introducing multiple unexpected bugs down the road.

I would rather advocate for features that make development as easy as possible, and having to resort to Proxy is simply the opposite of that direction.

I'm confused, how is this different from an auto-accessor besides the implicit nature of the transformation?

Auto-accessors are on the class prototype (like getters/setters). These fields (with hooks for get/set) are on the instance. That's a main difference, and also aligns with the [[Define]] semantic of foo = 123.

trusktr commented 5 months ago

Beyond that, getters and setters on instances are not optimizable, so it would ultimately result in worse performance across the board for these types of fields.

Of course function calls add overhead. This argument is not substantive because we haven't determined how exactly the feature would work, and if that overhead trumps the gain in ergonomics.

Yes there's a perf difference, as with everything, but does it really matter? If it does, I would like to understand exactly how critical it is.

The ergonomic win from this is substantial:

Take this existing code:

class Foo {
  foo = 123
}

Being able to do the following,

import {signal} from 'library-for-solid-js'

class Foo {
  @signal foo = 123
}

is much better than having to change the code to this,

import {signal} from 'library-for-solid-js'

class Foo {
  @signal accessor foo = 123
}

because the change in semantic of the property (from instance field to prototype property) could easily cause runtime errors and require unwanted refactoring, and that refactoring could even be impossible if the code is from 3rd parties.

On top of that, I've extensively used instance getters/setters in my daily 3D WebGL centered work for years (my profession), and I have had no performance issues due to instance getters/setters. The performance implication getters/setters have is small in comparison to the vast majority of problems that actually do cause performance issues. I'm saying this confidently from my years experience using and making reactivity on instances.

If someone has a rare slow field due to a decorator (easily detectable in Performance tab) they can drop that decorator and do something else.

This feature would be simply awesome for ergonomics and for most use cases the micro-performance differences will not matter.

(Measure our apps, be aware of what takes time, use or don't use libraries including decorator libraries based on measurements, and please allow good DX when the performance difference is not significant.)

Nice site btw, love the pixel art!

mhofman commented 5 months ago

What's the reasoning for that?

These fields (with hooks for get/set) are on the instance.

I'm now confused as to what the request is.

When looking at the property descriptor of a non-exotic object, if it's a data property, the code can make assumption as to what accessing the property will do (aka it will not execute user code). Accessors are explicit ways of saying accessing the property will result in code execution.

The auto-accessor feature went the route of using accessors exactly because it was not introducing new exotic behavior.

Now if the request is to somehow install the accessors on instances instead of prototypes, since there is no exotic behavior there, I'm totally fine with it, but others may have objections. I honestly still don't understand what the concern is with installing the accessors on the prototype.

trusktr commented 5 months ago

I'm now confused as to what the request is.

Let me try to shorten it. We have a class like this:

class Flower {
  color = 'cornflowerblue'
}

where color is a class field whose descriptor is on the instance (because class fields [[Define]]).

Then for example I want a decorator that can log the field's value whenever it changes,

class Flower {
  @log color = 'cornflowerblue'
}

new Flower().color = "violet" // logs "violet"

I want the field to still be a field, on the instance (i.e. I don't want the property to live on the prototype, I don't want to add accessor keyword to my class because I do not want to move the property descriptor to the class prototype).

The ask is to be able to provide getter/setter functions for fields to be able to intercept when the field storage is written or read.

I don't want to add accessor to my class definition because this drastically changes the shape of the class in a dangerous way, potentially breaking code and requiring unwanted refactors, all just for a log.

I want the field to remain a field (on the instance), so that code is much more likely to work as-is after adding the decorator (the @log decorator should have no effect on any part of the program other than logging the value and otherwise passing the value through).

Just to summarize: converting an existing field to an accessor can be very unwanted, and can be too difficult, making @log nearly impossible to use in some cases.

When looking at the property descriptor of a non-exotic object, if it's a data property, the code can make assumption as to what accessing the property will do (aka it will not execute user code). Accessors are explicit ways of saying accessing the property will result in code execution.

I understand this. So, in your opinion, if this was allowed, should the descriptor be modified to have a get and set? I would be ok with that, because having the descriptor in the same place will greatly reduce code ordering problems that could easily be caused by switching to accessor. In this case, each successive decorator would receive the latest get and set functions of the descriptor, and have a chance to replace or wrap them.

I honestly still don't understand what the concern is with installing the accessors on the prototype.

Hopefully the above clarified why they could be very inconvenient. But if an example is needed, I'd be happy to come up with one, let me know.

ljharb commented 5 months ago

I want the field to still be a field, on the instance (i.e. I don't want the property to live on the prototype, I don't want to add accessor keyword to my class because I do not want to move the property descriptor to the class prototype).

can you elaborate on why?

trusktr commented 5 months ago

I made an example. This "program" (it is a simple one, but for sake of example) relies on own keys:

class Foo {
    foo = 123

    constructor() {
        alert(Object.keys(this))
    }
}

new Foo()

live example (hit Run)

Now if you change it to use accessor, the behavior of the program changes (unwanted):

class Foo {
    accessor foo = 123

    constructor() {
        alert(Object.keys(this))
    }
}

new Foo()

live example

If we wanted to add a @log decorator and needed to also add accessor to use it, the change in program behavior could translate into a bug in the application.

ljharb commented 5 months ago

Can you help me understand why anyone would want to do this, though? not just use alert, which has been strongly discouraged for decades, but to care about the own properties of an object instance - caring about own properties is for "dictionary-like" objects, and instances aren't dictionary-like.

trusktr commented 5 months ago

The class that we want to add @log to might pre-exist. Maybe we didn't write it. alert(Object.keys(this)) is just an example placeholder for arbitrary logic of any size.

Adding @log accessor to a class could simply break it. That's the point. Doesn't matter why. You could tell me not to use alert all day, or to avoid Object.keys(), etc, for whatever reason you may have, but it would make no difference to the main point.

Simply put, code re-ordering can break existing programs.

I don't want to be required to refactor someone else's code just to add @log to a field, for example.

ljharb commented 5 months ago

If you're adding a decorator to the class, you own it - who wrote it is irrelevant, and you're responsible for taking into account the choices made in that class, including whether they're good choices or not. I agree that blindly adding a decorator can break code - this is often the case - but that's part of the responsibility of owning code. It'd be the same thing with blindly making any changes.

mhofman commented 5 months ago

So, in your opinion, if this was allowed, should the descriptor be modified to have a get and set?

Yes, the field would have to become an accessor property with get and set. However I don't think such implicit modification would be appropriate, for the same reason the accessor modifier was introduced, and as such an other explicit syntactic opt-in would be necessary. I also doubt the motivation to avoid putting the accessor on the prototype would be sufficient for the committee.

trusktr commented 5 months ago

If you're adding a decorator to the class, you own it - who wrote it is irrelevant

Not when this happens:

import {indispensable3rdPartyFeature} from 'blackbox-3rd-party-library'

class MyClass {
  foo = 123
  constructor() {
    indispensable3rdPartyFeature(this)
  }
}

where blackbox-3rd-party-library is from a 3rd party and indispensable3rdPartyFeature is an inherently important part of our class that we cannot easily replace (and it may happen just so happen to use Object.keys(), etc).

Convert it to accessor foo = 123 and 3rd-party code could break.

trusktr commented 2 months ago

Can we please re-open this?

I believe there are quite a number of people who don't want to write accessor everywhere.

I've been using the class+field decorator combo a long while now, to write reactive 3D applications without having to use the accessor keyword, and I have not had a single performance issue from this.

I'm sure there are scenarios where the performance will get non-ideal, but its just not that common. Yesterday's cellphones probably run the slower version faster than yesterday's desktops run the faster version.

We're talking about micro-optimization here, for all intents and purposes, and practically speaking.

Please let us intercept get/set for fields if we want to. Let us choose when performance optimization is ok for own apps without restricting us to more verbose code.

ljharb commented 2 months ago

This proposal is stage 3 and unlikely to change without implementation feedback, and some implementations have repeatedly insisted on the constraints that require the accessor prefix. It's simply not up for further discussion.

trusktr commented 2 months ago

I believe this addition can be a non-breaking change, as it won't break anything already specified.

The requirement from implementers was that the class shape should not change during decoration. This proposal aligns with that because:

This would be useful.


In my daily work, I swap the descriptors of my class fields after decoration time by employing the class+field decorator workaround, and I have encountered no performance issues in the 3D WebGL applications I develop.

I actually use this workaround for all reactivity in my work, on a daily basis, without any problems!

pzuraq commented 2 months ago

@trusktr this issue is not going to be re-litigated. To be clear, here is the timeline of events that led to this design.

I have tried every way possible to make the accessor keyword unnecessary, but I have not been able to do that. In the end, there were exactly 3 ways this proposal was going to advance:

  1. With accessor as the way to turn a field into a getter/setter pair, and with the keyword as part of the decorator proposal.
  2. With accessor as a SEPARATE followup proposal that had no guarantee of being included.
  3. Without accessor and with NO way to decorate a field with a getter/setter at all.

This was the best option for folks who prefer the accessor keyword.

Lastly, I want to address this part of your comment:

In my daily work, I swap the descriptors of my class fields after decoration time by employing the class+field decorator workaround, and I have encountered no performance issues in the 3D WebGL applications I develop.

You are making the assumption that the speed you have now is the fasted it could possibly be. That is a faulty assumption, because engines have not fully implemented it and have not explored ways to optimize decoration. Again, look at the V8 class fields blog post for an example here.

The initial implementation AND the polyfills were not very performant, but after fields were fully implemented they were able to figure out ways to optimize them significantly. So, what you're seeing right now might not be much more or less performant, but they may be able to squeeze another 100x speedup out of it just like in that case. That could matter a lot for apps that care about p99 initial page performance on low-powered devices, and for complex apps at scale.

trusktr commented 2 months ago
  • stage 2 design was deemed too complex, too magical, and too full of performance hazards

I agree with that decision. Makes a lot of sense.

From a developer experience perspective, performance of legacy decorators was not (and with my workarounds is still not) a problem for apps, but the fact that people could arbitrarily change class shapes, in absolutely any way, from any decorator, made it a mine field for bugs and difficulties debugging people's code when composing things together.

That, was the real problem. Performance? Meh. Like I'm sure some people faced perf issues in limited cases. (Some!)

The new design is, from a developer experience perspective, much cleaner. The perf gain is welcome, but not critical, and I'm literally losing nothing from working around the use of accessor in 3D WebGL applications, which says a lot (I can give proof later, I'm building a showcase).

4. Exactly the proposal you have in this issue, where only fields could be converted to accessors by decorators.

That's unfortunate, but also easy to add in an add-on proposal. It would be very very welcome. I very much dislike performance optimization when it makes no difference.

Like, take for example Solid.js (I'm a contributor on the infrastructure team), in which all variables are getter/setter pairs.

You see it winning in benchmarks, and a lot faster than React and competitors. That's how it got its name out there.

Now consider how much slower React is, and the fact that people still swear by it.

The performance argument here is practically moot if we're talking about getters/setters costing too much. And not all class fields are going to be accessors to begin with! (All signals in Solid are accessors!)

  • you would have had to do @init:foo to add an extra initializer for the decorator as well.

Yeah, that was just bad. Glad we're past that.

We finally agreed to adding a keyword to the proposal to hint that to the engine that the class will change shape due to decoration

Is there writing somewhere that defines exactly what are the requirements for the shape to be considered changed?

For example, if adding accessor means the engine knows that the descriptor will be on the prototype instead of on each instance, well, then the proposal of this issue means the engine will clearly know that the descriptor will not move. And the engine will not even create the descriptor until after the initializer.

This means that, after decorators have executed, but before any instances are ever created, the engine already knows whether each instance will have a value descriptor or an accessor descriptor, and therefore it will be able to optimize the in-memory class shape before any instance is ever created.

I have tried every way possible to make the accessor keyword unnecessary, but I have not been able to do that.

How exactly does this thread (and ideas like @deco both) not make accessor unnecessary?

Perhaps a question is, why exactly does the engine need to know if a member will be a value vs an accessor before decorators run? (If you're considering that to be deemed a class shape change)

In the end, there were exactly 3 ways this proposal was going to advance:

  1. With accessor as the way to turn a field into a getter/setter pair, and with the keyword as part of the decorator proposal.
  2. With accessor as a SEPARATE followup proposal that had no guarantee of being included.
  3. Without accessor and with NO way to decorate a field with a getter/setter at all.

This was the best option for folks who prefer the accessor keyword.

I don't think it was required at all, as per the ideas in this issue and @deco both.

Even without these two ideas, it still wasn't required. We can still achieve everything without accessor! Show me something that can be done with accessor and I'll show you an alternative without accessor.

And in the event that I provide an alternative, if you're going to say "that's slower", well, then, you might be right, and it might just not matter. But! It would be a reason to have accessor as an addon, not an incomplete feature.

Having accessor as an add-on would allow us to get real actual data from the field after implementation release decorators, to get actual measurable performance results and reports from actual apps.

You are making the assumption that the speed you have now is the fasted it could possibly be.

Not at all! I know it is not the fastest. But I still have plenty of budget left.

In fact, I strictly assume my implementation is slower. I'm not making a faulty assumption here. I'm just accepting slower (but very negligible) speed for better DX.

On top of that, my workarounds (and accessor) will get faster once I use native decorators instead of transpiled code, for sure. So I'm in fine shape.

I welcome any perf increase, but it still makes no difference in my cases, my major issues arrise from GPU render calls and GPU buffer management, and other data issues unrelated to decorators.

pzuraq commented 2 months ago

To clarify, I am not making the performance argument. It was the browser teams that were concerned with the original designs, and they blocked the proposal moving forward. I made these arguments, just as you have, and they did not find them convincing.