Skip to content

Commit

Permalink
fix(runtime): type not found (#3763)
Browse files Browse the repository at this point in the history
When a jsii module returned a private class implementation of an interface, consuming jsii.rtti data may return an FQN for the internal class, which does not exist in foreign languages, leading to an error.

Revert to tagging FQNs directly instead.

Fixes #3742
  • Loading branch information
RomainMuller committed Sep 22, 2022
1 parent ad085eb commit c7dfad3
Show file tree
Hide file tree
Showing 25 changed files with 589 additions and 36 deletions.
9 changes: 9 additions & 0 deletions .all-contributorsrc
Original file line number Diff line number Diff line change
Expand Up @@ -1457,6 +1457,15 @@
"contributions": [
"review"
]
},
{
"login": "cn-cit",
"name": "cn-cit",
"avatar_url": "https://avatars.githubusercontent.com/u/27255477?v=4",
"profile": "https://github.com/cn-cit",
"contributions": [
"bug"
]
}
],
"repoType": "github",
Expand Down
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -228,23 +228,24 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
</tr>
<tr>
<td align="center"><a href="https://github.com/arnogeurts-sqills"><img src="https://avatars.githubusercontent.com/u/79304871?v=4?s=100" width="100px;" alt=""/><br /><sub><b>arnogeurts-sqills</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Aarnogeurts-sqills+label%3Abug" title="Bug reports">🐛</a> <a href="https://github.com/aws/jsii/commits?author=arnogeurts-sqills" title="Code">💻</a></td>
<td align="center"><a href="https://github.com/cn-cit"><img src="https://avatars.githubusercontent.com/u/27255477?v=4?s=100" width="100px;" alt=""/><br /><sub><b>cn-cit</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Acn-cit+label%3Abug" title="Bug reports">🐛</a></td>
<td align="center"><a href="https://github.com/deccy-mcc"><img src="https://avatars0.githubusercontent.com/u/45844893?v=4?s=100" width="100px;" alt=""/><br /><sub><b>deccy-mcc</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Adeccy-mcc+label%3Abug" title="Bug reports">🐛</a></td>
<td align="center"><a href="https://github.com/apps/dependabot-preview"><img src="https://avatars3.githubusercontent.com/in/2141?v=4?s=100" width="100px;" alt=""/><br /><sub><b>dependabot-preview[bot]</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Adependabot-preview[bot]+label%3Abug" title="Bug reports">🐛</a> <a href="https://github.com/aws/jsii/pulls?q=is%3Apr+author%3Adependabot-preview[bot]" title="Maintenance">🚧</a></td>
<td align="center"><a href="https://github.com/apps/dependabot"><img src="https://avatars0.githubusercontent.com/in/29110?v=4?s=100" width="100px;" alt=""/><br /><sub><b>dependabot[bot]</b></sub></a><br /><a href="https://github.com/aws/jsii/pulls?q=is%3Apr+author%3Adependabot[bot]" title="Maintenance">🚧</a></td>
<td align="center"><a href="https://github.com/dheffx"><img src="https://avatars0.githubusercontent.com/u/22029918?v=4?s=100" width="100px;" alt=""/><br /><sub><b>dheffx</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Adheffx+label%3Abug" title="Bug reports">🐛</a></td>
<td align="center"><a href="https://github.com/gregswdl"><img src="https://avatars0.githubusercontent.com/u/47365273?v=4?s=100" width="100px;" alt=""/><br /><sub><b>gregswdl</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Agregswdl+label%3Abug" title="Bug reports">🐛</a></td>
<td align="center"><a href="https://github.com/guyroberts21"><img src="https://avatars.githubusercontent.com/u/47118902?v=4?s=100" width="100px;" alt=""/><br /><sub><b>guyroberts21</b></sub></a><br /><a href="https://github.com/aws/jsii/commits?author=guyroberts21" title="Documentation">📖</a></td>
</tr>
<tr>
<td align="center"><a href="https://github.com/guyroberts21"><img src="https://avatars.githubusercontent.com/u/47118902?v=4?s=100" width="100px;" alt=""/><br /><sub><b>guyroberts21</b></sub></a><br /><a href="https://github.com/aws/jsii/commits?author=guyroberts21" title="Documentation">📖</a></td>
<td align="center"><a href="https://github.com/mattBrzezinski"><img src="https://avatars.githubusercontent.com/u/4356074?v=4?s=100" width="100px;" alt=""/><br /><sub><b>mattBrzezinski</b></sub></a><br /><a href="https://github.com/aws/jsii/commits?author=mattBrzezinski" title="Documentation">📖</a></td>
<td align="center"><a href="https://github.com/mergify"><img src="https://avatars.githubusercontent.com/u/18240476?v=4?s=100" width="100px;" alt=""/><br /><sub><b>mergify</b></sub></a><br /><a href="https://github.com/aws/jsii/pulls?q=is%3Apr+author%3Amergify" title="Maintenance">🚧</a></td>
<td align="center"><a href="https://github.com/apps/mergify"><img src="https://avatars1.githubusercontent.com/in/10562?v=4?s=100" width="100px;" alt=""/><br /><sub><b>mergify[bot]</b></sub></a><br /><a href="https://github.com/aws/jsii/pulls?q=is%3Apr+author%3Amergify[bot]" title="Maintenance">🚧</a></td>
<td align="center"><a href="https://github.com/nathannaveen"><img src="https://avatars.githubusercontent.com/u/42319948?v=4?s=100" width="100px;" alt=""/><br /><sub><b>nathannaveen</b></sub></a><br /><a href="https://github.com/aws/jsii/pulls?q=is%3Apr+author%3Anathannaveen" title="Maintenance">🚧</a></td>
<td align="center"><a href="https://github.com/seiyashima"><img src="https://avatars2.githubusercontent.com/u/4947101?v=4?s=100" width="100px;" alt=""/><br /><sub><b>seiyashima42</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Aseiyashima+label%3Abug" title="Bug reports">🐛</a> <a href="https://github.com/aws/jsii/commits?author=seiyashima" title="Code">💻</a> <a href="https://github.com/aws/jsii/commits?author=seiyashima" title="Documentation">📖</a></td>
<td align="center"><a href="https://github.com/sullis"><img src="https://avatars3.githubusercontent.com/u/30938?v=4?s=100" width="100px;" alt=""/><br /><sub><b>sullis</b></sub></a><br /><a href="https://github.com/aws/jsii/commits?author=sullis" title="Code">💻</a></td>
<td align="center"><a href="https://github.com/vaneek"><img src="https://avatars1.githubusercontent.com/u/8113305?v=4?s=100" width="100px;" alt=""/><br /><sub><b>vaneek</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Avaneek+label%3Abug" title="Bug reports">🐛</a></td>
</tr>
<tr>
<td align="center"><a href="https://github.com/vaneek"><img src="https://avatars1.githubusercontent.com/u/8113305?v=4?s=100" width="100px;" alt=""/><br /><sub><b>vaneek</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Avaneek+label%3Abug" title="Bug reports">🐛</a></td>
<td align="center"><a href="https://github.com/wendysophie"><img src="https://avatars.githubusercontent.com/u/54415551?v=4?s=100" width="100px;" alt=""/><br /><sub><b>wendysophie</b></sub></a><br /><a href="https://github.com/aws/jsii/issues?q=author%3Awendysophie+label%3Abug" title="Bug reports">🐛</a></td>
</tr>
</tbody>
Expand Down
3 changes: 2 additions & 1 deletion gh-pages/content/specification/6-compliance-report.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
This section details the current state of each language binding with respect to our standard compliance suite.


| number | test | java (98.33%) | golang (79.17%) | Dotnet | Python |
| number | test | java (98.35%) | golang (79.34%) | Dotnet | Python |
| ------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | -------------------------------------------- | ------ | ------ |
| 1 | asyncOverrides_overrideCallsSuper | 🟢 | [🔴](https://github.com/aws/jsii/issues/2670) |||
| 2 | [arrayReturnedByMethodCanBeRead]("Array created in the kernel can be queried for its elements") | 🟢 | 🟢 |||
Expand Down Expand Up @@ -127,3 +127,4 @@ This section details the current state of each language binding with respect to
| 118 | [callbackParameterIsInterface]("Validates pure interfaces can be passed to callbacks") || 🟢 |||
| 119 | [classCanBeUsedWhenNotExpressedlyLoaded]("Validates that types not explicitly loaded by the user can safely be returned by JS code") | 🟢 | 🟢 |||
| 120 | [downcasting]("Ensures unsafe-cast features work as expected") || 🟢 |||
| 121 | [strippedDeprecatedMemberCanBeReceived]("Ensures --strip-deprecated does not cause odd runtime errors") | 🟢 | 🟢 |||
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using CompositeOperation = Amazon.JSII.Tests.CalculatorNamespace.Composition.CompositeOperation;
using Amazon.JSII.Tests.CalculatorNamespace.LibNamespace;
using Amazon.JSII.Tests.CalculatorNamespace.BaseOfBaseNamespace;
using Amazon.JSII.Tests.CalculatorNamespace.LibNamespace.DeprecationRemoval;
using Newtonsoft.Json.Linq;
using Xunit;
using Xunit.Abstractions;
Expand Down Expand Up @@ -1534,9 +1535,15 @@ public void ClassCanBeUsedWhenNotExpressedlyLoaded()
}

private sealed class Cdk16625Impl: Cdk16625 {
protected override double Unwrap(IRandomNumberGenerator rng) {
return rng.Next();
}
protected override double Unwrap(IRandomNumberGenerator rng) {
return rng.Next();
}
}

[Fact(DisplayName = Prefix + nameof(StrippedDeprecatedMemberCanBeReceived))]
public void StrippedDeprecatedMemberCanBeReceived()
{
Assert.NotNull(InterfaceFactory.Create());
}
}
}
7 changes: 7 additions & 0 deletions packages/@jsii/go-runtime-test/project/compliance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/aws/jsii/jsii-calc/go/jsiicalc/v3/submodule/child"
calclib "github.com/aws/jsii/jsii-calc/go/scopejsiicalclib"
"github.com/aws/jsii/jsii-calc/go/scopejsiicalclib/customsubmodulename"
"github.com/aws/jsii/jsii-calc/go/scopejsiicalclib/deprecationremoval"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
)
Expand Down Expand Up @@ -1655,6 +1656,12 @@ func (suite *ComplianceSuite) TestDownCasting() {
require.Equal(realValue.Foo(), jsii.Number(1337))
}

func (suite *ComplianceSuite) TestStrippedDeprecatedMemberCanBeReceived() {
require := suite.Require()

require.NotNil(deprecationremoval.InterfaceFactory_Create())
}

// required to make `go test` recognize the suite.
func TestComplianceSuite(t *testing.T) {
suite.Run(t, new(ComplianceSuite))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ObjectNode;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import software.amazon.jsii.ComplianceSuiteHarness;
Expand All @@ -14,6 +15,7 @@
import software.amazon.jsii.tests.calculator.cdk16625.Cdk16625;
import software.amazon.jsii.tests.calculator.composition.CompositeOperation;
import software.amazon.jsii.tests.calculator.custom_submodule_name.NestingClass.NestedStruct;
import software.amazon.jsii.tests.calculator.lib.deprecation_removal.InterfaceFactory;
import software.amazon.jsii.tests.calculator.lib.EnumFromScopedModule;
import software.amazon.jsii.tests.calculator.lib.IFriendly;
import software.amazon.jsii.tests.calculator.lib.MyFirstStruct;
Expand All @@ -24,10 +26,6 @@

import java.io.IOException;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.security.AccessController;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.time.Instant;
Expand Down Expand Up @@ -1790,13 +1788,17 @@ public void iso8601DoesNotDeserializeToDate() {
final String nowAsISO = df.format(new Date());

final IWallClock wallClock = new IWallClock() {
@NotNull
@Override
public String iso8601Now() {
return nowAsISO;
}
};

final Entropy entropy = new Entropy(wallClock) {
public String repeat(final String word) {
@NotNull
@Override
public String repeat(@NotNull final String word) {
return word;
}
};
Expand All @@ -1807,11 +1809,17 @@ public String repeat(final String word) {
@Test
public void classCanBeUsedWhenNotExpressedlyLoaded() {
final Cdk16625 subject = new Cdk16625() {
@NotNull
@Override
protected java.lang.Number unwrap(final IRandomNumberGenerator rng) {
return rng.next();
}
};
subject.test();
}

@Test
public void strippedDeprecatedMemberCanBeReceived() {
assertNotNull(InterfaceFactory.create());
}
}
2 changes: 1 addition & 1 deletion packages/@jsii/kernel/src/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ export class Kernel {
case spec.TypeKind.Class:
case spec.TypeKind.Enum:
const constructor = this._findSymbol(fqn);
tagJsiiConstructor(constructor, fqn, assm.metadata.version);
tagJsiiConstructor(constructor, fqn);
}
}
}
Expand Down
32 changes: 16 additions & 16 deletions packages/@jsii/kernel/src/objects.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as spec from '@jsii/spec';
import * as assert from 'assert';

import * as api from './api';
import { JsiiFault } from './kernel';
Expand All @@ -15,15 +16,12 @@ const OBJID_SYMBOL = Symbol.for('$__jsii__objid__$');
const IFACES_SYMBOL = Symbol.for('$__jsii__interfaces__$');

/**
* Symbol we use to tag the constructor of a JSII class
* Symbol we use to tag constructors that are exported from a JSII module.
*/
const JSII_RTTI_SYMBOL = Symbol.for('jsii.rtti');
const JSII_TYPE_FQN_SYMBOL = Symbol('$__jsii__fqn__$');

interface ManagedConstructor {
readonly [JSII_RTTI_SYMBOL]: {
readonly fqn: string;
readonly version: string;
};
readonly [JSII_TYPE_FQN_SYMBOL]: string;
}

type MaybeManagedConstructor = Partial<ManagedConstructor>;
Expand All @@ -36,7 +34,7 @@ type MaybeManagedConstructor = Partial<ManagedConstructor>;
* information.
*/
export function jsiiTypeFqn(obj: any): string | undefined {
return (obj.constructor as MaybeManagedConstructor)[JSII_RTTI_SYMBOL]?.fqn;
return (obj.constructor as MaybeManagedConstructor)[JSII_TYPE_FQN_SYMBOL];
}

/**
Expand Down Expand Up @@ -96,19 +94,21 @@ function tagObject(obj: unknown, objid: string, interfaces?: string[]) {
/**
* Set the JSII FQN for classes produced by a given constructor
*/
export function tagJsiiConstructor(
constructor: any,
fqn: string,
version: string,
) {
if (Object.prototype.hasOwnProperty.call(constructor, JSII_RTTI_SYMBOL)) {
return;
export function tagJsiiConstructor(constructor: any, fqn: string) {
if (Object.prototype.hasOwnProperty.call(constructor, JSII_TYPE_FQN_SYMBOL)) {
return assert(
constructor[JSII_TYPE_FQN_SYMBOL] === fqn,
`Unable to register ${constructor.name} as ${fqn}: it is already registerd with FQN ${constructor[JSII_TYPE_FQN_SYMBOL]}`,
);
}
Object.defineProperty(constructor, JSII_RTTI_SYMBOL, {

// Mark this constructor as exported from a jsii module, so we know we
// should be considering it's FQN as a valid exported type.
Object.defineProperty(constructor, JSII_TYPE_FQN_SYMBOL, {
configurable: false,
enumerable: false,
writable: false,
value: { fqn, version },
value: fqn,
});
}

Expand Down
5 changes: 5 additions & 0 deletions packages/@jsii/python-runtime/tests/test_compliance.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
from jsii_calc.submodule.child import SomeEnum
from scope.jsii_calc_lib import IFriendly, EnumFromScopedModule, Number
from scope.jsii_calc_lib.custom_submodule_name import IReflectable, ReflectableEntry
from scope.jsii_calc_lib.deprecation_removal import InterfaceFactory

# Note: The names of these test functions have been chosen to map as closely to the
# Java Compliance tests as possible.
Expand Down Expand Up @@ -1343,3 +1344,7 @@ def _unwrap(self, rng: IRandomNumberGenerator):

# This should NOT throw
Subject().test()


def test_stripped_deprecated_member_can_be_received():
assert InterfaceFactory.create() is not None
1 change: 1 addition & 0 deletions packages/@scope/jsii-calc-lib/deprecated-to-strip.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@scope/jsii-calc-lib.deprecationRemoval.DeprecatedImplementation
16 changes: 16 additions & 0 deletions packages/@scope/jsii-calc-lib/lib/deprecation-removal.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
export interface IInterface {
method(): void;
}

/** @deprecated do not use me! */
export class DeprecatedImplementation implements IInterface {
public method(): void { }
}

export class InterfaceFactory {
public static create(): IInterface {
return new DeprecatedImplementation();
}

private constructor() { }
}
1 change: 1 addition & 0 deletions packages/@scope/jsii-calc-lib/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,3 +129,4 @@ export class BaseFor2647 {

export * as submodule from './submodule';
export * from './duplicate-inherited-prop';
export * as deprecationRemoval from './deprecation-removal';
2 changes: 1 addition & 1 deletion packages/@scope/jsii-calc-lib/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"main": "build/index.js",
"types": "build/index.d.ts",
"scripts": {
"build": "jsii --project-references && jsii-rosetta",
"build": "jsii --project-references --strip-deprecated=deprecated-to-strip.txt && jsii-rosetta",
"pacmak": "jsii-pacmak",
"test": "diff-test test/assembly.jsii .jsii",
"test:update": "npm run build && UPDATE_DIFF=1 npm run test"
Expand Down
70 changes: 69 additions & 1 deletion packages/@scope/jsii-calc-lib/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@
},
"schema": "jsii/0.10.0",
"submodules": {
"@scope/jsii-calc-lib.deprecationRemoval": {
"locationInModule": {
"filename": "lib/index.ts",
"line": 132
},
"symbolId": "lib/deprecation-removal:"
},
"@scope/jsii-calc-lib.submodule": {
"locationInModule": {
"filename": "lib/index.ts",
Expand Down Expand Up @@ -729,6 +736,67 @@
],
"symbolId": "lib/index:StructWithOnlyOptionals"
},
"@scope/jsii-calc-lib.deprecationRemoval.IInterface": {
"assembly": "@scope/jsii-calc-lib",
"docs": {
"stability": "deprecated"
},
"fqn": "@scope/jsii-calc-lib.deprecationRemoval.IInterface",
"kind": "interface",
"locationInModule": {
"filename": "lib/deprecation-removal.ts",
"line": 1
},
"methods": [
{
"abstract": true,
"docs": {
"stability": "deprecated"
},
"locationInModule": {
"filename": "lib/deprecation-removal.ts",
"line": 2
},
"name": "method"
}
],
"name": "IInterface",
"namespace": "deprecationRemoval",
"symbolId": "lib/deprecation-removal:IInterface"
},
"@scope/jsii-calc-lib.deprecationRemoval.InterfaceFactory": {
"assembly": "@scope/jsii-calc-lib",
"docs": {
"stability": "deprecated"
},
"fqn": "@scope/jsii-calc-lib.deprecationRemoval.InterfaceFactory",
"kind": "class",
"locationInModule": {
"filename": "lib/deprecation-removal.ts",
"line": 10
},
"methods": [
{
"docs": {
"stability": "deprecated"
},
"locationInModule": {
"filename": "lib/deprecation-removal.ts",
"line": 11
},
"name": "create",
"returns": {
"type": {
"fqn": "@scope/jsii-calc-lib.deprecationRemoval.IInterface"
}
},
"static": true
}
],
"name": "InterfaceFactory",
"namespace": "deprecationRemoval",
"symbolId": "lib/deprecation-removal:InterfaceFactory"
},
"@scope/jsii-calc-lib.submodule.IReflectable": {
"assembly": "@scope/jsii-calc-lib",
"docs": {
Expand Down Expand Up @@ -954,5 +1022,5 @@
}
},
"version": "0.0.0",
"fingerprint": "kYLYyNyPod3JTyJWmgPJL1Z85k0HEEhQeMIs4zH4bEQ="
"fingerprint": "XDMAZYhhgc09X8VS8hpn3ch21YxKDn+HB0w82IofsRM="
}
Loading

0 comments on commit c7dfad3

Please sign in to comment.