-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(java): invalid code when multi-inheriting optional properties #2591
Changes from all commits
9ea2aef
7a13ea4
21bfabc
1f94889
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// This is half of the contraption, the rest is in jsii-calc | ||
// | ||
// @see https://github.com/aws/jsii/issues/2256 | ||
|
||
/** | ||
* This struct is intentionally private. Any type that implements it will get | ||
* a copy of it's properties hoisted in by jsii. | ||
*/ | ||
interface InternalDiamondTip { | ||
readonly hoistedTop?: string; | ||
} | ||
|
||
export interface DiamondLeft extends InternalDiamondTip { | ||
readonly left?: number; | ||
} | ||
|
||
export interface DiamondRight extends InternalDiamondTip { | ||
readonly right?: boolean; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { DiamondLeft, DiamondRight } from '@scope/jsii-calc-lib'; | ||
|
||
// This is half of the contraption, the rest is in @scope/jsii-calc-lib. | ||
// | ||
// @see https://github.com/aws/jsii/issues/2256 | ||
|
||
export interface DiamondBottom extends DiamondLeft, DiamondRight { | ||
readonly bottom?: Date; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -833,6 +833,8 @@ class JavaGenerator extends Generator { | |
} | ||
|
||
protected onEndInterface(ifc: spec.InterfaceType) { | ||
this.emitMultiplyInheritedOptionalProperties(ifc); | ||
|
||
if (ifc.datatype) { | ||
this.emitDataType(ifc); | ||
} else { | ||
|
@@ -883,6 +885,9 @@ class JavaGenerator extends Generator { | |
this.addJavaDocs(prop); | ||
this.emitStabilityAnnotations(prop); | ||
if (prop.optional) { | ||
if (prop.overrides) { | ||
this.code.line('@Override'); | ||
} | ||
this.code.openBlock(`default ${getterType} get${propName}()`); | ||
this.code.line('return null;'); | ||
this.code.closeBlock(); | ||
|
@@ -896,6 +901,9 @@ class JavaGenerator extends Generator { | |
this.code.line(); | ||
this.addJavaDocs(prop); | ||
if (prop.optional) { | ||
if (prop.overrides) { | ||
this.code.line('@Override'); | ||
} | ||
this.code.line('@software.amazon.jsii.Optional'); | ||
this.code.openBlock( | ||
`default void set${propName}(final ${type} value)`, | ||
|
@@ -911,6 +919,61 @@ class JavaGenerator extends Generator { | |
} | ||
} | ||
|
||
/** | ||
* Emits a local default implementation for optional properties inherited from | ||
* multiple distinct parent types. This remvoes the default method dispatch | ||
* ambiguity that would otherwise exist. | ||
* | ||
* @param ifc the interface to be processed. | ||
|
||
* | ||
* @see https://github.com/aws/jsii/issues/2256 | ||
*/ | ||
private emitMultiplyInheritedOptionalProperties(ifc: spec.InterfaceType) { | ||
if (ifc.interfaces == null || ifc.interfaces.length <= 1) { | ||
// Nothing to do if we don't have parent interfaces, or if we have exactly one | ||
return; | ||
} | ||
const inheritedOptionalProps = ifc.interfaces | ||
.map(allOptionalProps.bind(this)) | ||
// Calculate how many direct parents brought a given optional property | ||
.reduce((histogram, entry) => { | ||
for (const [name, spec] of Object.entries(entry)) { | ||
histogram[name] = histogram[name] ?? { spec, count: 0 }; | ||
histogram[name].count += 1; | ||
} | ||
return histogram; | ||
}, {} as Record<string, { readonly spec: spec.Property; count: number }>); | ||
|
||
const localProps = new Set(ifc.properties?.map((prop) => prop.name) ?? []); | ||
for (const { spec, count } of Object.values(inheritedOptionalProps)) { | ||
if (count < 2 || localProps.has(spec.name)) { | ||
continue; | ||
} | ||
this.onInterfaceProperty(ifc, spec); | ||
} | ||
|
||
function allOptionalProps(this: JavaGenerator, fqn: string) { | ||
const type = this.findType(fqn) as spec.InterfaceType; | ||
const result: Record<string, spec.Property> = {}; | ||
for (const prop of type.properties ?? []) { | ||
// Adding artifical "overrides" here for code-gen quality's sake. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why artificial? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, the property as we fetch it is not overridden... We actually generate an override, but per the jsii type model, it's only inherited. |
||
result[prop.name] = { ...prop, overrides: type.fqn }; | ||
} | ||
// Include optional properties of all super interfaces in the result | ||
for (const base of type.interfaces ?? []) { | ||
RomainMuller marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for (const [name, prop] of Object.entries( | ||
allOptionalProps.call(this, base), | ||
)) { | ||
if (!(name in result)) { | ||
result[name] = prop; | ||
} | ||
} | ||
} | ||
return result; | ||
} | ||
} | ||
|
||
private emitPackageInfo(mod: spec.Assembly) { | ||
if (!mod.docs) { | ||
return; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the latter half of this check doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoids re-declaring a property that was already overridden locally (that would be illegal).