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.

Sign up for GitHub

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

Jump to bottom

fix(dotnet): missing ? on nullable interface members #1287

Merged
merged 5 commits into from
Feb 25, 2020
Merged

fix(dotnet): missing ? on nullable interface members #1287

merged 5 commits into from
Feb 25, 2020

Conversation

RomainMuller
Copy link
Contributor

When interfaces declare nullable members, the code generator erroneously omitted the ? that denotes it, while generated implementations of the interface would have it, causing compilation failures.

Fixes #1285

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

@RomainMuller
fix(dotnet): missing ? on nullable interface members
658d2be 
When interfaces declare nullable members, the code generator erroneously omitted the `?` that denotes it, while generated implementations of the interface would have it, causing compilation failures.

Fixes #1285
@RomainMuller
Copy link
Contributor Author

I have locally code-generated for the AWS CDK with this patch enabled, and all modules compiled fine.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 658d2be
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 7166819
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

eladb
eladb approved these changes Feb 24, 2020
View reviewed changes
packages/jsii-pacmak/lib/targets/java.ts
Comment on lines -619 to +620
const getterType = this.toJavaType(prop.type);
const setterTypes = this.toJavaTypes(prop.type);
const getterType = this.toDecoratedJavaType(prop);
const setterTypes = this.toDecoratedJavaTypes(prop);
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know we didn't forget more of these?

@eladb
Copy link
Contributor

eladb commented Feb 24, 2020

This is a potential BREAKING CHANGE in .NET and Java, no?

@RomainMuller
Copy link
Contributor Author

In Java, I don't look at it as breaking (the annotations do not have any intrinsic compilation or runtime behavior. They may trigger linter rules by their presence, but generally speaking this will be in highlight of possible bugs, and shouldn't prevent compiling the code, still).

In DotNet, this may be breaking if the user's project enabled the Nullable feature, and configured to handle warnings as errors. All the nullable reference types do is add meta-information that will be used to emit new warnings if code violates apparent nullability rules. While this could break some user's code, I believe it's a narrow enough niche that it may not be considered breaking still.

@RomainMuller RomainMuller removed the request for review from MrArnoldPalmer February 24, 2020 17:11
@mergify
Copy link
Contributor

mergify bot commented Feb 25, 2020

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 mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Feb 25, 2020
@mergify
Copy link
Contributor

mergify bot commented Feb 25, 2020

Merging (with squash)...

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Feb 25, 2020

Merging (with squash)...

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 9299db2 into master Feb 25, 2020
@mergify mergify bot deleted the rmuller/fix-dotnet-codegen-issue branch February 25, 2020 11:15
@mergify
Copy link
Contributor

mergify bot commented Feb 25, 2020

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladb eladb eladb approved these changes

Assignees

@MrArnoldPalmer MrArnoldPalmer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

jsii 1.0.0 fails aws-cdk build
4 participants
@RomainMuller @aws-cdk-automation @eladb @MrArnoldPalmer