Skip to content

Commit

Permalink
feat(java): use covariant types for collection elements (#1884)
Browse files Browse the repository at this point in the history 
Using covariant expressions instead of just the type name enables a
better experience when using libraries such as Guava to build up
collections (`List` or `Map`) to use with the libraries. In particular,
collections of `any` are particularly annoying to use without covariant
typing because they'd force the user to manually specify generic
parameters in their code that add nothing to the safety of the program.

Fixes #1517
  • Loading branch information
@RomainMuller
RomainMuller committed Aug 21, 2020
1 parent 96259b7 commit be2c7e2
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 59 deletions.
32 changes: 21 additions & 11 deletions .github/workflows/main.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,27 +387,37 @@ jobs:
- name: Integration Test (build)
run: |-
npx lerna run build
npx lerna run build --stream 2>&1 > ${{ runner.temp }}/build.log
working-directory: aws-cdk

# In the interest of speed, only process monocdk-experiment / aws-cdk-lib from now on
- name: Integration Test (jsii-rosetta)
run: |-
npx lerna exec --scope=monocdk-experiment --scope=aws-cdk-lib -- \
${ROSETTA} \
--compile \
--output ./dist/samples.tabl.json \
--directory . \
--verbose
npx lerna exec --scope=monocdk-experiment --scope=aws-cdk-lib --stream -- \
${ROSETTA} \
--compile \
--output ./dist/samples.tabl.json \
--directory . \
--verbose \
2>&1 > ${{ runner.temp }}/rosetta.log
working-directory: aws-cdk
- name: Integration Test (jsii-pacmak)
run: |-
npx lerna exec --scope=monocdk-experiment --scope=aws-cdk-lib -- \
${PACMAK} \
--rosetta-tablet ./dist/samples.tabl.json \
--verbose
npx lerna exec --scope=monocdk-experiment --scope=aws-cdk-lib --stream -- \
${PACMAK} \
--rosetta-tablet ./dist/samples.tabl.json \
--verbose \
2>&1 > ${{ runner.temp }}/pacmak.log
working-directory: aws-cdk

- name: Upload Logs
# Upload logs whether successful or failed (not using always because we don't care about cancellations)
if: success() || failure()
uses: actions/upload-artifact@v2
with:
name: integ-test-logs
path: ${{ runner.temp }}/*.log

- name: Upload Result
uses: actions/upload-artifact@v2
with:
Expand Down
143 changes: 105 additions & 38 deletions packages/jsii-pacmak/lib/targets/java.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,9 @@ export default class Java extends Target {
await shell(
'mvn',
[
// If we don't run in verbose mode, turn on quiet mode
...(this.arguments.verbose ? [] : ['--quiet']),
'--batch-mode',
...mvnArguments,
'deploy',
`-D=altDeploymentRepository=local::default::${url}`,
Expand Down Expand Up @@ -477,6 +480,9 @@ interface JavaProp {
// The java type for the property (eg: 'List<String>')
fieldJavaType: string;

// The java type for the parameter (e.g: 'List<? extends SomeType>')
paramJavaType: string;

// The NativeType representation of the property's type
fieldNativeType: string;

Expand Down Expand Up @@ -849,7 +855,6 @@ class JavaGenerator extends Generator {

protected onInterfaceProperty(_ifc: spec.InterfaceType, prop: spec.Property) {
const getterType = this.toDecoratedJavaType(prop);
const setterTypes = this.toDecoratedJavaTypes(prop);
const propName = this.code.toPascalCase(
JavaGenerator.safeJavaPropertyName(prop.name),
);
Expand All @@ -867,6 +872,7 @@ class JavaGenerator extends Generator {
}

if (!prop.immutable) {
const setterTypes = this.toDecoratedJavaTypes(prop);
for (const type of setterTypes) {
this.code.line();
this.addJavaDocs(prop);
Expand Down Expand Up @@ -1186,7 +1192,7 @@ class JavaGenerator extends Generator {

for (const prop of consts) {
const constName = this.renderConstName(prop);
const propType = this.toNativeType(prop.type, true);
const propType = this.toNativeType(prop.type, { forMarshalling: true });
const statement = `software.amazon.jsii.JsiiObject.jsiiStaticGet(${javaClass}.class, "${prop.name}", ${propType})`;
this.code.line(
`${constName} = ${this.wrapCollection(
Expand Down Expand Up @@ -1222,7 +1228,9 @@ class JavaGenerator extends Generator {
overrides = !!prop.overrides,
) {
const getterType = this.toDecoratedJavaType(prop);
const setterTypes = this.toDecoratedJavaTypes(prop);
const setterTypes = this.toDecoratedJavaTypes(prop, {
covariant: prop.static,
});
const propName = this.code.toPascalCase(
JavaGenerator.safeJavaPropertyName(prop.name),
);
Expand Down Expand Up @@ -1251,7 +1259,9 @@ class JavaGenerator extends Generator {
statement = 'this.jsiiGet(';
}

statement += `"${prop.name}", ${this.toNativeType(prop.type, true)})`;
statement += `"${prop.name}", ${this.toNativeType(prop.type, {
forMarshalling: true,
})})`;

this.code.line(
`return ${this.wrapCollection(statement, prop.type, prop.optional)};`,
Expand Down Expand Up @@ -1469,9 +1479,12 @@ class JavaGenerator extends Generator {
nullable: !!property.optional,
fieldName: this.code.toCamelCase(safeName),
fieldJavaType: this.toJavaType(property.type),
paramJavaType: this.toJavaType(property.type, { covariant: true }),
fieldNativeType: this.toNativeType(property.type),
fieldJavaClass: `${this.toJavaType(property.type, true)}.class`,
javaTypes: this.toJavaTypes(property.type),
fieldJavaClass: `${this.toJavaType(property.type, {
forMarshalling: true,
})}.class`,
javaTypes: this.toJavaTypes(property.type, { covariant: true }),
immutable: property.immutable || false,
inherited,
};
Expand Down Expand Up @@ -1525,7 +1538,7 @@ class JavaGenerator extends Generator {
fieldName: this.code.toCamelCase(
JavaGenerator.safeJavaPropertyName(param.name),
),
javaType: this.toJavaType(param.type),
javaType: this.toJavaType(param.type, { covariant: true }),
}));

const builtType = this.toJavaType(cls);
Expand Down Expand Up @@ -1625,7 +1638,9 @@ class JavaGenerator extends Generator {
},
],
};
for (const javaType of this.toJavaTypes(prop.type.spec!)) {
for (const javaType of this.toJavaTypes(prop.type.spec!, {
covariant: true,
})) {
this.addJavaDocs(setter);
this.emitStabilityAnnotations(prop.spec);
this.code.openBlock(
Expand Down Expand Up @@ -1713,10 +1728,23 @@ class JavaGenerator extends Generator {
}
this.code.line(' */');
this.emitStabilityAnnotations(prop.spec);
// We add an explicit cast if both types are generic but they are not identical (one is covariant, the other isn't)
const explicitCast =
type.includes('<') &&
prop.fieldJavaType.includes('<') &&
type !== prop.fieldJavaType
? `(${prop.fieldJavaType})`
: '';
if (explicitCast !== '') {
// We'll be doing a safe, but unchecked cast, so suppress that warning
this.code.line('@SuppressWarnings("unchecked")');
}
this.code.openBlock(
`public ${builderName} ${prop.fieldName}(${type} ${prop.fieldName})`,
);
this.code.line(`this.${prop.fieldName} = ${prop.fieldName};`);
this.code.line(
`this.${prop.fieldName} = ${explicitCast}${prop.fieldName};`,
);
this.code.line('return this;');
this.code.closeBlock();
}
Expand Down Expand Up @@ -1856,8 +1884,11 @@ class JavaGenerator extends Generator {
' * Constructor that initializes the object based on literal property values passed by the {@link Builder}.',
);
this.code.line(' */');
if (props.some((prop) => prop.fieldJavaType !== prop.paramJavaType)) {
this.code.line('@SuppressWarnings("unchecked")');
}
const constructorArgs = props
.map((prop) => `final ${prop.fieldJavaType} ${prop.fieldName}`)
.map((prop) => `final ${prop.paramJavaType} ${prop.fieldName}`)
.join(', ');
this.code.openBlock(
`private ${INTERFACE_PROXY_CLASS_NAME}(${constructorArgs})`,
Expand All @@ -1866,8 +1897,12 @@ class JavaGenerator extends Generator {
'super(software.amazon.jsii.JsiiObject.InitializationMode.JSII);',
);
props.forEach((prop) => {
const explicitCast =
prop.fieldJavaType !== prop.paramJavaType
? `(${prop.fieldJavaType})`
: '';
this.code.line(
`this.${prop.fieldName} = ${_validateIfNonOptional(
`this.${prop.fieldName} = ${explicitCast}${_validateIfNonOptional(
prop.fieldName,
prop,
)};`,
Expand Down Expand Up @@ -2168,8 +2203,11 @@ class JavaGenerator extends Generator {
return this.toJavaType({ fqn: cls.base });
}

private toDecoratedJavaType(optionalValue: spec.OptionalValue): string {
const result = this.toDecoratedJavaTypes(optionalValue);
private toDecoratedJavaType(
optionalValue: spec.OptionalValue,
{ covariant = false } = {},
): string {
const result = this.toDecoratedJavaTypes(optionalValue, { covariant });
if (result.length > 1) {
return `${
optionalValue.optional ? ANN_NULLABLE : ANN_NOT_NULL
Expand All @@ -2178,15 +2216,21 @@ class JavaGenerator extends Generator {
return result[0];
}

private toDecoratedJavaTypes(optionalValue: spec.OptionalValue): string[] {
return this.toJavaTypes(optionalValue.type).map(
private toDecoratedJavaTypes(
optionalValue: spec.OptionalValue,
{ covariant = false } = {},
): string[] {
return this.toJavaTypes(optionalValue.type, { covariant }).map(
(nakedType) =>
`${optionalValue.optional ? ANN_NULLABLE : ANN_NOT_NULL} ${nakedType}`,
);
}

private toJavaType(type: spec.TypeReference, forMarshalling = false): string {
const types = this.toJavaTypes(type, forMarshalling);
private toJavaType(
type: spec.TypeReference,
opts?: { forMarshalling?: boolean; covariant?: boolean },
): string {
const types = this.toJavaTypes(type, opts);
if (types.length > 1) {
return 'java.lang.Object';
}
Expand All @@ -2195,15 +2239,14 @@ class JavaGenerator extends Generator {

private toNativeType(
type: spec.TypeReference,
forMarshalling = false,
recursing = false,
{ forMarshalling = false, covariant = false, recursing = false } = {},
): string {
if (spec.isCollectionTypeReference(type)) {
const nativeElementType = this.toNativeType(
type.collection.elementtype,
const nativeElementType = this.toNativeType(type.collection.elementtype, {
forMarshalling,
true,
);
covariant,
recursing: true,
});
switch (type.collection.kind) {
case spec.CollectionKind.Array:
return `software.amazon.jsii.NativeType.listOf(${nativeElementType})`;
Expand All @@ -2216,27 +2259,30 @@ class JavaGenerator extends Generator {
}
}
return recursing
? `software.amazon.jsii.NativeType.forClass(${this.toJavaType(
type,
? `software.amazon.jsii.NativeType.forClass(${this.toJavaType(type, {
forMarshalling,
)}.class)`
: `${this.toJavaType(type, forMarshalling)}.class`;
covariant,
})}.class)`
: `${this.toJavaType(type, { forMarshalling, covariant })}.class`;
}

private toJavaTypes(
typeref: spec.TypeReference,
forMarshalling = false,
{ forMarshalling = false, covariant = false } = {},
): string[] {
if (spec.isPrimitiveTypeReference(typeref)) {
return [this.toJavaPrimitive(typeref.primitive)];
} else if (spec.isCollectionTypeReference(typeref)) {
return [this.toJavaCollection(typeref, forMarshalling)];
return [this.toJavaCollection(typeref, { forMarshalling, covariant })];
} else if (spec.isNamedTypeReference(typeref)) {
return [this.toNativeFqn(typeref.fqn)];
} else if (typeref.union) {
const types = new Array<string>();
for (const subtype of typeref.union.types) {
for (const t of this.toJavaTypes(subtype, forMarshalling)) {
for (const t of this.toJavaTypes(subtype, {
forMarshalling,
covariant,
})) {
types.push(t);
}
}
Expand All @@ -2247,23 +2293,39 @@ class JavaGenerator extends Generator {

private toJavaCollection(
ref: spec.CollectionTypeReference,
forMarshalling: boolean,
{
forMarshalling,
covariant,
}: { forMarshalling: boolean; covariant: boolean },
) {
const elementJavaType = this.toJavaType(ref.collection.elementtype);
const elementJavaType = this.toJavaType(ref.collection.elementtype, {
covariant,
});
const typeConstraint = covariant
? makeCovariant(elementJavaType)
: elementJavaType;
switch (ref.collection.kind) {
case spec.CollectionKind.Array:
return forMarshalling
? 'java.util.List'
: `java.util.List<${elementJavaType}>`;
: `java.util.List<${typeConstraint}>`;
case spec.CollectionKind.Map:
return forMarshalling
? 'java.util.Map'
: `java.util.Map<java.lang.String, ${elementJavaType}>`;
: `java.util.Map<java.lang.String, ${typeConstraint}>`;
default:
throw new Error(
`Unsupported collection kind: ${ref.collection.kind as any}`,
);
}

function makeCovariant(javaType: string): string {
// Don't emit a covariant expression for String (it's `final` in Java)
if (javaType === 'java.lang.String') {
return javaType;
}
return `? extends ${javaType}`;
}
}

private toJavaPrimitive(primitive: spec.PrimitiveType) {
Expand Down Expand Up @@ -2335,7 +2397,9 @@ class JavaGenerator extends Generator {
statement += `"${method.name}"`;

if (method.returns) {
statement += `, ${this.toNativeType(method.returns.type, true)}`;
statement += `, ${this.toNativeType(method.returns.type, {
forMarshalling: true,
})}`;
} else {
statement += ', software.amazon.jsii.NativeType.VOID';
}
Expand Down Expand Up @@ -2396,10 +2460,13 @@ class JavaGenerator extends Generator {
const params = [];
if (method.parameters) {
for (const p of method.parameters) {
// We can render covariant parameters only for methods that aren't overridable... so only for static methods currently.
params.push(
`final ${this.toDecoratedJavaType(p)}${
p.variadic ? '...' : ''
} ${JavaGenerator.safeJavaPropertyName(p.name)}`,
`final ${this.toDecoratedJavaType(p, {
covariant: (method as spec.Method).static,
})}${p.variadic ? '...' : ''} ${JavaGenerator.safeJavaPropertyName(
p.name,
)}`,
);
}
}
Expand Down
3 changes: 2 additions & 1 deletion packages/jsii-pacmak/lib/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ export async function shell(
new Error(
[
`Command (${command}) failed with ${reason}:`,
prefix(out, '#STDOUT> '),
// STDERR first, the erro message could be truncated in logs.
prefix(err, '#STDERR> '),
prefix(out, '#STDOUT> '),
].join('\n'),
),
);
Expand Down
Loading

0 comments on commit be2c7e2

Please sign in to comment.