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

Add support for public properties defined in a private constructor #256

Merged
merged 2 commits into from
Oct 10, 2018

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Oct 9, 2018

This change is incomplete; the .NET generator doesn't know how to
deal with a private ("missing") constructor, so it throws a
NullReferenceException.

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

This change is incomplete; the .NET generator doesn't know how to
deal with a private ("missing") constructor, so it throws a
NullReferenceException.
@rix0rrr rix0rrr requested a review from costleya October 9, 2018 11:16
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 9, 2018

@costleya I'm going to need your help to land this. Constructors can be private, but I think the .NET generator has been written with the assumption that every class will always have a constructor, and so it errors out trying to access it:

lerna ERR! Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
lerna ERR!    at Amazon.JSII.Generator.MethodExtensions.GetParametersJsonSyntaxToken(Method method) in /home/local/ANT/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/MethodExtensions.cs:line 47
lerna ERR!    at Amazon.JSII.Generator.Class.ClassGenerator.<CreateType>g__CreateAttributes|1_0() in /home/local/ANT/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/Class/ClassGenerator.cs:line 33
lerna ERR!    at Amazon.JSII.Generator.Class.ClassGenerator.CreateType() in /home/local/ANT/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/Class/ClassGenerator.cs:line 20
lerna ERR!    at Amazon.JSII.Generator.TypeGeneratorBase`1.CreateSyntaxTree() in /home/local/ANT/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/TypeGeneratorBase.cs:line 35
lerna ERR!    at Amazon.JSII.Generator.AssemblyGenerator.<>c__DisplayClass4_0.<Save>g__SaveType|3(Type type) in /home/local/ANT/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/AssemblyGenerator.cs:line 293
lerna ERR!    at Amazon.JSII.Generator.AssemblyGenerator.Save(String packageOutputRoot, ISymbolMap symbols, Assembly assembly, String tarballFileName, String jsiiFileName) in /home/local/ANT/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/AssemblyGenerator.cs:line 108
lerna ERR!    at Amazon.JSII.Generator.AssemblyGenerator.Generate(String jsiiFile, String tarballPath, ISymbolMap symbols) in /home/local/ANT/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator/AssemblyGenerator.cs:line 59
lerna ERR!    at CommandLine.ParserResultExtensions.WithParsed[T](ParserResult`1 result, Action`1 action)
lerna ERR!    at Amazon.JSII.Generator.CLI.Program.Main(String[] args) in /home/local/ANT/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-dotnet-generator/src/Amazon.JSII.Generator.CLI/Program.cs:line 11

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 9, 2018

Feel free to append to this PR to make it pass.

This was referenced Oct 9, 2018
@eladb
Copy link
Contributor

eladb commented Oct 9, 2018

Can you please open two issues to add a conformance test in each language for this feature?
You can add one for Java like this one:

public void classWithPrivateConstructorAndAutomaticProperties() {
  ClassWithPrivateConstructorAndAutomaticProperties obj = new ClassWithPrivateConstructorAndAutomaticProperties();
  obj.setReadWriteString("Hello");
  assertEquals("<Some expected value>", obj.getReadOnlyString());
}

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 9, 2018

@costleya hope it's not too much work, maybe a couple of ifs?

@costleya
Copy link
Contributor

#259

@eladb This class has no public constructors, so I cannot implement a protocol test as you described. Instead, I used reflection to inspect the class and made sure it looks as expected.

@eladb
Copy link
Contributor

eladb commented Oct 10, 2018

@costleya 🤦‍♂️

@eladb
Copy link
Contributor

eladb commented Oct 10, 2018

@costleya can you push your fixes so we can merge this in and release?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 10, 2018

On it

@rix0rrr rix0rrr merged commit 181012e into master Oct 10, 2018
@rix0rrr rix0rrr deleted the huijbers/private-constructor-autoproperties branch October 10, 2018 07:22
eladb pushed a commit that referenced this pull request Oct 10, 2018
Bug Fixes
=========

* **dotnet:** abstract classes should have proxy implementations ([#241](#241)) ([828a26f](828a26f)), closes [#223](#223)
* **jsii:** better usage reporting of private types ([#247](#247)) ([96ac5d6](96ac5d6))
* **jsii:** support  public autoproperties in private constructor ([#256](#256)) ([181012e](181012e))
* **jsii:** use default jsx compiler options ([#260](#260)) ([660ae79](660ae79)), closes [aws/aws-cdk#830](aws/aws-cdk#830)
* **jsii-dotnet-generator:** Use FQ type returns in conflict. ([#258](#258)) ([a78784a](a78784a)), closes [#252](#252)
* **jsii-runtime:** Use buffer factory methods instead of constructor. ([#246](#246)) ([6ad6b9d](6ad6b9d))
* **kernel:** Return object literals as references ([#249](#249)) ([61cb3a4](61cb3a4)), closes [#248](#248) [aws/aws-cdk#774](aws/aws-cdk#774)

Misc
====

bump.sh was updated to let "lerna publish" automatically determine
the version number using conventional commits.
eladb pushed a commit that referenced this pull request Oct 10, 2018
Bug Fixes
=========

* **dotnet:** abstract classes should have proxy implementations ([#241](#241)) ([828a26f](828a26f)), closes [#223](#223)
* **jsii:** better usage reporting of private types ([#247](#247)) ([96ac5d6](96ac5d6))
* **jsii:** support  public autoproperties in private constructor ([#256](#256)) ([181012e](181012e))
* **jsii:** use default jsx compiler options ([#260](#260)) ([660ae79](660ae79)), closes [aws/aws-cdk#830](aws/aws-cdk#830)
* **jsii-dotnet-generator:** Use FQ type returns in conflict. ([#258](#258)) ([a78784a](a78784a)), closes [#252](#252)
* **jsii-runtime:** Use buffer factory methods instead of constructor. ([#246](#246)) ([6ad6b9d](6ad6b9d))
* **kernel:** Return object literals as references ([#249](#249)) ([61cb3a4](61cb3a4)), closes [#248](#248) [aws/aws-cdk#774](aws/aws-cdk#774)

Misc
====

bump.sh was updated to let "lerna publish" automatically determine
the version number using conventional commits.
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.

3 participants