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

Delete Reflect.deleteField #11543

Open
Simn opened this issue Feb 4, 2024 · 1 comment
Open

Delete Reflect.deleteField #11543

Simn opened this issue Feb 4, 2024 · 1 comment

Comments

@Simn
Copy link
Member

Simn commented Feb 4, 2024

I would like to discuss the removal of this particular function. My main problem with it is that it penalizes targets that generate anonymous structure in an optimized way:

  1. Targets that basically treat anonymous objects like glorified StringMap<Dynamic> can make great use of deleteField, because deleting a field means removing an entry from that table.
  2. Targets that create a wrapper type and store the values in some collection can delete fields somewhat easily by updating both the collection and the lookup method.
  3. Targets which generate real classes with real fields cannot actually delete such a field value. The best they can do is hide these fields in the API, but the field will never truly be deleted.

As an example, the following code doesn't work as expected on the JVM target:

function main() {
	var a = {foo: 12, bar: 13};
	Reflect.deleteField(a, "foo");
	a.foo = 14;
	trace(a); // {bar: 13}
	trace(Reflect.hasField(a, "foo")); // false
}

The reason is that the a.foo = 14 is a real field write without any nonsense in the generated code:

        if (a instanceof Anon0) {
            ((Anon0)a).foo = 14;
        } else {
            Jvm.writeField(a, "foo", 14);
        }

This is why anonymous objects on the JVM target are fast, and it's compatible with the entire Haxe reflection API except for deleteField. The slower path through Jvm.writeField does work as expected, but even there we have to make really awkward checks to support it:

    public void _hx_setField(String name, Object value) {
        switch(name.hashCode()) {
        case 97299:
            this.bar = Jvm.toInt(value);
            if (this._hx_deletedAField != null) {
                super._hx_setField(name, value);
            }
            break;
        case 101574:
            this.foo = Jvm.toInt(value);
            if (this._hx_deletedAField != null) {
                super._hx_setField(name, value);
            }
            break;
        default:
            super._hx_setField(name, value);
        }

    }

The only reason these this._hx_deletedAField != null operations exist is to support deleteField. Needless to say, this implies that every anonymous object on this target carries an additional public var _hx_deletedAField:Null<Bool> only to support deleteField.

I'd like to think that this API is from a time when we treated anonymous objects like lower-class citizens and were fine with stating that their performance sucks anyway. I don't know how much code relies on the capability to physically delete a field from objects, but my hope is that there's not too much beside the unit tests.

My deprecation plan here would be to keep deleteField, but simply make it call setField(obj, field, null).

So let me hear why this function is mega-important...

@Simn Simn added the discussion label Feb 4, 2024
@kLabz
Copy link
Contributor

kLabz commented Feb 4, 2024

My deprecation plan here would be to keep deleteField, but simply make it call setField(obj, field, null).

On js target we likely want to keep current behavior, with a deprecation warning asking to call a js-specific API (in js.Lib for example) added for when we actually remove Reflect.deleteField.

I'm not sure it's used that much on other targets

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

No branches or pull requests

2 participants