Skip to content

Commit d6a243b

Browse files
rix0rrrrix0rrr
authored
fix(jsii-pacmak): toString() on anonymous classes does not call JavaScript implementation (#5027)
In recent changes, we dropped as many methods from `Jsii$Proxy` as possible. This includes the implementation of `toString()` from `Jsii$Proxy`, which means that `toString()` falls back to the Java implementation instead of the JavaScript one. ```java @jsii(module = $Module.class, fqn = "aws-cdk-lib.IResolvable") @Proxy(IResolvable.Jsii$Proxy.class) public interface IResolvable extends JsiiSerializable { List<String> getCreationStack(); ResolutionTypeHint getTypeHint(); Object resolve(@NotNull IResolveContext paramIResolveContext); String toString(); // <---- ⚠️ note this! public static interface Jsii$Default extends IResolvable { @NotNull default List<String> getCreationStack() { // ... } @nullable default ResolutionTypeHint getTypeHint() { // ... } @NotNull default Object resolve(@NotNull IResolveContext context) { // ... } // <--- ❌ but missing here! } } ``` This doesn't fail because only there is there also a default `toString()` implementation on all objects in Java. There was no problem when we called `toString()` on a proxy of a public JSII class, but in case we were using a pure interface proxy (when the concrete class was private or even anonymous), we wouldn't forward to the JavaScript-side `toString()` but call the default Java implementation instead. Fix that and add a test for this behavior. --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
1 parent 20111a6 commit d6a243b

File tree

12 files changed

+1095
-106
lines changed

12 files changed

+1095
-106
lines changed

‎packages/@jsii/java-runtime-test/project/src/test/java/software/amazon/jsii/testing/TypeCheckingTest.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,4 +135,25 @@ public void staticMethod()
135135
});
136136
assertEquals("Expected struct to be one of: software.amazon.jsii.tests.calculator.StructA, software.amazon.jsii.tests.calculator.StructB; received class java.lang.String", e.getMessage());
137137
}
138+
139+
@Test
140+
public void canStringifyPublicClassViaInterface() {
141+
IStringable s = Stringable.makePublicStringable();
142+
assertEquals(s.toString(), s.nativeToString());
143+
assertEquals(s.describe(), s.toString());
144+
}
145+
146+
@Test
147+
public void canStringifyPrivateClassViaInterface() {
148+
IStringable s = Stringable.makePrivateStringable();
149+
assertEquals(s.toString(), s.nativeToString());
150+
assertEquals(s.describe(), s.toString());
151+
}
152+
153+
@Test
154+
public void canStringifyAnonymousClassViaInterface() {
155+
IStringable s = Stringable.makeAnonymousStringable();
156+
assertEquals(s.toString(), s.nativeToString());
157+
assertEquals(s.describe(), s.toString());
158+
}
138159
}

‎packages/@jsii/kernel/src/kernel.ts‎

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -984,12 +984,60 @@ export class Kernel {
984984
// if we didn't find the method on the prototype, it could be a literal object
985985
// that implements an interface, so we look if we have the method on the object
986986
// itself. if we do, we invoke it.
987-
let fn = instance.constructor.prototype[methodName];
987+
//
988+
//--------------------------------------------------------------
989+
//
990+
// huijbers@ (2026) -- I'm pretty sure the above logic is wrong. It reads like
991+
// looking up methods from the prototype is intended to prevent cyclic calls
992+
// when subclassing JS classes from a jsii language. We don't want:
993+
//
994+
// ```java
995+
// class MyClass extends JavaScriptClass {
996+
// public void myMethod() {
997+
// super.myMethod(); <-- should call myMethod on base class
998+
// }
999+
// }
1000+
// ```
1001+
//
1002+
// To call the same `myMethod` again and infinitely recurse, which a naive `invoke(this, 'myMethod')`
1003+
// would do. In order to work around this we seem to be default-ignoring functions
1004+
// that live directly on an object, *unless* we otherwise can't find it on the class.
1005+
//
1006+
// But `#findInvokeTarget()` is used for *all* invokes, and this now finds the wrong
1007+
// method in situations where both a class and the instance have a method, for whatever
1008+
// reason; maybe the JS object got patched or something.
1009+
//
1010+
// At least one case where I ran into this is when calling `toString()` on an anonymous
1011+
// object. Because 'Object.prototype' already has an implementation for `toString`, we
1012+
// never call the one on the anonymous object but always the built-in one which returns
1013+
// "[object Object]".
1014+
//
1015+
// I'm tempted to reverse the logic: look up the method on the instance, *unless* we
1016+
// have reasons to think the object we're looking at is a proxy for a jsii-client object
1017+
// in which case we look up on the parent. But even that is wrong because it would also
1018+
// do the wrong thing in this case:
1019+
//
1020+
// ```java
1021+
// class MyClass extends JavaScriptClass {
1022+
// @Override public void myMethod1() {
1023+
// this.myMethod2(); <-- should call myMethod2 below, not from parent
1024+
// }
1025+
// @Override public void myMethod2() {
1026+
// }
1027+
// }
1028+
// ```
1029+
//
1030+
// Pretty sure this is wrong and needs attention, but for now I'll just make an exception
1031+
// for the one case I need to get working: `toString`.
1032+
let fn: any;
1033+
// Prototype first, but only if the method is not 'toString'
1034+
if (methodName !== 'toString') {
1035+
fn = instance.constructor.prototype[methodName];
1036+
}
1037+
// Then instance
1038+
fn ??= instance[methodName];
9881039
if (!fn) {
989-
fn = instance[methodName];
990-
if (!fn) {
991-
throw new JsiiFault(`Cannot find ${methodName} on object`);
992-
}
1040+
throw new JsiiFault(`Cannot find ${methodName} on object`);
9931041
}
9941042
return { ti, obj: instance, fn };
9951043
}

‎packages/jsii-calc/lib/compliance.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3173,3 +3173,67 @@ export class AnyPropertyAccess {
31733173

31743174
private constructor() {}
31753175
}
3176+
3177+
export interface IStringable {
3178+
describe(): string;
3179+
3180+
// Calling toString from JS should produce the same value as calling toString from Java/C#/Python/...
3181+
nativeToString(): string;
3182+
3183+
toString(): string;
3184+
}
3185+
3186+
export class Stringable implements IStringable {
3187+
private readonly stringValue = 'string value produced in JS';
3188+
3189+
public static makePublicStringable(): IStringable {
3190+
return new Stringable();
3191+
}
3192+
3193+
public static makePrivateStringable(): IStringable {
3194+
return new PrivateStringable();
3195+
}
3196+
3197+
public static makeAnonymousStringable(): IStringable {
3198+
const stringValue = 'string value produced in JS';
3199+
return {
3200+
describe() {
3201+
return stringValue;
3202+
},
3203+
toString() {
3204+
return stringValue;
3205+
},
3206+
nativeToString() {
3207+
return this.toString();
3208+
},
3209+
};
3210+
}
3211+
3212+
public describe(): string {
3213+
return this.stringValue;
3214+
}
3215+
3216+
public toString(): string {
3217+
return this.stringValue;
3218+
}
3219+
3220+
public nativeToString(): string {
3221+
return this.toString();
3222+
}
3223+
}
3224+
3225+
class PrivateStringable implements IStringable {
3226+
private readonly stringValue = 'string value produced in JS';
3227+
3228+
public describe(): string {
3229+
return this.stringValue;
3230+
}
3231+
3232+
public toString(): string {
3233+
return this.stringValue;
3234+
}
3235+
3236+
public nativeToString(): string {
3237+
return this.toString();
3238+
}
3239+
}

‎packages/jsii-calc/test/assembly.jsii‎

Lines changed: 185 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8355,6 +8355,70 @@
83558355
],
83568356
"symbolId": "lib/stability:IStableInterface"
83578357
},
8358+
"jsii-calc.IStringable": {
8359+
"assembly": "jsii-calc",
8360+
"docs": {
8361+
"stability": "stable"
8362+
},
8363+
"fqn": "jsii-calc.IStringable",
8364+
"kind": "interface",
8365+
"locationInModule": {
8366+
"filename": "lib/compliance.ts",
8367+
"line": 3177
8368+
},
8369+
"methods": [
8370+
{
8371+
"abstract": true,
8372+
"docs": {
8373+
"stability": "stable"
8374+
},
8375+
"locationInModule": {
8376+
"filename": "lib/compliance.ts",
8377+
"line": 3178
8378+
},
8379+
"name": "describe",
8380+
"returns": {
8381+
"type": {
8382+
"primitive": "string"
8383+
}
8384+
}
8385+
},
8386+
{
8387+
"abstract": true,
8388+
"docs": {
8389+
"stability": "stable"
8390+
},
8391+
"locationInModule": {
8392+
"filename": "lib/compliance.ts",
8393+
"line": 3181
8394+
},
8395+
"name": "nativeToString",
8396+
"returns": {
8397+
"type": {
8398+
"primitive": "string"
8399+
}
8400+
}
8401+
},
8402+
{
8403+
"abstract": true,
8404+
"docs": {
8405+
"stability": "stable"
8406+
},
8407+
"locationInModule": {
8408+
"filename": "lib/compliance.ts",
8409+
"line": 3183
8410+
},
8411+
"name": "toString",
8412+
"returns": {
8413+
"type": {
8414+
"primitive": "string"
8415+
}
8416+
}
8417+
}
8418+
],
8419+
"name": "IStringable",
8420+
"symbolId": "lib/compliance:IStringable"
8421+
},
83588422
"jsii-calc.IStructReturningDelegate": {
83598423
"assembly": "jsii-calc",
83608424
"docs": {
@@ -13990,6 +14054,126 @@
1399014054
"name": "StringEnum",
1399114055
"symbolId": "lib/compliance:StringEnum"
1399214056
},
14057+
"jsii-calc.Stringable": {
14058+
"assembly": "jsii-calc",
14059+
"docs": {
14060+
"stability": "stable"
14061+
},
14062+
"fqn": "jsii-calc.Stringable",
14063+
"initializer": {
14064+
"docs": {
14065+
"stability": "stable"
14066+
}
14067+
},
14068+
"interfaces": [
14069+
"jsii-calc.IStringable"
14070+
],
14071+
"kind": "class",
14072+
"locationInModule": {
14073+
"filename": "lib/compliance.ts",
14074+
"line": 3186
14075+
},
14076+
"methods": [
14077+
{
14078+
"docs": {
14079+
"stability": "stable"
14080+
},
14081+
"locationInModule": {
14082+
"filename": "lib/compliance.ts",
14083+
"line": 3197
14084+
},
14085+
"name": "makeAnonymousStringable",
14086+
"returns": {
14087+
"type": {
14088+
"fqn": "jsii-calc.IStringable"
14089+
}
14090+
},
14091+
"static": true
14092+
},
14093+
{
14094+
"docs": {
14095+
"stability": "stable"
14096+
},
14097+
"locationInModule": {
14098+
"filename": "lib/compliance.ts",
14099+
"line": 3193
14100+
},
14101+
"name": "makePrivateStringable",
14102+
"returns": {
14103+
"type": {
14104+
"fqn": "jsii-calc.IStringable"
14105+
}
14106+
},
14107+
"static": true
14108+
},
14109+
{
14110+
"docs": {
14111+
"stability": "stable"
14112+
},
14113+
"locationInModule": {
14114+
"filename": "lib/compliance.ts",
14115+
"line": 3189
14116+
},
14117+
"name": "makePublicStringable",
14118+
"returns": {
14119+
"type": {
14120+
"fqn": "jsii-calc.IStringable"
14121+
}
14122+
},
14123+
"static": true
14124+
},
14125+
{
14126+
"docs": {
14127+
"stability": "stable"
14128+
},
14129+
"locationInModule": {
14130+
"filename": "lib/compliance.ts",
14131+
"line": 3212
14132+
},
14133+
"name": "describe",
14134+
"overrides": "jsii-calc.IStringable",
14135+
"returns": {
14136+
"type": {
14137+
"primitive": "string"
14138+
}
14139+
}
14140+
},
14141+
{
14142+
"docs": {
14143+
"stability": "stable"
14144+
},
14145+
"locationInModule": {
14146+
"filename": "lib/compliance.ts",
14147+
"line": 3220
14148+
},
14149+
"name": "nativeToString",
14150+
"overrides": "jsii-calc.IStringable",
14151+
"returns": {
14152+
"type": {
14153+
"primitive": "string"
14154+
}
14155+
}
14156+
},
14157+
{
14158+
"docs": {
14159+
"stability": "stable"
14160+
},
14161+
"locationInModule": {
14162+
"filename": "lib/compliance.ts",
14163+
"line": 3216
14164+
},
14165+
"name": "toString",
14166+
"overrides": "jsii-calc.IStringable",
14167+
"returns": {
14168+
"type": {
14169+
"primitive": "string"
14170+
}
14171+
}
14172+
}
14173+
],
14174+
"name": "Stringable",
14175+
"symbolId": "lib/compliance:Stringable"
14176+
},
1399314177
"jsii-calc.StripInternal": {
1399414178
"assembly": "jsii-calc",
1399514179
"docs": {
@@ -19990,5 +20174,5 @@
1999020174
"intersection-types"
1999120175
],
1999220176
"version": "3.20.120",
19993-
"fingerprint": "lIHvj1O/NYsjHa9x/fPvrEdD8ChVE2BSlsfyzXUQaP4="
20177+
"fingerprint": "1S7GvkZ3HrOIC2Ats7hbLlT+W9UTRUwvKOdPD8/ogs8="
1999420178
}

0 commit comments

Comments
 (0)