fix(dotnet): Use nested classes for proxies to avoid name collision

[jsteinich]

Fixes #2367

C# proxy classes now nested in associated type using the name Jsii_Proxy. Using non-standard _ in naming to further reduce chance of collision.

I would like to do some more testing since I'm not familiar with the details of how the proxies work.

---

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

[RomainMuller]

Oh this is neat! I wonder if we shouldn't simply name the proxy type _Proxy... The _ alone should guarantee there is no collision (this prefix - depending on the case - either signifies declarations are hidden from jsii, or is illegal).

Besides this, we'll need to dutifully make sure this is not a breaking change to existing users...

[jsteinich]

Besides this, we'll need to dutifully make sure this is not a breaking change to existing users...

Since the proxy class is an internal type, it would only be directly usable via reflection, which should reduce the chance of >breakage.
Since users could be committing generated code, should definitely call out the change though.

You're correct... I guess there isn't much to worry about in this case (we cannot support people using internal declarations using reflection anyway, or we may never change anything...).

I saw some references to the aws cdk in tests; does it run a full build of that? Or is there a good way to test things out against the cdk?

The integration test run on GitHub workflows indeed runs a full build and generation of the CDK (actually - now only monocdk-example/aws-cdk-lib are code-generated for other languages, in the interest of speed), and provides all packages (in all languages) as a GitHub Artefact. Those can then be used to manually verify nothing unexpected happens.

[RomainMuller]

@all-contributors add @jsteinich for code

[allcontributors]

@RomainMuller

I've put up a pull request to add @jsteinich! 🎉

[RomainMuller]

(I like this, marked it as pr/do-not-merge so I have a chance to inspect the generated CDK artefacts before shipping this for realz)

You may want to make the PR "ready for review" (as far as I'm concerned, it is...).
I could do this on your behalf but I tend to think of it as overstepping a little (you're the one who should decide if you think it's ready or not).

[jsteinich]

You may want to make the PR "ready for review" (as far as I'm concerned, it is...).

I've taken some more time to understand what tests exist and how the proxies are used, so I'll go ahead and update.

The one not quite ideal piece is some warnings because of the new keyword the proxy class. It's there because otherwise subtypes of types with proxies will warn the other way. It seems possible to loop through base classes / implemented interfaces to determine if it is necessary, but that doesn't feel very clean or reliable.

[RomainMuller]

The one not quite ideal piece is some warnings because of the new keyword the proxy class. It's there because otherwise subtypes of types with proxies will warn the other way. It seems possible to loop through base classes / implemented interfaces to determine if it is necessary, but that doesn't feel very clean or reliable.

I think we should be able to get a relatively clean signal - if any type in the inheritance chain is from the same library and has a proxy, then we add the new modifier, otherwise not. We could also disable that warning (but I feel this is slightly worse overall).

[RomainMuller]

Alright I have added some logic to only emit the new modifier when it's actually necessary, and I've asked the compiler to keep me honest on that front (turning the new warnings into errors).

Pull request has been modified.

[mergify]

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]

Merging (with squash)...

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify]

Merging (with squash)...