Skip to content

Commit

Permalink
fix(kernel): Improve tagged type of wire values (#346)
Browse files Browse the repository at this point in the history 
When passing references across the JSII language boundary, the static
return type of a method is used instead of the runtime type of the
object if they are not the same, even if they are compatible. This
causes issues that make it impossible to up-cast from methods such as
`IConstruct.findChild` that return a super-class that is expected to be
up-casted to it's known dynamic type.

Fixes #345
  • Loading branch information
@RomainMuller
RomainMuller authored Jan 29, 2019
1 parent 0301e30 commit 8ea39ac
Show file tree
Hide file tree
Showing 23 changed files with 589 additions and 7 deletions.
25 changes: 24 additions & 1 deletion packages/jsii-calc/lib/compliance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1232,4 +1232,27 @@ export class JsiiAgent {
public static get jsiiAgent(): string | undefined {
return process.env.JSII_AGENT;
}
};
};

// Ensure the JSII kernel tags instances with the "most appropriate" FQN type label, so that runtimes are able to
// correctly choose the implementation proxy that should be used. Failure to do so could cause situations where userland
// needs to up-cast an instance to an incompatible type, which certain runtimes (such as Java) will prevent.
// @See https://github.com/awslabs/jsii/issues/345
export class PublicClass {
public hello(): void {}
}
export interface IPublicInterface {
bye(): void;
}
export class InbetweenClass extends PublicClass {}
class PrivateClass extends InbetweenClass implements IPublicInterface {
public bye(): void {}
}
export class Constructors {
public static makeClass(): PublicClass {
return new PrivateClass();
}
public static makeInterface(): IPublicInterface {
return new PrivateClass();
}
}
65 changes: 64 additions & 1 deletion packages/jsii-calc/test/assembly.jsii
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,31 @@
}
]
},
"jsii-calc.Constructors": {
"assembly": "jsii-calc",
"fqn": "jsii-calc.Constructors",
"initializer": {
"initializer": true
},
"kind": "class",
"methods": [
{
"name": "makeClass",
"returns": {
"fqn": "jsii-calc.PublicClass"
},
"static": true
},
{
"name": "makeInterface",
"returns": {
"fqn": "jsii-calc.IPublicInterface"
},
"static": true
}
],
"name": "Constructors"
},
"jsii-calc.DefaultedConstructorArgument": {
"assembly": "jsii-calc",
"fqn": "jsii-calc.DefaultedConstructorArgument",
Expand Down Expand Up @@ -1675,6 +1700,18 @@
}
]
},
"jsii-calc.IPublicInterface": {
"assembly": "jsii-calc",
"fqn": "jsii-calc.IPublicInterface",
"kind": "interface",
"methods": [
{
"abstract": true,
"name": "bye"
}
],
"name": "IPublicInterface"
},
"jsii-calc.IRandomNumberGenerator": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -1743,6 +1780,18 @@
}
]
},
"jsii-calc.InbetweenClass": {
"assembly": "jsii-calc",
"base": {
"fqn": "jsii-calc.PublicClass"
},
"fqn": "jsii-calc.InbetweenClass",
"initializer": {
"initializer": true
},
"kind": "class",
"name": "InbetweenClass"
},
"jsii-calc.InterfaceImplementedByAbstractClass": {
"assembly": "jsii-calc",
"datatype": true,
Expand Down Expand Up @@ -2852,6 +2901,20 @@
}
]
},
"jsii-calc.PublicClass": {
"assembly": "jsii-calc",
"fqn": "jsii-calc.PublicClass",
"initializer": {
"initializer": true
},
"kind": "class",
"methods": [
{
"name": "hello"
}
],
"name": "PublicClass"
},
"jsii-calc.ReferenceEnumFromScopedPackage": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -3762,5 +3825,5 @@
}
},
"version": "0.7.13",
"fingerprint": "mc9Rni6GLgVDz8M6CGHapEiYbWsdEIl7CmBI0Qn+KXU="
"fingerprint": "WNp0Sw1d2+3FaQD0Fn01K4wYQi7Ms3w+NQatOylD1kM="
}
12 changes: 11 additions & 1 deletion ...ges/jsii-dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,16 @@ public void ReceiveInstanceOfPrivateClass()
Assert.True(new ReturnsPrivateImplementationOfInterface().PrivateImplementation.Success);
}

[Fact(DisplayName = Prefix + nameof(ObjRefsAreLabelledUsingWithTheMostCorrectType))]
public void ObjRefsAreLabelledUsingWithTheMostCorrectType()
{
var classRef = Constructors.MakeClass();
var ifaceRef = Constructors.MakeInterface();

Assert.Equal(typeof(InbetweenClass), classRef.GetType());
Assert.NotEqual(typeof(InbetweenClass), ifaceRef.GetType());
}

class NumberReturner : DeputyBase, IIReturnsNumber
{
public NumberReturner(double number)
Expand Down Expand Up @@ -1038,4 +1048,4 @@ public double Next()
}
}
}
}
}
16 changes: 15 additions & 1 deletion ...-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@
import software.amazon.jsii.tests.calculator.GreetingAugmenter;
import software.amazon.jsii.tests.calculator.IFriendlier;
import software.amazon.jsii.tests.calculator.IFriendlyRandomGenerator;
import software.amazon.jsii.tests.calculator.IPublicInterface;
import software.amazon.jsii.tests.calculator.InterfaceWithProperties;
import software.amazon.jsii.tests.calculator.IRandomNumberGenerator;
import software.amazon.jsii.tests.calculator.InbetweenClass;
import software.amazon.jsii.tests.calculator.InterfaceImplementedByAbstractClass;
import software.amazon.jsii.tests.calculator.JSObjectLiteralForInterface;
import software.amazon.jsii.tests.calculator.JSObjectLiteralToNative;
Expand All @@ -31,6 +33,7 @@
import software.amazon.jsii.tests.calculator.NumberGenerator;
import software.amazon.jsii.tests.calculator.Polymorphism;
import software.amazon.jsii.tests.calculator.Power;
import software.amazon.jsii.tests.calculator.PublicClass;
import software.amazon.jsii.tests.calculator.ReferenceEnumFromScopedPackage;
import software.amazon.jsii.tests.calculator.ReturnsPrivateImplementationOfInterface;
import software.amazon.jsii.tests.calculator.Statics;
Expand All @@ -48,6 +51,8 @@
import software.amazon.jsii.tests.calculator.lib.Value;
import software.amazon.jsii.tests.calculator.JavaReservedWords;
import software.amazon.jsii.tests.calculator.ClassWithPrivateConstructorAndAutomaticProperties;
import software.amazon.jsii.tests.calculator.Constructors;

import org.junit.Test;

import java.io.IOException;
Expand Down Expand Up @@ -954,7 +959,7 @@ public void nullShouldBeTreatedAsUndefined() {
obj.setChangeMeToUndefined(null);
obj.verifyPropertyIsUndefined();
}

@Test
public void testJsiiAgent() {
assertEquals("Java/" + System.getProperty("java.version"), JsiiAgent.getJsiiAgent());
Expand All @@ -968,6 +973,15 @@ public void receiveInstanceOfPrivateClass() {
assertTrue(new ReturnsPrivateImplementationOfInterface().getPrivateImplementation().getSuccess());
}

@Test
public void objRefsAreLabelledUsingWithTheMostCorrectType() {
final PublicClass classRef = Constructors.makeClass();
final IPublicInterface ifaceRef = Constructors.makeInterface();

assertTrue(classRef instanceof InbetweenClass);
assertTrue(ifaceRef instanceof IPublicInterface);
}

static class MulTen extends Multiply {
public MulTen(final int value) {
super(new Number(value), new Number(10));
Expand Down
27 changes: 26 additions & 1 deletion packages/jsii-kernel/lib/kernel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,7 @@ export class Kernel {
// have an object id, so we need to allocate one for it.
this._debug('creating objref for', v);
const fqn = this._fqnForObject(v);
if (!targetType || !spec.isNamedTypeReference(targetType) || fqn === targetType.fqn) {
if (!targetType || !spec.isNamedTypeReference(targetType) || this._isAssignable(fqn, targetType)) {
return this._createObjref(v, fqn);
}
}
Expand Down Expand Up @@ -1029,6 +1029,31 @@ export class Kernel {
return v;
}

/**
* Tests whether a given type (by it's FQN) can be assigned to a named type reference.
*
* @param actualTypeFqn the FQN of the type that is being tested.
* @param requiredType the required reference type.
*
* @returns true if ``requiredType`` is a super-type (base class or implemented interface) of the type designated by
* ``actualTypeFqn``.
*/
private _isAssignable(actualTypeFqn: string, requiredType: spec.NamedTypeReference): boolean {
if (requiredType.fqn === actualTypeFqn) {
return true;
}
const actualType = this._typeInfoForFqn(actualTypeFqn);
if (spec.isClassType(actualType) && actualType.base) {
if (this._isAssignable(actualType.base.fqn, requiredType)) {
return true;
}
}
if (spec.isClassOrInterfaceType(actualType) && actualType.interfaces) {
return actualType.interfaces.find(iface => this._isAssignable(iface.fqn, requiredType)) != null;
}
return false;
}

private _toSandboxValues(args: any[]) {
return args.map(v => this._toSandbox(v));
}
Expand Down
12 changes: 11 additions & 1 deletion packages/jsii-kernel/test/test.kernel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,7 @@ defineTest('object literals are returned by reference', async (test, sandbox) =>

test.equal(newValue,
sandbox.get({
objref: sandbox.get({ objref, property: 'mutableObject' }).value,
objref: sandbox.get({ objref, property: 'mutableObject' }).value,
property: 'value'
}).value);

Expand Down Expand Up @@ -963,6 +963,16 @@ defineTest('JSII_AGENT is undefined in node.js', async (test, sandbox) => {
test.equal(sandbox.sget({ fqn: 'jsii-calc.JsiiAgent', property: 'jsiiAgent' }).value, undefined);
});

defineTest('ObjRefs are labeled with the "most correct" type', async (test, sandbox) => {
const classRef = sandbox.sinvoke({ fqn: 'jsii-calc.Constructors', method: 'makeClass' }).result as api.ObjRef;
const ifaceRef = sandbox.sinvoke({ fqn: 'jsii-calc.Constructors', method: 'makeInterface' }).result as api.ObjRef;

test.ok(classRef[api.TOKEN_REF].startsWith('jsii-calc.InbetweenClass'),
`${classRef[api.TOKEN_REF]} starts with jsii-calc.InbetweenClass`);
test.ok(ifaceRef[api.TOKEN_REF].startsWith('jsii-calc.IPublicInterface'),
`${ifaceRef[api.TOKEN_REF]} starts with jsii-calc.IPublicInterface`);
});

// =================================================================================================

const testNames: { [name: string]: boolean } = { };
Expand Down
65 changes: 64 additions & 1 deletion ...es/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,31 @@
}
]
},
"jsii-calc.Constructors": {
"assembly": "jsii-calc",
"fqn": "jsii-calc.Constructors",
"initializer": {
"initializer": true
},
"kind": "class",
"methods": [
{
"name": "makeClass",
"returns": {
"fqn": "jsii-calc.PublicClass"
},
"static": true
},
{
"name": "makeInterface",
"returns": {
"fqn": "jsii-calc.IPublicInterface"
},
"static": true
}
],
"name": "Constructors"
},
"jsii-calc.DefaultedConstructorArgument": {
"assembly": "jsii-calc",
"fqn": "jsii-calc.DefaultedConstructorArgument",
Expand Down Expand Up @@ -1675,6 +1700,18 @@
}
]
},
"jsii-calc.IPublicInterface": {
"assembly": "jsii-calc",
"fqn": "jsii-calc.IPublicInterface",
"kind": "interface",
"methods": [
{
"abstract": true,
"name": "bye"
}
],
"name": "IPublicInterface"
},
"jsii-calc.IRandomNumberGenerator": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -1743,6 +1780,18 @@
}
]
},
"jsii-calc.InbetweenClass": {
"assembly": "jsii-calc",
"base": {
"fqn": "jsii-calc.PublicClass"
},
"fqn": "jsii-calc.InbetweenClass",
"initializer": {
"initializer": true
},
"kind": "class",
"name": "InbetweenClass"
},
"jsii-calc.InterfaceImplementedByAbstractClass": {
"assembly": "jsii-calc",
"datatype": true,
Expand Down Expand Up @@ -2852,6 +2901,20 @@
}
]
},
"jsii-calc.PublicClass": {
"assembly": "jsii-calc",
"fqn": "jsii-calc.PublicClass",
"initializer": {
"initializer": true
},
"kind": "class",
"methods": [
{
"name": "hello"
}
],
"name": "PublicClass"
},
"jsii-calc.ReferenceEnumFromScopedPackage": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -3762,5 +3825,5 @@
}
},
"version": "0.7.13",
"fingerprint": "mc9Rni6GLgVDz8M6CGHapEiYbWsdEIl7CmBI0Qn+KXU="
"fingerprint": "WNp0Sw1d2+3FaQD0Fn01K4wYQi7Ms3w+NQatOylD1kM="
}
Loading

0 comments on commit 8ea39ac

Please sign in to comment.