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(jsii): unable to use nested types from dependencies #1866

Merged
merged 13 commits into from
Aug 12, 2020

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Aug 10, 2020

Using --project-references, nested types from dependencies could not
be used as they would result in the following jsii error:

Type "<type name>" cannot be used as the return type because it is private or @internal

THis is because in --project-references mode, the ambient declaration
for the nested type is the declaration that gets resolved to, and those
cannot have the export keyword: instead, the surrounding module
declaration is annotated with export ambient.

This adds the necessary code path to actually identify this scenario and
to appropriately detect the type is actually visible and exported.

The check that was failing had been added in #1861 as a way to provide a
more helpful error message when private or @internal types are
mistakenly used on exported APIs; which explains why this situation did
not previously occur.

Hidden gem: we were previously adding the static keyword when
generating nested classes in C#. This is a Java-ism and results
in code that won't compile, since C# does not allow static nested
classes to declare protected members (which we actually emit some
of).


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

When obtained through an untyped way (e.g: as part of an opaque object
for example), instances of `AnonymousObject` could not be cast back to
their Kernel form, because the converter lacked a code path to handle
this type. Instead, it insisted on trying to infer a "better" run-time
type for it.

This adds the necessary code path to handle this condition and properly
return the reverse conversion.

Fixes aws/aws-cdk#7977
Using `--project-references`, nested types from dependencies could not
be used as they would result in the following `jsii` error:

```
Type "<type name>" cannot be used as the return type because it is private or @internal
```

THis is because in `--project-references` mode, the ambient declaration
for the nested type is the declaration that gets resolved to, and those
cannot have the `export` keyword: instead, the surrounding module
declaration is annotated with `export ambient`.

This adds the necessary code path to actually identify this scenario and
to appropriately detect the type is actually visible and exported.

The check that was failing had been added in #1861 as a way to provide a
more helpful error message when private or `@internal` types are
mistakenly used on exported APIs; which explains why this situation did
not previously occur.
@RomainMuller RomainMuller added bug This issue is a bug. effort/small Small work item – less than a day of effort module/compiler Issues affecting the JSII compiler labels Aug 10, 2020
@RomainMuller RomainMuller requested a review from a team August 10, 2020 16:37
@RomainMuller RomainMuller self-assigned this Aug 10, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 10, 2020
@@ -294,7 +293,7 @@ export class DotNetGenerator extends Generator {
this.dotnetRuntimeGenerator.emitAttributesForClass(cls);

this.code.openBlock(
`public${inner}${absPrefix} class ${className}${implementsExpr}`,
`public${absPrefix} class ${className}${implementsExpr}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

C# does not allow static member classes to declare protected members (our contract includes some, so that was bad). The presence of the static keyword here was a Java-ism, since nested classes in C# (with or without static) are actually semantically related to Java static nested classes (the syntactic sugar that Java has for non-static nested classes does not exist in C#).

Copy link
Contributor

Choose a reason for hiding this comment

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

This should have been a separate PR, don'tcha agree?

Copy link
Contributor Author

@RomainMuller RomainMuller Aug 11, 2020

Choose a reason for hiding this comment

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

Except that separate PR's test would not have been able to pass without the other changes in this PR (cannot test I can generate a valid nested class if I can't compile it), and vice-versa (cannot check I generate valid code for a nested class if I generate invalid code for a nested class)...

@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 11, 2020
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Approved but you know you've been a naughty boy and you should really split this up into 2 PRs before you merge it 😎

@RomainMuller
Copy link
Contributor Author

Approved but you know you've been a naughty boy and you should really split this up into 2 PRs before you merge it 😎

How so? I need both changes in order for the test around either to pass...

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 0f211ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@RomainMuller RomainMuller merged commit 44f9109 into master Aug 12, 2020
@RomainMuller RomainMuller deleted the rmuller/fix-nested-foreign-class branch August 12, 2020 07:41
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. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort module/compiler Issues affecting the JSII compiler pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants