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

Added CSharp13 Support #6130

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Added CSharp13 Support #6130

wants to merge 4 commits into from

Conversation

dersia
Copy link

@dersia dersia commented Feb 12, 2025

This PR adds CSharp13 as another language.

This will have the following effects:

this also fixes: #3944

Different than the PR #3945 this keeps backward compatibility for net framework and older C# versions.
(@0xced thanks for the PR it inspired me to this PR)

It also allows us to incorporate newer C# feature in the future when generating C# code.

@dersia dersia requested a review from a team as a code owner February 12, 2025 01:26
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I don't think we'll be able to accept this pull request. Fundamentally this is because it doubles up the support cost for C#.

When we started kiota, over 4 years ago now. The dotnet landscape was really different:

Now the situation is different:

  • only net core 8 and up are supported
  • only net fx 4.6.2 and up are supported
  • in about 8 months only VS 2017 and up will be supported (2025 still has about 8 months of support remaining)
  • about 1% of new projects are still created targeting netfx (but there's a world of existing projects being maintained)

The main reason why we (the kiota team) decided to support an older version of the language was so that large audience who bought and ran products that are still support didn't get left behind.
Now, that audience has reduced drastically, and VS2019 supports targeting net5 and up so I think it'd be time to re-evaluate and potentially drop support for net framework. Which would unlock all modern language features.
To be clear, this is my personal opinion, which still needs to go through management approval, funding, etc... And when this gets approved, we'll need to major rev kiota in the process as per our support policy

I hope this explanation shine lights on the current "status quo".
Let us know if you have any additional comments or questions.

@darrelmiller
Copy link
Member

@baywet What could we do to enable people like @dersia without taking on additional maintenance in the core?
I guess one option is a fork, but that makes it difficult for community extensions to be deployed and distributed without creating a whole new parallel distribution. And that will cause customer confusion for everyone.

We should revisit the notion of pluggable language support and see if the .net ecosystem has better options/solutions than it did 4 years ago.

I do agree though that having two C# variants in the core of Kiota is problematic from a customer adoption and support perspective.

@baywet
Copy link
Member

baywet commented Feb 12, 2025

@darrelmiller regardless of the module loading approach and its ergonomic/security/discoverability considerations for the end user, this approach will also bring significant additional maintenance costs, I'd argue they are greater than "doubling up" specific languages.

We'd effectively need to:

  • (set up infrastructure to discover, load and select those modules)
  • maintain source compatibility on the DOM interface.
  • maintain behaviour compatibility on the DOM interface.
  • document the interfaces like OpenAPI generator does today

In any case, the guidance hasn't changed much:

  1. define interfaces
  2. ship an assembly which implements them
  3. use dynamic assembly loading and reflection to load the implementations (which doesn't work with trimming a and a few other contexts)

There seems to be a new plugin definition approach, but it's simply hiding away some of the complexity and fundamentally relying on the same approach.

Some open source projects like this one help bootstrap the catalog discovery and inventory aspects.

@dersia
Copy link
Author

dersia commented Feb 15, 2025

Hi @baywet,

I appreciate your reply. I would kindly disagree with you on the point, that it would double the support cost for c#. I implemented it exactly this way to avoid this. This implementation does exactly the same as the Csharp language implementation does. I just refactored a little bit of the code, some for efficiency and some for the features that I added, and then just used if statements in 3 places to implement the cleaner output.

It doesn't take much to keep this up, as it is filled on a bool and just avoids outputting all the necessary #if pragmas to support netfx. that's it.

Thinking of the future of kiota and what is coming to c# we will surly have to implement something like this. Once union support is in c# we need to change a few things in kiota. While looking through the code I realized that right now it is using refiners to resolve unions and then there are places in the code that make sure that unions are implemented in a compatible way. Once union types arrive, we will need that code for netfx and older version, but surly kiota has to support real c# unions if we want user acceptance. that will have a bigger impact than this change.

Although I have to admit, I am not very happy about the name CSharp13 and have thought about alternatives like CSharpNext so it can roll forward. I also considered just adding a commandline flag but I had the issue that I couldn't come up with a name that describes "all" the changes except --csharp-13-support, but even then it would behave the same way.

And please don't get me wrong, I like the support for netfx, I do have some customers that are still on netfx and I know, that those will profit form such support and they will greatly appreciate the effort, however I also see customers like myself, who want to use kiota for client generation, but they also want to have cleaner, more readable and efficient code. This can only be guaranteed by splitting the implementation, just like there is a split between netfx and net (core).

@darrelmiller thank your for your intervention, but I don't think a plugin system for languages is a good approach for kiota unless we would define a full-featured intermediate language. On the otherside I would love to see a plugin system to support other specs than just OpenApi, i.e. Typespec (without going through OpenAPI Generation), or protobuf. Sure in this scenario a intermediate language would help a lot.

With all that being said, I hope @baywet can take time to look more thoroughly through this PR and reconsider it, since it isn't really changing much and the support cost will stay small.

PS:
I hope we will still take efficiency changes and the small refactoring even if the feature itself won't be accepted.

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.

Kiota should unconditionally generate C# classes with nullable reference types
4 participants