Skip to content
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(kernel): Improve tagged type of wire values #346

Merged
merged 3 commits into from Jan 29, 2019

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Jan 11, 2019

When passing references across the JSII language boundary, the static
return type of a method is used instead of the runtime type of the
object if they are not the same, even if they are compatible. This
causes issues that make it impossible to up-cast from methods such as
IConstruct.findChild that return a super-class that is expected to be
up-casted to it's known dynamic type.

Fixes #345

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


TODO:

  • Add compliance test

When passing references across the JSII language boundary, the static
return type of a method is used instead of the runtime type of the
object if they are not the same, even if they are compatible. This
causes issues that make it impossible to up-cast from methods such as
`IConstruct.findChild` that return a super-class that is expected to be
up-casted to it's known dynamic type.

Fixes #345
@RomainMuller RomainMuller requested a review from a team as a code owner January 11, 2019 16:39
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 11, 2019

I think we need a test

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests at the kernel level and in Java

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 12, 2019

Also don't like how this doesn't cover the intermediate class case. It's not that much more work (as we've discussed via chat) and covers us for future cases where people will be confused, expecting the more correct behavior.

@RomainMuller
Copy link
Contributor Author

But... does this not cover the "more correct behavior"? I don't see the difference... o_O

@RomainMuller
Copy link
Contributor Author

Added the compliance tests (kernel, Java, C#).

packages/jsii-calc/lib/compliance.ts Outdated Show resolved Hide resolved
@RomainMuller RomainMuller merged commit 8ea39ac into master Jan 29, 2019
@RomainMuller RomainMuller deleted the rmuller/issue-345 branch January 29, 2019 08:38
RomainMuller added a commit that referenced this pull request Feb 4, 2019
### Bug Fixes
* remove use of private API ([#351](#351)) ([874cbac](874cbac)), closes [#350](#350)
* **jsii-dotnet-runtime:** Fix EPIPE on Windows. ([1d7cc8b](1d7cc8b)), closes [#341](#341)
* **jsii-dotnet-runtime:** Redirect to STDERR. ([e20f401](e20f401))
* **kernel:** Improve tagged type of wire values ([#346](#346)) ([8ea39ac](8ea39ac)), closes [#345](#345)

### Features
* **jsii:** support multiple class declaration sites ([#348](#348)) ([4ecf28c](4ecf28c))
* Generate NuGet symbol and source packages ([#243](#243)) ([aafd405](aafd405))
@RomainMuller RomainMuller mentioned this pull request Feb 4, 2019
RomainMuller added a commit that referenced this pull request Feb 4, 2019
### Bug Fixes
* remove use of private API ([#351](#351)) ([874cbac](874cbac)), closes [#350](#350)
* **jsii-dotnet-runtime:** Fix EPIPE on Windows. ([1d7cc8b](1d7cc8b)), closes [#341](#341)
* **jsii-dotnet-runtime:** Redirect to STDERR. ([e20f401](e20f401))
* **kernel:** Improve tagged type of wire values ([#346](#346)) ([8ea39ac](8ea39ac)), closes [#345](#345)

### Features
* **jsii:** support multiple class declaration sites ([#348](#348)) ([4ecf28c](4ecf28c))
* Generate NuGet symbol and source packages ([#243](#243)) ([aafd405](aafd405))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants