Skip to content

Commit b37101f

Browse files
RomainMullerElad Ben-Israel
RomainMuller
authored and
Elad Ben-Israel
committed
fix: Missing types in JSII assembly, invalid Java code, confusing docs (#208)
* The new `jsii` would omit interfaces defined within namespaces because of the way namespaces were processed (using members instead of listing all exports). * Some `jsii` type validations could, in some rare cases, happen too early, attempting to dereference types that hadn't been processed yet. * The `jsii-pacmak` generator for Java could generate property names that were reserved words (e.g: `assert`). * the `jsii-pacmak` generator for Sphinx would generate confusing (or incorrect) type documentation for entities of array types, and particularly so for arrays of unions. * Fixes #175: interface proxies do not respect optional method arguments Testing gaps: - [ ] Test surface in `jsii-calc` or its dependencies that exercise reserved words of various languages.
1 parent 6d92b60 commit b37101f

File tree

34 files changed

+888
-145
lines changed

34 files changed

+888
-145
lines changed

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -782,3 +782,35 @@ export class ReferenceEnumFromScopedPackage {
782782
this.foo = value;
783783
}
784784
}
785+
786+
/**
787+
* awslabs/jsii#208
788+
* Interface within a namespace
789+
*/
790+
export namespace InterfaceInNamespaceOnlyInterface {
791+
792+
// it's a special case when only an interface is exported from a namespace
793+
export interface Hello {
794+
foo: number
795+
}
796+
797+
}
798+
799+
export namespace InterfaceInNamespaceIncludesClasses {
800+
801+
export class Foo {
802+
public bar?: string;
803+
}
804+
805+
export interface Hello {
806+
foo: number
807+
}
808+
}
809+
810+
/**
811+
* awslabs/jsii#175
812+
* Interface proxies (and builders) do not respect optional arguments in methods
813+
*/
814+
export interface InterfaceWithOptionalMethodArguments {
815+
hello(arg1: string, arg2?: number): void
816+
}

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

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1244,6 +1244,86 @@
12441244
}
12451245
]
12461246
},
1247+
"jsii-calc.InterfaceInNamespaceIncludesClasses.Foo": {
1248+
"assembly": "jsii-calc",
1249+
"fqn": "jsii-calc.InterfaceInNamespaceIncludesClasses.Foo",
1250+
"initializer": {
1251+
"initializer": true
1252+
},
1253+
"kind": "class",
1254+
"name": "Foo",
1255+
"namespace": "InterfaceInNamespaceIncludesClasses",
1256+
"properties": [
1257+
{
1258+
"name": "bar",
1259+
"type": {
1260+
"optional": true,
1261+
"primitive": "string"
1262+
}
1263+
}
1264+
]
1265+
},
1266+
"jsii-calc.InterfaceInNamespaceIncludesClasses.Hello": {
1267+
"assembly": "jsii-calc",
1268+
"datatype": true,
1269+
"fqn": "jsii-calc.InterfaceInNamespaceIncludesClasses.Hello",
1270+
"kind": "interface",
1271+
"name": "Hello",
1272+
"namespace": "InterfaceInNamespaceIncludesClasses",
1273+
"properties": [
1274+
{
1275+
"name": "foo",
1276+
"type": {
1277+
"primitive": "number"
1278+
}
1279+
}
1280+
]
1281+
},
1282+
"jsii-calc.InterfaceInNamespaceOnlyInterface.Hello": {
1283+
"assembly": "jsii-calc",
1284+
"datatype": true,
1285+
"fqn": "jsii-calc.InterfaceInNamespaceOnlyInterface.Hello",
1286+
"kind": "interface",
1287+
"name": "Hello",
1288+
"namespace": "InterfaceInNamespaceOnlyInterface",
1289+
"properties": [
1290+
{
1291+
"name": "foo",
1292+
"type": {
1293+
"primitive": "number"
1294+
}
1295+
}
1296+
]
1297+
},
1298+
"jsii-calc.InterfaceWithOptionalMethodArguments": {
1299+
"assembly": "jsii-calc",
1300+
"docs": {
1301+
"comment": "awslabs/jsii#175\nInterface proxies (and builders) do not respect optional arguments in methods"
1302+
},
1303+
"fqn": "jsii-calc.InterfaceWithOptionalMethodArguments",
1304+
"kind": "interface",
1305+
"methods": [
1306+
{
1307+
"name": "hello",
1308+
"parameters": [
1309+
{
1310+
"name": "arg1",
1311+
"type": {
1312+
"primitive": "string"
1313+
}
1314+
},
1315+
{
1316+
"name": "arg2",
1317+
"type": {
1318+
"optional": true,
1319+
"primitive": "number"
1320+
}
1321+
}
1322+
]
1323+
}
1324+
],
1325+
"name": "InterfaceWithOptionalMethodArguments"
1326+
},
12471327
"jsii-calc.JSObjectLiteralForInterface": {
12481328
"assembly": "jsii-calc",
12491329
"fqn": "jsii-calc.JSObjectLiteralForInterface",
@@ -2844,5 +2924,5 @@
28442924
}
28452925
},
28462926
"version": "0.7.1",
2847-
"fingerprint": "xRKsZzmRl0yM7EYcHnUbf691eFzAupWFIvCbEqWcUuA="
2927+
"fingerprint": "3uROoDToOcKIpYs9haAGnS37Iebz1/+ldcknrh37qDQ="
28482928
}

‎packages/jsii-pacmak/lib/generator.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,16 @@ export abstract class Generator implements IGenerator {
302302
return this.excludeTypes.includes(name);
303303
}
304304

305-
private createOverloadsForOptionals(method: spec.Method) {
305+
/**
306+
* Returns all the method overloads needed to satisfy optional arguments.
307+
* For example, for the method `foo(bar: string, hello?: number, world?: number)`
308+
* this method will return:
309+
* - foo(bar: string)
310+
* - foo(bar: string, hello: number)
311+
*
312+
* Notice that the method that contains all the arguments will not be returned.
313+
*/
314+
protected createOverloadsForOptionals(method: spec.Method) {
306315
const methods = new Array<spec.Method>();
307316

308317
// if option disabled, just return the empty array.

‎packages/jsii-pacmak/lib/targets/java.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -652,6 +652,10 @@ class JavaGenerator extends Generator {
652652
for (const methodName of Object.keys(methods)) {
653653
const method = methods[methodName];
654654
this.emitMethod(ifc, method, /* overrides: */ true);
655+
656+
for (const overloadedMethod of this.createOverloadsForOptionals(method)) {
657+
this.emitMethod(ifc, overloadedMethod, /* overrides: */ true);
658+
}
655659
}
656660

657661
this.code.closeBlock();
@@ -754,20 +758,20 @@ class JavaGenerator extends Generator {
754758
for (const prop of props) {
755759
if (prop.optional) { this.code.line(JSR305_NULLABLE); }
756760
// tslint:disable-next-line:max-line-length
757-
this.code.line(`private${prop.immutable ? ' final' : ''} ${prop.fieldJavaType} ${prop.fieldName} = ${_validateIfNonOptional(`_${prop.fieldName}`, prop)};`);
761+
this.code.line(`private${prop.immutable ? ' final' : ''} ${prop.fieldJavaType} $${prop.fieldName} = ${_validateIfNonOptional(`_${prop.fieldName}`, prop)};`);
758762
}
759763
for (const prop of props) {
760764
this.code.line();
761765
this.code.line('@Override');
762766
this.code.openBlock(`public ${prop.fieldJavaType} get${prop.propName}()`);
763-
this.code.line(`return this.${prop.fieldName};`);
767+
this.code.line(`return this.$${prop.fieldName};`);
764768
this.code.closeBlock();
765769
if (!prop.immutable) {
766770
for (const type of prop.javaTypes) {
767771
this.code.line();
768772
this.code.line('@Override');
769773
this.code.openBlock(`public void set${prop.propName}(${prop.optional ? `${JSR305_NULLABLE} ` : ''}final ${type} value)`);
770-
this.code.line(`this.${prop.fieldName} = ${_validateIfNonOptional('value', prop)};`);
774+
this.code.line(`this.$${prop.fieldName} = ${_validateIfNonOptional('value', prop)};`);
771775
this.code.closeBlock();
772776
}
773777
}

‎packages/jsii-pacmak/lib/targets/sphinx.ts‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -517,33 +517,42 @@ class SphinxDocsGenerator extends Generator {
517517
};
518518
} else if (spec.isCollectionTypeReference(type)) {
519519
const elementType = this.renderTypeRef(type.collection.elementtype);
520+
const ref = wrap(elementType.ref);
521+
const display = wrap(elementType.display);
520522

521523
switch (type.collection.kind) {
522524
case spec.CollectionKind.Array:
523525
result = {
524-
ref: elementType.ref,
525-
display: `${elementType.display}[]`
526+
ref: `${ref}[]`,
527+
display: `${display}[]`
526528
};
527529
break;
528530
case spec.CollectionKind.Map:
529531
result = {
530-
ref: elementType.ref,
531-
display: `string => ${elementType.display}`
532+
ref: `string => ${ref}`,
533+
display: `string => ${display}`
532534
};
533535
break;
534536
default:
535537
throw new Error(`Unexpected collection kind: ${type.collection.kind}`);
536538
}
537539
} else if (spec.isUnionTypeReference(type)) {
540+
const mappedTypes = type.union.types.map(t => this.renderTypeRef(t));
538541
result = {
539-
display: type.union.types.map(t => this.renderTypeRef(t).display).join(' or '),
540-
ref: type.union.types.map(t => this.renderTypeRef(t).ref).join(' or '),
542+
display: mappedTypes.map(t => t.display).join(' or '),
543+
ref: mappedTypes.map(t => t.ref).join(' or '),
541544
};
542545
} else {
543546
throw new Error('Unexpected type ref');
544547
}
545548
if (type.optional) { result.ref = `${result.ref} or undefined`; }
546549
return result;
550+
551+
// Wrap a string between parenthesis if it contains " or "
552+
function wrap(str: string): string {
553+
if (str.indexOf(' or ') === -1) { return str; }
554+
return `(${str})`;
555+
}
547556
}
548557

549558
private renderProperty(prop: spec.Property) {

‎packages/jsii-pacmak/test/expected.jsii-calc-base/java/src/main/java/software/amazon/jsii/tests/calculator/base/BaseProps.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,27 +45,27 @@ public Builder withFoo(final software.amazon.jsii.tests.calculator.baseofbase.Ve
4545
*/
4646
public BaseProps build() {
4747
return new BaseProps() {
48-
private java.lang.String bar = java.util.Objects.requireNonNull(_bar, "bar is required");
49-
private software.amazon.jsii.tests.calculator.baseofbase.Very foo = java.util.Objects.requireNonNull(_foo, "foo is required");
48+
private java.lang.String $bar = java.util.Objects.requireNonNull(_bar, "bar is required");
49+
private software.amazon.jsii.tests.calculator.baseofbase.Very $foo = java.util.Objects.requireNonNull(_foo, "foo is required");
5050

5151
@Override
5252
public java.lang.String getBar() {
53-
return this.bar;
53+
return this.$bar;
5454
}
5555

5656
@Override
5757
public void setBar(final java.lang.String value) {
58-
this.bar = java.util.Objects.requireNonNull(value, "bar is required");
58+
this.$bar = java.util.Objects.requireNonNull(value, "bar is required");
5959
}
6060

6161
@Override
6262
public software.amazon.jsii.tests.calculator.baseofbase.Very getFoo() {
63-
return this.foo;
63+
return this.$foo;
6464
}
6565

6666
@Override
6767
public void setFoo(final software.amazon.jsii.tests.calculator.baseofbase.Very value) {
68-
this.foo = java.util.Objects.requireNonNull(value, "foo is required");
68+
this.$foo = java.util.Objects.requireNonNull(value, "foo is required");
6969
}
7070

7171
};

‎packages/jsii-pacmak/test/expected.jsii-calc-lib/java/src/main/java/software/amazon/jsii/tests/calculator/lib/MyFirstStruct.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -75,39 +75,39 @@ public Builder withFirstOptional(@javax.annotation.Nullable final java.util.List
7575
*/
7676
public MyFirstStruct build() {
7777
return new MyFirstStruct() {
78-
private java.lang.Number anumber = java.util.Objects.requireNonNull(_anumber, "anumber is required");
79-
private java.lang.String astring = java.util.Objects.requireNonNull(_astring, "astring is required");
78+
private java.lang.Number $anumber = java.util.Objects.requireNonNull(_anumber, "anumber is required");
79+
private java.lang.String $astring = java.util.Objects.requireNonNull(_astring, "astring is required");
8080
@javax.annotation.Nullable
81-
private java.util.List<java.lang.String> firstOptional = _firstOptional;
81+
private java.util.List<java.lang.String> $firstOptional = _firstOptional;
8282

8383
@Override
8484
public java.lang.Number getAnumber() {
85-
return this.anumber;
85+
return this.$anumber;
8686
}
8787

8888
@Override
8989
public void setAnumber(final java.lang.Number value) {
90-
this.anumber = java.util.Objects.requireNonNull(value, "anumber is required");
90+
this.$anumber = java.util.Objects.requireNonNull(value, "anumber is required");
9191
}
9292

9393
@Override
9494
public java.lang.String getAstring() {
95-
return this.astring;
95+
return this.$astring;
9696
}
9797

9898
@Override
9999
public void setAstring(final java.lang.String value) {
100-
this.astring = java.util.Objects.requireNonNull(value, "astring is required");
100+
this.$astring = java.util.Objects.requireNonNull(value, "astring is required");
101101
}
102102

103103
@Override
104104
public java.util.List<java.lang.String> getFirstOptional() {
105-
return this.firstOptional;
105+
return this.$firstOptional;
106106
}
107107

108108
@Override
109109
public void setFirstOptional(@javax.annotation.Nullable final java.util.List<java.lang.String> value) {
110-
this.firstOptional = value;
110+
this.$firstOptional = value;
111111
}
112112

113113
};

‎packages/jsii-pacmak/test/expected.jsii-calc-lib/java/src/main/java/software/amazon/jsii/tests/calculator/lib/StructWithOnlyOptionals.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -72,40 +72,40 @@ public Builder withOptional3(@javax.annotation.Nullable final java.lang.Boolean
7272
public StructWithOnlyOptionals build() {
7373
return new StructWithOnlyOptionals() {
7474
@javax.annotation.Nullable
75-
private java.lang.String optional1 = _optional1;
75+
private java.lang.String $optional1 = _optional1;
7676
@javax.annotation.Nullable
77-
private java.lang.Number optional2 = _optional2;
77+
private java.lang.Number $optional2 = _optional2;
7878
@javax.annotation.Nullable
79-
private java.lang.Boolean optional3 = _optional3;
79+
private java.lang.Boolean $optional3 = _optional3;
8080

8181
@Override
8282
public java.lang.String getOptional1() {
83-
return this.optional1;
83+
return this.$optional1;
8484
}
8585

8686
@Override
8787
public void setOptional1(@javax.annotation.Nullable final java.lang.String value) {
88-
this.optional1 = value;
88+
this.$optional1 = value;
8989
}
9090

9191
@Override
9292
public java.lang.Number getOptional2() {
93-
return this.optional2;
93+
return this.$optional2;
9494
}
9595

9696
@Override
9797
public void setOptional2(@javax.annotation.Nullable final java.lang.Number value) {
98-
this.optional2 = value;
98+
this.$optional2 = value;
9999
}
100100

101101
@Override
102102
public java.lang.Boolean getOptional3() {
103-
return this.optional3;
103+
return this.$optional3;
104104
}
105105

106106
@Override
107107
public void setOptional3(@javax.annotation.Nullable final java.lang.Boolean value) {
108-
this.optional3 = value;
108+
this.$optional3 = value;
109109
}
110110

111111
};

‎packages/jsii-pacmak/test/expected.jsii-calc-lib/sphinx/_scope_jsii-calc-lib.rst‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ MyFirstStruct (interface)
248248

249249
.. py:attribute:: firstOptional
250250
251-
:type: string or undefined
251+
:type: string[] or undefined
252252

253253

254254
Number

0 commit comments

Comments
 (0)