Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

static accessor doesn't work for subclasses #468

Closed
hax opened this issue Apr 30, 2022 · 17 comments
Closed

static accessor doesn't work for subclasses #468

hax opened this issue Apr 30, 2022 · 17 comments

Comments

@hax
Copy link
Member

hax commented Apr 30, 2022

class A {
  static accessor x = 42
}
class B extends A {}

A.x // 42
B.x // throw!

It's not a new problem, actually it's the unfortunate consequence of class static private fields design, but maybe static accessor modifiers could at least mitigate the issue by converting static accessor to something like:

class A {
  static #x = 42
  static get x() { return A.#x }
  static set x(v) { A.#x = v }
}
@senocular
Copy link
Contributor

Could that cause confusion if A and B are using the same backing data? The expectation might be that it match usage of having a public #x

class A {
  static _x = 42
  static get x() { return this._x }
  static set x(v) { this._x = v }
}
class B extends A {}

A.x = 1
B.x = 2
console.log(A.x) // 1
console.log(B.x) // 2

Maybe an error is better than going down a different path here?

I believe similar errors also already exist in the language. Maybe not the best example but one off the top of my head:

Object.create(new Number()).valueOf() // Error

The [[NumberData]] is not installed on the created object itself so valueOf() fails with an error.

If accessor is specifically tied to private fields by design, the error should be the expectation. If accessor is bound by some other magic, I think you could probably get away with using something like A.#x

@Jamesernator
Copy link

The A.#x behaviour definitely seems more useful as it's more like what people would intend to write if they expanded accessor by hand.

@trusktr
Copy link
Contributor

trusktr commented May 17, 2022

It would be strange that a modifying a subclass property changes the parent class property too.

With regular (non-static) properties, people don't write accessors that modify properties on the class prototype object, they write accessors that modify this. The A.#x trick would be as if we wrote the following non-static accessor:

class Foo {
  get foo() {
    return Foo.prototype._foo
  }
  set foo(v) {
    Foo.prototype._foo = v
  }
}

and that is strange: people don't expect modifying one instance modifies all instances. It is weird.

Throwing an error is also weird and strange, but at least prevents an issue. It is so weird.


If private fields had simple (actual) WeakMap semantics, as in foo(obj) {obj.#foo = 123} being simple sugar for foo(obj) {foo.set(obj, 123)}, then this issue wouldn't exist (plus private fields would be more useful); inherited static accessors would work very simply.

@Jamesernator
Copy link

Jamesernator commented May 18, 2022

The A.#x trick would be as if we wrote the following non-static accessor:

Kind've, but there's already the divergence that for instances #x is an own-"property" whereas getters/setters are on the prototype. Whereas for static members, all members are own-"properties", i.e. #x and get x() are on the same object for static member, whereas for instance methods #x is per-instance and get x() is on the prototype.

Given these existing differences, I don't think A.#x would be a particularly suprising choice of behaviour given that static members don't have a separate prototype and instance value.

@senocular
Copy link
Contributor

Whereas for static members, all members are own-"properties", i.e. #x and get x() are on the same object for static member, whereas for instance methods #x is per-instance and get x() is on the prototype.

Not all static members are own properties. My previous example is demonstrating that B inherits its get x() from A.

A.hasOwnProperty('x') // true
B.hasOwnProperty('x') // false

@trusktr's example is accurate for how A.#x would work in static getters. B would inherit get x() from A but then be setting A.#x, rather than its own #x .

In fact the public member version using a public _x in place of #x is flawed too, and not for dissimilar reasons to why a private version doesn't work. Because the backing field is not assigned to the subclass until the subclass sets its own value, it will inherit the base class's value of that field. So if the base class sets first, it would then be reflected in the sub.

A.x = 1
// B.x = 2
console.log(A.x) // 1
console.log(B.x) // 1

The difference with private members is that it's an error if the private member doesn't already exist as an own member, which it wouldn't for B because there's no super constructor-like step to install A's private members on B as there is for class instances.

@Jamesernator
Copy link

Jamesernator commented May 19, 2022

Not all static members are own properties. My previous example is demonstrating that B inherits its get x() from A.

I was meaning that within a single definition of a class:

class A {
    #x; // A instance
    get x(); // A.prototype
    
    static #x; // A
    static get x(); // A
}

i.e. For static members, placement is always the same object, but for non-static members placement may be either prototype or instance objects.

So having different behaviour for accessor might not be all that surprising given placement behaviour is already different (and people need to be aware of that when using static #field themselves already).

With regular (non-static) properties, people don't write accessors that modify properties on the class prototype object, they write accessors that modify this. The A.#x trick would be as if we wrote the following non-static accessor:

@trusktr's example is accurate for how A.#x would work in static getters. B would inherit get x() from A but then be setting A.#x, rather than its own #x .

Yes, this is how private fields already work for static fields though, like if people write static method() { this.#x = ... } it won't work today with subclassing.

Just as an example use case for static accessors, consider something like:

const DEFAULT_WORKER_POOL_SIZE = navigator.hardwareConcurrency;

class WorkerPool {
    #defaultWorkerPoolSize = DEFAULT_WORKER_POOL_SIZE;
    
    static get defaultPoolSize(): number {
        return WorkerPool.#defaultWorkerPoolSize;
    }
    
    static set defaultPoolSize(newSize: number): number {
        if (newSize <= 0) {
            throw new RangeError(`poolSize must be at least one`);
        }
        WorkerPool.#defaultPoolSize = newSize;
    }
    
    // instance stuff
    readonly #pool = new Set<Worker>();
    
    constructor(script: string | URL, {
        poolSize = WorkerPool.#defaultPoolSize,
    }: { poolSize?: number }={}) {
        for (let i = 0; i < WorkerPool.#defaultPoolSize) {
            this.#pool.add(new Worker(script));
        }
    }
    
    runTask(taskName: string): Promise<void> {
        // pick available worker somehow, etc and send it the taskName to perform
    }
}

// Configure all worker pools to use only half of cpus by default
WorkerPool.defaultPoolSize = Math.ceil(navigator.hardwareConcurrency / 2);

In general the idea here is that yes the property would indeed apply to subclasses as well. If subclasses really wanted different behaviour, well that's what overriding get/set defaultPoolSize is for (and customizing their own constructor).

The addition of accessor doesn't really change this pattern, like with accessor the main thing I'd be imagining is like:

class WorkerPool {
    @ensurePositiveNumber // Wrapper for if set value <= 0
    accessor defaultPoolSize;
}

@Jamesernator
Copy link

Jamesernator commented May 19, 2022

Although I suppose there is the case of wanting to be able to SubclassWorkerPool.defaultPoolSize = ..., however this would basically need private field inheritance, as you still need SubclassWorkerPool.defaultPoolSize getter to work without a set value.

i.e.:

class WorkerPool {
    static #defaultPoolSize = 12;
    
    static get defaultPoolSize() {
        return this.#defaultPoolSize;
    }
    
    static set defaultPoolSize(newSize: number) {
        this.#defaultPoolSize = newSize;
    }
}

class SubWorkerPool extends WorkerPool {}

// Still should return 12 as we've done nothing it
SubWorkerPool.defaultPoolSize;

// Would need to install new field on SubWorkerPool to avoid inheriting
SubWorkerPool.defaultPoolSize = 99;

This wouldn't work with simple weakmap semantics because if SubWorkerPool.#defaultPoolSize were the same as defaultPoolSizeWeakMap.get(defaultPoolSize) there would be no entry in the weakmap for SubWorkerPool and it'd just be undefined.

@trusktr
Copy link
Contributor

trusktr commented May 19, 2022

Given these existing differences, I don't think A.#x would be a particularly suprising choice of behaviour given that static members don't have a separate prototype and instance value.

This may be true if someone knows about JS specifics, but perhaps not what people from arbitrary languages with less background (also considering that class syntax tends to make prototypes less obvious) may expect. Not sure what a solution is though.


What if the semantics were like the following?

const defaultPoolSize = new WeakMap

class WorkerPool {    
    static get defaultPoolSize() {
        return defaultPoolSize.get(this);
    }
    
    static set defaultPoolSize(newSize: number) {
        defaultPoolSize.set(this, newSize);
    }
}
defaultPoolSize.set(WorkerPool, 12)

class SubWorkerPool extends WorkerPool {}
defaultPoolSize.set(SubWorkerPool, 12) // initialized during extension, but only accessible in WorkerPool scope, similar to a super() call for instances.

// Still should return 12 as we've done nothing it
SubWorkerPool.defaultPoolSize; // returns 12

// field is already installed, is set to new value
SubWorkerPool.defaultPoolSize = 99;

I guess the engine would carry around a super() like semantic anywhere that extends is used. Since the user already has the reference to the BaseClass, the engine can also have the initialization code to run.

But the private fields ship sailed already. Can a rescue mission be sent out for it?

@hax
Copy link
Member Author

hax commented May 23, 2022

Could we consider some way to allow subclasses "inherit" special static initializers? Is that the desired semantic in static accessor use cases?

@hax
Copy link
Member Author

hax commented May 23, 2022

@trusktr @senocular

I guess this may work as expect:

class A {
  static accessor x = 42
}
class B extends A {}

->

function _Private() {
  const store = new WeakMap()
  return {
    init(o, v) {
      if (store.has(o)) throw new TypeError()
      store.set(o, v)
    },
    get(o) {
      if (!store.has(o)) throw new TypeError()
      return store.get(o)
    },
    set(o, v) {
      if (!store.has(o)) throw new TypeError()
      store.set(o, v)
    },
  }
}

const _static = _Private()

const _x = _Private()

class A {
  static #init() { _x.init(this, 42) }
  static get x() { return _x.get(this) }
  static set x(v) { _x.set(this, v) }
  static {
    _static.init(this, this.#init)
    this.#init()
  }
}

class B extends A {
  static #init() { _static.get(A).call(this) }
  static {
    _static.init(this, this.#init)
    this.#init()
  }
}

@Jamesernator
Copy link

Jamesernator commented May 24, 2022

I guess this may work as expect:

Yes, although it it a fairly new powerful capability that doesn't currently exist, specifically it would allow interception of subclasses during their creation i.e.:

class A {
     accessor x = function() {
         if (this !== A) {
              // "this" is a subclass and we can manipulate it
         }
     }.call(this);
}

// Currently at most A.[[Get]]("prototype") is observable from this statement
// by A
class B extends A {}

At present, the best you can do is guess that a subclass is being created because A.[[Get]](prototype) is called (and even then you need to use a proxy to observe it).

It seems like a very powerful capability to attach to something like accessor. Although it does beg the question as to whether or not a more explicit static-constructor feature would be desirable:

class A {
     // some way to declare a field should be installed on subclasses as well
     static inherited #field;

     // technically "static constructor" is already valid syntax, so some other
     // syntax would need to be chosen
     static constructor() {
         console.log(this); // whatever subclass is being created
         this.#field = "someValue";
     }
}

@hax
Copy link
Member Author

hax commented May 28, 2022

@Jamesernator

Yeah, I think we eventually need static constructor, and it seems the expected static accessor semantic rely on it.

About the capability, base class already could collect subclasses via new.target, so I think it's not a very new capability, though the timing of first access is much earlier.

@trusktr
Copy link
Contributor

trusktr commented Sep 12, 2022

Yeah, I think we eventually need static constructor, and it seems the expected static accessor semantic rely on it.

How would the code look like?

@pzuraq
Copy link
Collaborator

pzuraq commented Nov 25, 2022

I feel like I lean toward the original proposed solution here:

class A {
  static #x = 42
  static get x() { return A.#x }
  static set x(v) { A.#x = v }
}

It would definitely be confusing that static accessors could not be inherited at all, and I think that folks who use static private fields would probably use this pattern pretty often when they realized inheritance doesn't work.

@Jamesernator
Copy link

I feel like I lean toward the original proposed solution here:

Is this a planned change? If so it's probably worth bringing it up in the March agenda as implementations are starting to happen.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 6, 2023

It is, thank you for bringing it up again. I'm going to be bringing this and a number of other normative changes to the upcoming plenary for consensus.

@pzuraq
Copy link
Collaborator

pzuraq commented Apr 12, 2023

In further examination of the spec and thinking this through, I actually believe this is already the way the spec text works. When we create the ClassElementDefinition of static values, we pass the class itself as the homeObject for those elements:

          1. Else, let _result_ be Completion(ClassElementEvaluation of _e_ with argument _F_).

This becomes the this value for accessors, so I think it should be the correct value and be hardcoded, since static accessors are defined only on the superclass and not subclasses. This could be a transpiler error if we're seeing this behavior in the wild.

Going to close this issue for now as fixed, if anyone disagrees and the spec is incorrect, we do have consensus that this should be the behavior, so we will update it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants