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

feat(jsii): prohibit changing visibility when overriding #1876

Merged
merged 2 commits into from
Aug 12, 2020

Conversation

RomainMuller
Copy link
Contributor

Since C# does not allow changing the visibility of a member when
overriding it, added a check to ensure nobody attempts to make a
protected member public when overriding it.

This is a somewhat common pitfall for users (as seen for example in
aws/aws-cdk#9616), and the check will allow faster detection and
correction (i.e: at compilation time instead of during jsii-pacmak).


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

Since C# does not allow changing the visibility of a member when
overriding it, added a check to ensure nobody attempts to make a
protected member public when overriding it.

This is a somewhat common pitfall for users (as seen for example in
aws/aws-cdk#9616), and the check will allow faster detection and
correction (i.e: at compilation time instead of during `jsii-pacmak`).
@RomainMuller RomainMuller added feature-request A feature should be added or improved. contribution/core This is a PR that came from AWS. module/compiler Issues affecting the JSII compiler labels Aug 12, 2020
@RomainMuller RomainMuller requested a review from a team August 12, 2020 08:13
@RomainMuller RomainMuller self-assigned this Aug 12, 2020
@RomainMuller RomainMuller added the effort/small Small work item – less than a day of effort label Aug 12, 2020
@RomainMuller RomainMuller changed the title feat(jsii): detect changing visibility when overriding feat(jsii): prohibit changing visibility when overriding Aug 12, 2020
@mergify
Copy link
Contributor

mergify bot commented Aug 12, 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 Aug 12, 2020
@mergify
Copy link
Contributor

mergify bot commented Aug 12, 2020

Merging (with squash)...

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@RomainMuller RomainMuller merged commit cfc99c2 into master Aug 12, 2020
@RomainMuller RomainMuller deleted the rmuller/prohibit-visibility-changes-in-overrides branch August 12, 2020 11:51
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Aug 12, 2020
skorfmann added a commit to hashicorp/terraform-cdk that referenced this pull request Aug 19, 2020
In the recent Jsii release https://github.com/aws/jsii/releases/tag/v1.11.0 a detection for
changing visibility when overriding was added aws/jsii#1876 and prevents
it from building.
anubhavmishra pushed a commit to hashicorp/terraform-cdk that referenced this pull request Aug 20, 2020
* Don't change visibility for "synthesizeAttributes"

In the recent Jsii release https://github.com/aws/jsii/releases/tag/v1.11.0 a detection for
changing visibility when overriding was added aws/jsii#1876 and prevents
it from building.

* Update snapshots to reflect the changed visibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. module/compiler Issues affecting the JSII compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants