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

kernel: return type for unexported types #345

Closed
rix0rrr opened this issue Jan 11, 2019 · 2 comments · Fixed by #352
Closed

kernel: return type for unexported types #345

rix0rrr opened this issue Jan 11, 2019 · 2 comments · Fixed by #352
Assignees
Labels
bug This issue is a bug.

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 11, 2019

Someone reported a failing case over email, where we were returning a Construct to Java, which should have been a particular L1 class.

Key insight: the type we construct a proxy for depends on the requested type. Compliance test:

export class PublicClass {
    hello(): void;
}

export interface IPublicInterface {
    bye(): void;
}

export class InbetweenClass extends PublicClass {
}

class PrivateClass extends InbetweenClass implements IPublicInterface {
    public hello() {
        process.stdout.write('Hello\n');
    }

    public bye() {
        process.stdout.write('Bye\n');
    }
}

export class Constructors {
    public static makeClass(): PublicClass {
        return new PrivateClass();
    }

    public static makeInterface(): IPublicInterface {
        return new PrivateClass();
    }
}

Given the same object instance:

  • Constructors.makeClass() should return a proxy object for InbetweenClass.
  • Constructors.makeInterface() should return a proxy object for IPublicInterface.

Given:

  • The actual type (PrivateClass)
  • The requested type (PublicClass vs. IPublicInterface)
  • The inheritance hierarchy

We should be returning the most specific type on the inheritance path between actual and requested that is itself exported.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jan 11, 2019

An issue implementing this might be that the kernel does not have type information on unexported types, so it will only have:

  • Requested type PublicClass
  • Actual type PrivateClass

And it for sure cannot return PrivateClass to a JSII consumer.

@RomainMuller
Copy link
Contributor

I reckon https://github.com/awslabs/jsii/blob/0301e305aab027f6350b3051a5d2b08b6fe7cbb5/packages/jsii-kernel/lib/kernel.ts#L964 should be changed so it accepts to return the current runtime type if it is assignable to the requested type. That would widely increase the scope of what can be down-casted in languages like Java.

RomainMuller added a commit that referenced this issue 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
@RomainMuller RomainMuller added the bug This issue is a bug. label Jan 11, 2019
@RomainMuller RomainMuller self-assigned this Jan 11, 2019
RomainMuller added a commit that referenced this issue Jan 29, 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
RomainMuller added a commit that referenced this issue 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 issue Feb 4, 2019
RomainMuller added a commit that referenced this issue 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
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants