haxe icon indicating copy to clipboard operation
haxe copied to clipboard

Delete Reflect.deleteField

Open Simn opened this issue 1 year ago • 1 comments

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 avatar Feb 04 '24 06:02 Simn

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

kLabz avatar Feb 04 '24 07:02 kLabz