feat(pacmak/java): emit default interface implementations #2076

RomainMuller · 2020-10-05T12:55:01Z

It should be possible to add properties or methods to interfaces that
are only meant to be returned by APIs (i.e: non-@sublassable) without
breaking extensions of those interfaces in other modules. Unfortunatley,
when such an interface is extended in another module, it's generated
proxy class would previously not have implementations for the new
member, creating the potential for a NoSuchMethodException at runtime.

This PR changes the code generation so that a new Jsii$Default nested
interface is generated for each interface, which contains default
implementations (that forward calls to the jsii kernel) for all of its
members.

Note: members whose name collide with members declared on
java.lang.Object cannot have default implementations, since these
would always be overridden by java.lang.Object implementations.

The new Jsii$Default interface is implemented (and the default
implementations used) by the Jsii$Proxy class. When an interface
extends one or more bases, its Jsii$Default implementation extends
those of it's parent interfaces (and inherits their implementations).

In order to support backwards compatibility with libraries generated
with versions of jsii-pacmak that do not implement this feature, a
new metadata.jsii.pacmak.hasDefaultInterfaces entry is added to the
jsii assembly at compilation time. It's value is used to determine
whether jsii-pacmak can rely on the existence of the Jsii$Default
interface being present.

Finally, new elements were added to the Java runtime library for jsii:

The @Internal annotation is used to designate API elements that are
not meant for public usage
The IJsiiObject interface is the base interface of all
Jsii$Default interfaces, and is implemented by JsiiObject. Since
interface members need be public, the methods declared by this
interface have been maked @Internal, and were renamed from jsiiFoo
to $jsii$foo (in order to completely remove the risk for name
clashes with user-defined APIs)

Fixes #2014

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

When generating interface proxies, and the proxied interface has exactly one super-interface, it is possible for it's proxy to re-use the implementation of the base's proxy by extending it, instead of repeating the whole implementation. Doing so reduces the amount of code that is generated, compiled and released, making it faster to compile, and smaller to download. Additionally this means proxies for interfaces that extend a single foreign (i.e: from a dependency) interface are safer to use (the proxy remains "fully valid" even if the resolved version of the dependency has additional methods or properties). The same approach can unfortunately not be used for abstract class proxies, or for interfaces with more than one super-interface, since Java does not allow multiple inheritance (making it impossible to inherit from more than one base proxy class). Fixes #2014

rix0rrr · 2020-10-05T13:34:07Z

I guess you didn't go default-implementations-in-interfaces on purpose?

Is there still a point to this change if we don't go all the way? Aren't we setting ourselves up to fail in the future, again, just in more complicated cases?

rix0rrr

Okay with suggestion and questions about long-term plans

rix0rrr · 2020-10-05T13:38:50Z

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

+    const ownProps = props.filter(
+      (prop) => baseInterface == null || !prop.inherited,


This is stretching the definition of ownProps somewhat, in my opinion.

I mean, I guess it's "own" for someone (the generated class or the generatee interface), but this is not the someone I would have expected. I would have expected "own" to mean "as declared", not "as generated".

Maybe call it hereProps/thereProps instead? (For funzies)

Or declaredProps/inheritedProps ?

Yeah I guess it's "contextually own props" sort of... Can definitely be misleading when looking only at usage sites...

I'm going for descriptive:

propsToGenerate

propsToInherit

RomainMuller · 2020-10-05T14:21:50Z

I guess you didn't go default-implementations-in-interfaces on purpose?

Is there still a point to this change if we don't go all the way? Aren't we setting ourselves up to fail in the future, again, just in more complicated cases?

"Default implementation in interface" is an interesting approach however it will require introducing a whole additional set of interfaces: if the default behavior on the currently generated interface is "forward call to the jsii kernel", then a user implementing the interface (in Java-land) who "forgot" to implement one method would get a crash on any attempt to call that method (java -> asks kernel to call method -> method does not exist in kernel-owned instance -> 💥).

The current change improves the situation in a good chunk of uses; and which must be done anyway, because I cannot just assume all existing Java interfaces - including from dependencies - have been generated WITH default implementations; and might even be a breaking change if we happen to actually have to emit additional interfaces (cannot assume those are PRESENT in dependencies).

RomainMuller · 2020-10-05T14:35:51Z

After further conversation, even the current change is a breaking change, since this assumes the Jsii$Proxy classes are non-private; which would result in compilation failures when extending interfaces generated with previous jsii-pacmak releases.

So I guess since this must break, we should probably go "all in" with the default implementation-having interfaces... And we'll need to gate this behind a feature flag.

current change would be breaking, cannot ship as-is

It should be possible to add properties or methods to interfaces that are only meant to be returned by APIs (i.e: non-`@sublassable`) without breaking extensions of those interfaces in other modules. Unfortunatley, when such an interface is extended in another module, it's generated proxy class would previously not have implementations for the new member, creating the potential for a `NoSuchMethodException` at runtime. This PR changes the code generation so that a new `Jsii$Default` nested interface is generated for each interface, which contains default implementations (that forward calls to the *jsii kernel*) for all of its members. > *Note:* members whose name collide with members declared on > `java.lang.Object` cannot have `default` implementations, since these > would always be overridden by `java.lang.Object` implementations. The new `Jsii$Default` interface is implemented (and the default implementations used) by the `Jsii$Proxy` class. When an interface extends one or more bases, its `Jsii$Default` implementation extends those of it's parent interfaces (and inherits their implementations). In order to support backwards compatibility with libraries generated with versions of `jsii-pacmak` that do not implement this feature, a new `metadata.jsii.pacmak.hasDefaultInterfaces` entry is added to the *jsii assembly* at compilation time. It's value is used to determine whether `jsii-pacmak` can rely on the existence of the `Jsii$Default` interface being present. Finally, new elements were added to the Java runtime library for jsii: - The `@Internal` annotation is used to designate API elements that are not meant for public usage - The `IJsiiObject` interface is the base interface of all `Jsii$Default` interfaces, and is implemented by `JsiiObject`. Since interface members need be `public`, the methods declared by this interface have been maked `@Internal`, and were renamed from `jsiiFoo` to `$jsii$foo` (in order to completely remove the risk for name clashes with user-defined APIs) Fixes #2014

rix0rrr

Love the metadata annotation solution!

One question: the devil is usually in the transitive dependencies. Will the following daisy-chain of linking work properly?

cdk-without-$Default
used by
library-with-$Default
used by
library-with-$Default

I'll trust you on the implementation front :)

packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/IJsiiObject.java

packages/@scope/jsii-calc-base-of-base/test/assembly.jsii

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

packages/jsii/lib/project-info.ts

rix0rrr

Provisional approval after some requested deep soul searching :P.

Also:

RomainMuller · 2020-10-19T10:15:31Z

And so with respects to the "daisy chaining", I believe I have tested this to work properly. The usage of the interfaces is conditioned to the dependency that owns a class having the flag... Worst case scenario we'll be generating sub-par code if the dependency closure is "unfortunate" here; but it should work (at least - as good as it did before this change, no worse).

rix0rrr · 2020-10-19T12:08:58Z

Not too keen on the $jsii$get etc. appearing in the autocomplete of every single object. I'd rather have those as static methods somewhere. (#2076 (comment))

…inheritance

RomainMuller · 2020-10-21T16:54:28Z

Not too keen on the $jsii$get etc. appearing in the autocomplete of every single object. I'd rather have those as static methods somewhere. (#2076 (comment))

Good idea! Implemented!

mergify · 2020-10-21T17:31:14Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

mergify · 2020-10-21T17:31:25Z

Merging (with squash)...

aws-cdk-automation · 2020-10-21T17:48:53Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: 8e96bb0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-10-21T18:34:25Z

Merging (with squash)...

RomainMuller added language/java Related to Java bindings effort/small Small work item – less than a day of effort module/pacmak Issues affecting the `jsii-pacmak` module labels Oct 5, 2020

RomainMuller requested a review from a team October 5, 2020 12:55

RomainMuller self-assigned this Oct 5, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 5, 2020

Merge branch 'master' into rmuller/java-proxy-inheritance

07e096d

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Oct 5, 2020

rix0rrr previously approved these changes Oct 5, 2020

View reviewed changes

@rix0rrr suggestion

bf17994

RomainMuller added pr/do-not-merge This PR should not be merged at this time. and removed pr/do-not-merge This PR should not be merged at this time. labels Oct 5, 2020

RomainMuller marked this pull request as draft October 5, 2020 14:38

RomainMuller changed the title ~~feat(pacmak/java): emitted proxy types extend base where possible~~ feat(pacmak/java): emit default interface implementations Oct 8, 2020

RomainMuller marked this pull request as ready for review October 8, 2020 09:58

Merge branch 'master' into rmuller/java-proxy-inheritance

e41eda4

RomainMuller requested a review from rix0rrr October 8, 2020 09:58

RomainMuller removed the pr/do-not-merge This PR should not be merged at this time. label Oct 8, 2020

fixup test expectations

55476a4

RomainMuller assigned rix0rrr Oct 14, 2020

Merge branch 'master' into rmuller/java-proxy-inheritance

546c0b2

rix0rrr reviewed Oct 19, 2020

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Oct 19, 2020

rix0rrr approved these changes Oct 19, 2020

View reviewed changes

RomainMuller added effort/large Large work item – several weeks of effort and removed effort/small Small work item – less than a day of effort labels Oct 19, 2020

RomainMuller added 3 commits October 19, 2020 12:15

Merge branch 'master' into rmuller/java-proxy-inheritance

19f89db

mark IJsiiObject methods as @internal

1bd9bfe

Mark JsiiSerializable method @internal

6700973

RomainMuller added 2 commits October 21, 2020 18:19

Merge remote-tracking branch 'origin/master' into rmuller/java-proxy-…

8c069d6

…inheritance

Remodel default implementations using static methods on JsiiObject.

b59c0b4

RomainMuller force-pushed the rmuller/java-proxy-inheritance branch from 0bcce0d to b59c0b4 Compare October 21, 2020 16:20

RomainMuller removed the pr/do-not-merge This PR should not be merged at this time. label Oct 21, 2020

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Oct 21, 2020

Merge branch 'master' into rmuller/java-proxy-inheritance

8e96bb0

mergify bot merged commit c618de3 into master Oct 21, 2020

mergify bot deleted the rmuller/java-proxy-inheritance branch October 21, 2020 18:34

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pacmak/java): emit default interface implementations #2076

feat(pacmak/java): emit default interface implementations #2076

RomainMuller commented Oct 5, 2020 •

edited

Loading

rix0rrr commented Oct 5, 2020 •

edited

Loading

rix0rrr left a comment

rix0rrr Oct 5, 2020

RomainMuller Oct 5, 2020 •

edited

Loading

RomainMuller commented Oct 5, 2020

RomainMuller commented Oct 5, 2020

rix0rrr left a comment

rix0rrr left a comment

RomainMuller commented Oct 19, 2020

rix0rrr commented Oct 19, 2020

RomainMuller commented Oct 21, 2020

mergify bot commented Oct 21, 2020

mergify bot commented Oct 21, 2020

aws-cdk-automation commented Oct 21, 2020

mergify bot commented Oct 21, 2020

		const ownProps = props.filter(
		(prop) => baseInterface == null \|\| !prop.inherited,

feat(pacmak/java): emit default interface implementations #2076

feat(pacmak/java): emit default interface implementations #2076

Conversation

RomainMuller commented Oct 5, 2020 • edited Loading

rix0rrr commented Oct 5, 2020 • edited Loading

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Oct 5, 2020

Choose a reason for hiding this comment

RomainMuller Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

RomainMuller commented Oct 5, 2020

RomainMuller commented Oct 5, 2020

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

RomainMuller commented Oct 19, 2020

rix0rrr commented Oct 19, 2020

RomainMuller commented Oct 21, 2020

mergify bot commented Oct 21, 2020

mergify bot commented Oct 21, 2020

aws-cdk-automation commented Oct 21, 2020

AWS CodeBuild CI Report

mergify bot commented Oct 21, 2020

RomainMuller commented Oct 5, 2020 •

edited

Loading

rix0rrr commented Oct 5, 2020 •

edited

Loading

RomainMuller Oct 5, 2020 •

edited

Loading