fix(java): invalid code when multi-inheriting optional properties #2591
Conversation
The generated code for interfaces includes default implementations for optional properties (since adding a new optional property is a non breaking change in TypeScript, this allows us to preserve this property in Java, too). However, if two distinct super-interfaces declare the same property, the child inherits two unrelated default implementations for the same method, which results in dispatch ambiguity that is illegal. In order to remove the ambiguity, a new default implementation must be provided on the child interface, so that the dispatch is unambiguous again. Fixes #22556
RomainMuller
added
bug
| /** | ||
| */ | ||
| @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Deprecated) | ||
| @Deprecated | ||
| default @org.jetbrains.annotations.Nullable java.lang.String getHoistedTop() { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This right here is what used to not be emitted - leading to DiamondBottom inheriting the default implementation from both DiamondLeft and DiamondRight (both of which hoisted that from their internal parent InternalDiamondTop) -- causing illegal dispatch ambiguity.
New default declared here replaces the parent's, and solves the issue.
| 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.
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.
|
|
||
| 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)) { |
There was a problem hiding this comment.
What's the latter half of this check doing?
There was a problem hiding this comment.
Avoids re-declaring a property that was already overridden locally (that would be illegal).
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
RomainMuller
removed
the
pr/do-not-merge
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
The generated code for interfaces includes default implementations for
optional properties (since adding a new optional property is a non
breaking change in TypeScript, this allows us to preserve this property
in Java, too).
However, if two distinct super-interfaces declare the same property, the
child inherits two unrelated default implementations for the same
method, which results in dispatch ambiguity that is illegal.
In order to remove the ambiguity, a new default implementation must be
provided on the child interface, so that the dispatch is unambiguous
again.
Fixes #22556
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.