jsii kernel: method lookup logic is extremely sus
- Dominant language
- TypeScript
- Stars
- 2.9k
- Forks
- 267
- Avg merge
- 1d 25m
- Merged PRs (30d)
- 14
Description
### Describe the bug
I'm pretty sure the effect of this bug will be:
* The wrong method gets called when a JavaScript method exists both on a class, and it's also `defineProperty`'ed directly on the instance (or simply assigned), and it gets called form Java.
* The wrong method gets called when two methods are overridden on a Java subclass inheriting from a JavaScript subclass, and a `this.theOtherMethod()` call is done (calls the JavaScript base method instead of the Java overridden one).
In both cases this means: behavior will be different between Java and JavaScript in hard-to-debug ways.
Full explanation from a PR. See https://github.com/aws/jsii/blame/main/packages/@jsii/kernel/src/kernel.ts#L980
```ts
// always first look up the method in the prototype. this practically bypasses
// any methods overridden by derived classes (which are by definition native
// methods). this serves to allow native call to invoke "super.method()" when
// overriding the method.
// if we didn't find the method on the prototype, it could be a literal object
// that implements an interface, so we look if we have the method on the object
// itself. if we do, we invoke it.
//
//--------------------------------------------------------------
//
// huijbers@ (2026) -- I'm pretty sure the above logic is wrong. It reads like
// looking up methods from the prototype is intended to prevent cyclic calls
// when subclassing JS classes from a jsii language. We don't want:
//
// ```java
// class MyClass extends JavaScriptClass {
// public void myMethod() {
// super.myMethod(); <-- should call myMethod on base class
// }
// }
// ```
//
// To call the same `myMethod` again and infinitely recurse, which a naive `invoke(this, 'myMethod')`
// would do. In order to work around this we seem to be default-ignoring functions
// that live directly on an object, *unless* we otherwise can't find it on the class.
//
// But `#findInvokeTarget()` is used for *all* invokes, and this now finds the wrong
// method in situations where both a class and the instance have a method, for whatever
// reason; maybe the JS object got patched or something.
//
// At least one case where I ran into this is when calling `toString()` on an anonymous
// object. Because 'Object.prototype' already has an implementation for `toString`, we
// never call the one on the anonymous object but always the built-in one which returns
// "[object Object]".
//
// I'm tempted to reverse the logic: look up the method on the instance, *unless* we
// have reasons to think the object we're looking at is a proxy for a jsii-client object
// in which case we look up on the parent. But even that is wrong because it would also
// do the wrong thing in this case:
//
// ```java
// class MyClass extends JavaScriptClass {
// @Override public void myMethod1() {
// this.myMethod2(); <-- should call myMethod2 below, not from parent
// }
// @Override public void myMethod2() {
// }
// }
// ```
//
// Pretty sure this is wrong and needs attention, but for now I'll just make an exception
// for the one case I need to get working: `toString`.
let fn = instance.constructor.prototype[methodName];
if (!fn) {
fn = instance[methodName];
if (!fn) {
throw new JsiiFault(`Cannot find ${methodName} on object`);
}
}
return { ti, obj: instance, fn };
```
Contributor guide
Research direction
Start in packages/@jsii/kernel/src/kernel.ts around #findInvokeTarget() and the lookup logic near line 980. Read the surrounding comments and trace the described class-versus-instance and Java-subclass cases. Done means the dispatch behavior is consistent for overridden methods, instance-defined methods, and anonymous-object toString calls without introducing recursive super calls.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java, javascript, typescript
- Domain
- developer-experience
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100