Skip to content

Commit 018396e

Browse files
rix0rrrrix0rrr
authored
fix(pacmak): generates too many methods for Java (#5007)
When pacmak is generating Java code, it is generating `Jsii$Default` interfaces and `Jsii$Proxy` classes; these have default implementations that call out to the jsii kernel for every property and method. Because all `Jsii$Default` interfaces already have default implementations for every method to call out to jsii, any methods that are inherited from these default interfaces don't need to be inherited on `$Proxy` and `$Default` subtypes. The logic for this was incorrect, and didn't cut out as many types as it could. Example: ```ts interface ISuper { public readonly super: string; } interface ISub extends ISuper { public readonly sub: string; } ``` Leads to (roughly) the following Java code: ```java interface ISuper { String getSuper(); interface Jsii$Default extends ISuper { String getSuper() { return /* call jsii kernel here */ } } } interface ISub extends ISuper { String getSub(); interface Jsii$Default extends ISub, ISuper.Jsii$Default { String getSuper() { // <--- ❌ doesn't need to be here! return /* call jsii kernel here */ } String getSub() { return /* call jsii kernel here */ } } } ``` The logic that decided whether or not to render methods made two mistakes: - `abstract`: abstract members need to be rendered, but only for classes. Interface members are always abstract, but also will have a default implementation on the `$Default` interface already. - The check was using `member.parentType`, but should have been using `member.definingType` instead: `parentType` is *always* the current type we're rendering members for. The upshot of not rendering unnecessary members is that if we happen to backwards incompatibly break some upstream interface (😇) the upstream implementation can just be inherited instead of re-rendered. This improves compatibility across versions, and makes it easier to roll out breaking changes like this. Unfortunately there was also a bug in the original implementation which led it to generating too *few* members -- method overloads for optional arguments were not being generated into the `Jsii$Default` implementation. So we still need to generate methods for overloads in the subtypes. We can only drop the generation of these overloads when the lowest version of the jsii package that a library can be used in conjunction with definitely has them already, and I'm not sure we can know that in advance (only if users set up the version combination correctly, which we can't really control). Do the cleanup of that later. There logic of rendering optional properties that are multiply inherited was also overzealous: it was not figuring out that a single implementation inherited via different parents didn't need to be duplicated; and in fact said it was doing only optional properties, but was actually doing all properties. --- 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 29b29de commit 018396e

3 files changed

Lines changed: 267 additions & 1561 deletions

File tree

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

Lines changed: 135 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,15 @@ const ANN_NOT_NULL = '@org.jetbrains.annotations.NotNull';
4646
const ANN_NULLABLE = '@org.jetbrains.annotations.Nullable';
4747
const ANN_INTERNAL = '@software.amazon.jsii.Internal';
4848

49+
/**
50+
* Because of a historical bug, $Default interfaces we inherit from might not
51+
* have all method overloads generated correctly.
52+
*
53+
* So when inheriting these, we might need to generate the overloads in
54+
* subinterfaces/subclasses.
55+
*/
56+
const GENERATE_POTENTIALLY_MISING_DEFAULT_OVERLOADS = true;
57+
4958
/**
5059
* Build Java packages all together, by generating an aggregate POM
5160
*
@@ -884,7 +893,7 @@ class JavaGenerator extends Generator {
884893
}
885894

886895
protected onEndInterface(ifc: spec.InterfaceType) {
887-
this.emitMultiplyInheritedOptionalProperties(ifc);
896+
this.emitMultiplyInheritedProperties(ifc);
888897

889898
if (ifc.datatype) {
890899
this.emitDataType(ifc);
@@ -1016,60 +1025,40 @@ class JavaGenerator extends Generator {
10161025
}
10171026

10181027
/**
1019-
* Emits a local default implementation for optional properties inherited from
1020-
* multiple distinct parent types. This remvoes the default method dispatch
1021-
* ambiguity that would otherwise exist.
1028+
* Emits a local default implementation for properties inherited from multiple
1029+
* distinct parent types. This removes the default method dispatch ambiguity
1030+
* that would otherwise exist.
10221031
*
10231032
* @param ifc the interface to be processed.
10241033
10251034
*
10261035
* @see https://github.com/aws/jsii/issues/2256
10271036
*/
1028-
private emitMultiplyInheritedOptionalProperties(ifc: spec.InterfaceType) {
1037+
private emitMultiplyInheritedProperties(ifc: spec.InterfaceType) {
10291038
if (ifc.interfaces == null || ifc.interfaces.length <= 1) {
10301039
// Nothing to do if we don't have parent interfaces, or if we have exactly one
10311040
return;
10321041
}
1033-
const inheritedOptionalProps = ifc.interfaces
1034-
.map(allOptionalProps.bind(this))
1035-
// Calculate how many direct parents brought a given optional property
1036-
.reduce(
1037-
(histogram, entry) => {
1038-
for (const [name, spec] of Object.entries(entry)) {
1039-
histogram[name] = histogram[name] ?? { spec, count: 0 };
1040-
histogram[name].count += 1;
1041-
}
1042-
return histogram;
1043-
},
1044-
{} as Record<string, { readonly spec: spec.Property; count: number }>,
1045-
);
10461042

1047-
const localProps = new Set(ifc.properties?.map((prop) => prop.name) ?? []);
1048-
for (const { spec, count } of Object.values(inheritedOptionalProps)) {
1049-
if (count < 2 || localProps.has(spec.name)) {
1050-
continue;
1043+
const memberSources: Record<string, Record<string, spec.Property>> = {};
1044+
for (const parent of ifc.interfaces) {
1045+
const type = this.reflectAssembly.system.findInterface(parent);
1046+
for (const prop of type.allProperties) {
1047+
if (!(prop.name in memberSources)) {
1048+
memberSources[prop.name] = {};
1049+
}
1050+
memberSources[prop.name][prop.definingType.fqn] = prop.spec;
10511051
}
1052-
this.onInterfaceProperty(ifc, spec);
10531052
}
10541053

1055-
function allOptionalProps(this: JavaGenerator, fqn: string) {
1056-
const type = this.findType(fqn) as spec.InterfaceType;
1057-
const result: Record<string, spec.Property> = {};
1058-
for (const prop of type.properties ?? []) {
1059-
// Adding artifical "overrides" here for code-gen quality's sake.
1060-
result[prop.name] = { ...prop, overrides: type.fqn };
1061-
}
1062-
// Include optional properties of all super interfaces in the result
1063-
for (const base of type.interfaces ?? []) {
1064-
for (const [name, prop] of Object.entries(
1065-
allOptionalProps.call(this, base),
1066-
)) {
1067-
if (!(name in result)) {
1068-
result[name] = prop;
1069-
}
1070-
}
1054+
for (const defininingTypes of Object.values(memberSources)) {
1055+
// Ignore our own type
1056+
delete defininingTypes[ifc.fqn];
1057+
1058+
const keys = Object.keys(defininingTypes);
1059+
if (keys.length > 1) {
1060+
this.onInterfaceProperty(ifc, defininingTypes[keys[0]]);
10711061
}
1072-
return result;
10731062
}
10741063
}
10751064

@@ -1959,10 +1948,12 @@ class JavaGenerator extends Generator {
19591948
);
19601949
this.code.line(' */');
19611950

1951+
// Get the list of $Default interfaces
19621952
const baseInterfaces = this.defaultInterfacesFor(type, {
19631953
includeThisType: true,
19641954
});
19651955

1956+
// Add ourselves if we don't have a $Default interface
19661957
if (type.isInterfaceType() && !hasDefaultInterfaces(type.assembly)) {
19671958
// Extend this interface directly since this module does not have the Jsii$Default
19681959
baseInterfaces.push(this.toNativeFqn(type.fqn));
@@ -1989,13 +1980,7 @@ class JavaGenerator extends Generator {
19891980
this.code.closeBlock();
19901981

19911982
// emit all properties
1992-
for (const reflectProp of type.allProperties.filter(
1993-
(prop) =>
1994-
prop.abstract &&
1995-
(prop.parentType.fqn === type.fqn ||
1996-
prop.parentType.isClassType() ||
1997-
!hasDefaultInterfaces(prop.assembly)),
1998-
)) {
1983+
for (const reflectProp of type.allProperties.filter(needsProxyImpl)) {
19991984
const prop = clone(reflectProp.spec);
20001985
prop.abstract = false;
20011986
// Emitting "final" since this is a proxy and nothing will/should override this
@@ -2006,25 +1991,13 @@ class JavaGenerator extends Generator {
20061991
}
20071992

20081993
// emit all the methods
2009-
for (const reflectMethod of type.allMethods.filter(
2010-
(method) =>
2011-
method.abstract &&
2012-
(method.parentType.fqn === type.fqn ||
2013-
method.parentType.isClassType() ||
2014-
!hasDefaultInterfaces(method.assembly)),
1994+
for (const reflectMethod of type.allMethods.flatMap(
1995+
this.makeProxyImpls.bind(this),
20151996
)) {
2016-
const method = clone(reflectMethod.spec);
2017-
method.abstract = false;
1997+
const method = clone(reflectMethod);
20181998
// Emitting "final" since this is a proxy and nothing will/should override this
1999+
method.abstract = false;
20192000
this.emitMethod(type.spec, method, { final: true, overrides: true });
2020-
2021-
for (const overloadedMethod of this.createOverloadsForOptionals(method)) {
2022-
overloadedMethod.abstract = false;
2023-
this.emitMethod(type.spec, overloadedMethod, {
2024-
final: true,
2025-
overrides: true,
2026-
});
2027-
}
20282001
}
20292002

20302003
this.code.closeBlock();
@@ -2046,29 +2019,18 @@ class JavaGenerator extends Generator {
20462019
.join(', ')}`,
20472020
);
20482021

2049-
for (const property of type.allProperties.filter(
2050-
(prop) =>
2051-
prop.abstract &&
2052-
// Only checking the getter - java.lang.Object has no setters.
2053-
!isJavaLangObjectMethodName(`get${jsiiToPascalCase(prop.name)}`) &&
2054-
(prop.parentType.fqn === type.fqn ||
2055-
!hasDefaultInterfaces(prop.assembly)),
2056-
)) {
2022+
for (const property of type.allProperties.filter(needsDefaultImpl)) {
20572023
this.emitProperty(type.spec, property.spec, property.definingType.spec, {
20582024
defaultImpl: true,
20592025
overrides: type.isInterfaceType(),
20602026
});
20612027
}
2062-
for (const method of type.allMethods.filter(
2063-
(method) =>
2064-
method.abstract &&
2065-
!isJavaLangObjectMethodName(method.name) &&
2066-
(method.parentType.fqn === type.fqn ||
2067-
!hasDefaultInterfaces(method.assembly)),
2028+
for (const method of type.allMethods.flatMap(
2029+
this.makeDefaultImpls.bind(this),
20682030
)) {
2069-
this.emitMethod(type.spec, method.spec, {
2031+
this.emitMethod(type.spec, method, {
20702032
defaultImpl: true,
2071-
overrides: type.isInterfaceType(),
2033+
overrides: true,
20722034
});
20732035
}
20742036
this.code.closeBlock();
@@ -3388,6 +3350,62 @@ class JavaGenerator extends Generator {
33883350
return moduleClass;
33893351
}
33903352

3353+
/**
3354+
* Given a method, return the methods that we should generate implementations for on the $Default interface
3355+
*
3356+
* This can be 0..N:
3357+
*
3358+
* - 0: if the method can be inherited from a parent $Default implementation
3359+
* - 1: if the method cannot be inherited from a parent $Default implementation
3360+
* - N: ah-ha, wait! There can be overloads! And because of a historical bug,
3361+
* we didn't use to generate overloads onto $Default interfaces. So it's possible
3362+
* that we don't generate the "main" implementation, but we do generate its overloads.
3363+
*
3364+
* Technically speaking we only have to account for the bug if the type is from a different
3365+
* assembly (because we know all types from the current assembly will be generated ✨ bugless ✨,
3366+
* but just to keep it simple we'll always do the same thing).
3367+
*
3368+
* We can only get rid of this bug once the oldest dependency package a Java
3369+
* package can be used with definitely has overloaded $Default impls. So that will be a while.
3370+
*/
3371+
private makeDefaultImpls(m: reflect.Method): spec.Method[] {
3372+
const ret: spec.Method[] = [];
3373+
if (needsDefaultImpl(m)) {
3374+
ret.push(m.spec);
3375+
}
3376+
3377+
// Account for a past bug
3378+
if (needsDefaultImpl(m) || GENERATE_POTENTIALLY_MISING_DEFAULT_OVERLOADS) {
3379+
ret.push(...this.createOverloadsForOptionals(m.spec));
3380+
}
3381+
3382+
return ret;
3383+
}
3384+
3385+
/**
3386+
* Given a method, return the methods that we should generate implementations for on the $Proxy class
3387+
*
3388+
* See `makeDefaultImpls` for the main rationale behind this. The $Proxy class inherits from $Default
3389+
* so technically this could have usually been empty, but we need to account for the possibility that
3390+
* we implement a $Default interface from another assembly that has been generated with a buggy version
3391+
* of pacmak.
3392+
*/
3393+
private makeProxyImpls<A extends reflect.Method | reflect.Property>(
3394+
m: A,
3395+
): Array<A['spec']> {
3396+
const ret: spec.Method[] = [];
3397+
if (needsProxyImpl(m)) {
3398+
ret.push(m.spec);
3399+
}
3400+
3401+
// Account for a past bug
3402+
if (needsProxyImpl(m) || GENERATE_POTENTIALLY_MISING_DEFAULT_OVERLOADS) {
3403+
ret.push(...this.createOverloadsForOptionals(m.spec));
3404+
}
3405+
3406+
return ret;
3407+
}
3408+
33913409
private emitJsiiInitializers(cls: spec.ClassType) {
33923410
this.code.line();
33933411
this.code.openBlock(
@@ -4041,3 +4059,41 @@ async function resolveMavenVersions(directory: string) {
40414059
},
40424060
);
40434061
}
4062+
4063+
/**
4064+
* Whether the given property or method needs to be implemented on a $Proxy class
4065+
*
4066+
* Proxies extend the class they're for (if for a class), and the $Default interfaces
4067+
* of all the base interfaces, so implementations need to be present for everything
4068+
* that is not abstract and can be inherited from a $Default interface.
4069+
*/
4070+
function needsProxyImpl(x: reflect.Property | reflect.Method) {
4071+
// Interface members are always marked 'abstract', but we only need to
4072+
// implement them if they come from a class (because interface members
4073+
// will have a $Default impl that calls out to jsii already).
4074+
const isAbstractClassMember = x.definingType.isClassType() && x.abstract;
4075+
return (
4076+
isAbstractClassMember || !hasDefaultInterfaces(x.definingType.assembly)
4077+
);
4078+
}
4079+
4080+
/**
4081+
* Whether the given property or method needs to be implemented on a $Default interface
4082+
*
4083+
* $Default interfaces extend the interface they're for, and the $Default interfaces
4084+
* of all the base interfaces, so implementations need to be present for everything
4085+
* that is defined on the current interface or cannot be inherited from a $Default interface.
4086+
*/
4087+
function needsDefaultImpl(x: reflect.Property | reflect.Method) {
4088+
const isBuiltinMethod =
4089+
x instanceof reflect.Property
4090+
? // Only checking the getter - java.lang.Object has no setters.
4091+
isJavaLangObjectMethodName(`get${jsiiToPascalCase(x.name)}`)
4092+
: isJavaLangObjectMethodName(x.name);
4093+
4094+
return (
4095+
(!hasDefaultInterfaces(x.definingType.assembly) ||
4096+
x.definingType.fqn === x.parentType.fqn) &&
4097+
!isBuiltinMethod
4098+
);
4099+
}

‎packages/jsii-pacmak/test/generated-code/__snapshots__/examples.test.js.snap‎

Lines changed: 0 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)