qooxdoo / qooxdoo

qooxdoo - Universal JavaScript Framework
http://qooxdoo.org
Other
764 stars 259 forks source link

Compiler Bugs: `$$createdAt` and `_createQxObjectImpl` injections #10656

Closed WillsterJohnsonAtZenesis closed 2 months ago

WillsterJohnsonAtZenesis commented 3 months ago

The issues fixed are best described visually.

Take the following class, which contains an anonymous class and a new expression to construct it.

qx.Class.define("demo.Application", {
  extend: qx.application.Standalone,
  members: {
    fn() {
      const anonCtor = qx.Class.define(null, {
        extend: qx.core.Object,
        objects: {
          example() {
            return "example";
          },
        },
      });
      const anonInstance = new anonCtor();
      return anonInstance;
    },
  },
});

A few things are expected to happen within this class' body at compile time.

  1. the call to new anonCtor() should be wrapped in qx.$$createdAt (assuming the compiler option is set)
  2. within both this class and the inner anonymous class, the _createQxObjectImpl method should be ensured, with necessary calls to qx.core.MObjectId.handleObjects to process the top level objects.

Omitting some parts we don't care about here (and formatting the code tightly), this is the compiled output;

qx.Class.define("demo.Application", {
  extend: qx.application.Standalone,
  members: {
    fn() {
      const anonCtor = qx.Class.define(null, {
        extend: qx.core.Object,
        objects: {
          example() { return "example"; }
        },
        members: {
          _createQxObjectImpl() {
            // !! ISSUE #1: the first argument is undefined. Though intended behavior, this means we cannot use top-level objects on anonymous classes.
            { let object = qx.core.MObjectId.handleObjects(undefined, this, ...arguments);
              if (object) { return object; } }
            // !! ISSUE #2: This code is injected twice - that's two times more than is needed as the return injection covers this case
            { let object = qx.core.MObjectId.handleObjects(undefined, this, ...arguments);
              if (object) { return object; } }
            // !! ISSUE #3: The first argument is incorrect
            return qx.core.MObjectId.handleObjects(demo.Application, this, ...arguments) ?? qx.core.Object.prototype._createQxObjectImpl.call(this, ...arguments);
          }
        }
      });
      // !! ISSUE #4: The new expression is wrapped in `qx.$$createdAt` twice
      const anonInstance = qx.$$createdAt(qx.$$createdAt(new anonCtor(), "demo/Application.js", 14, 27), "demo/Application.js", 14, 27);
      return anonInstance;
    },
    _createQxObjectImpl() {
      // !! ISSUE #2: injected 1 more time than needed
      { let object = qx.core.MObjectId.handleObjects(demo.Application, this, ...arguments);
        if (object) { return object; } }
      return qx.core.MObjectId.handleObjects(demo.Application, this, ...arguments) ?? demo.Application.superclass.prototype._createQxObjectImpl.call(this, ...arguments);
    }
  }
});

After the few small tweaks to the implementation of top-level objects, the result is as intended, and there's even limited support for tep-level objects in anonymous classes.

qx.Class.define("demo.Application", {
  extend: qx.application.Standalone,
  members: {
    fn() {
      const anonCtor = qx.Class.define(null, {
        extend: qx.core.Object,
        objects: {
          example() { return "example"; }
        },
        members: {
          _createQxObjectImpl() {
            return qx.core.MObjectId.handleObjects(this.constructor, this, ...arguments) ?? qx.core.Object.prototype._createQxObjectImpl.call(this, ...arguments);
          }
        }
      });
      const anonInstance = qx.$$createdAt(new anonCtor(), "demo/Application.js", 14, 27);
      return anonInstance;
    },
    _createQxObjectImpl() {
      return qx.core.MObjectId.handleObjects(demo.Application, this, ...arguments) ?? demo.Application.superclass.prototype._createQxObjectImpl.call(this, ...arguments);
    }
  }
});
hkollmann commented 3 months ago

A lot of tests are not working any longer ....

goldim commented 2 months ago

@WillsterJohnsonAtZenesis @johnspackman Is it fine to suppress tests instead of fixing problem? Or these tests/asserts no more needed? Then they have to be removed then.

WillsterJohnsonAtZenesis commented 2 months ago

@goldim the test cases that were changed are just reverting to what they were before 50ae4c5 (#10645), they were only changed then to 'cover up' an implementation quirk that is no longer there.