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

C# nullable reference types and sample compilation error #2073

Closed
martinothamar opened this issue Dec 15, 2022 · 10 comments · Fixed by #2146
Closed

C# nullable reference types and sample compilation error #2073

martinothamar opened this issue Dec 15, 2022 · 10 comments · Fixed by #2146
Assignees
Labels
Csharp Pull requests that update .net code enhancement New feature or request generator Issues or improvements relater to generation capabilities.
Milestone

Comments

@martinothamar
Copy link

Hey,

These days we use .NET 7 and with NRT enabled, We also treat warnings as errors, so we can't compile the generated code. In addition, the getting started sample doesn't build currently.

image

I am evaluating this as an alternative to NSwag based codegen. Are there plans to support NRT (generate code that respects NRT)?

@baywet baywet self-assigned this Dec 15, 2022
@baywet
Copy link
Member

baywet commented Dec 15, 2022

Hi @martinothamar
Thanks for your interest in Kiota and for bringing this to our attention.

Samples

I just pushed a few commits to the samples repo to refresh the clients, we've made a number of breaking changes recently as we're getting ready to GA a few languages. Please try it again and let us know if that solved your issue.

Why no null reference types support in kiota?

This is something we've long debated internally as this features brings a lot of value through failsafe.
Dotnet Framework only supports dotnet standard 2.0, not 2.1.
C#8, which brings the NRT feature, requires dotnet standard 2.1 support. There are ways to make it work on dotnet framework, but it's not supported.

In addition to dropping support for dotnet framework by moving to dotnet standard 2.1 we'd drop support for:

Can we do anything about it?

We could decide to drop support for these runtimes (I honestly think that the only one that's problematic is .net framework), which would be a trade of. Getting additional data on how many new projects are being built for dotnet framework today relative to new "core" projects.

Alternatively we could add an auto-generated comment (scroll up, no good anchor) so at least the warnings and errors don't show up anymore. /// <auto-generated/> as first line of the file.

@martinothamar
Copy link
Author

martinothamar commented Dec 15, 2022

Would it be possible to conditionally do nullable annotations based on flags in the generate CLI? NSwag has a lot of configuration options available from CLI or JSON file, maybe languages could provide specialized configuration options based on things like this?

The sample works now, thanks 👍

@baywet
Copy link
Member

baywet commented Dec 15, 2022

Generating different flavours of the same language based off switches or configuration is something we've excluded from the design principles of Kiota from the beginning.

In our experience those flags are detrimental to a code generator for the following reasons:

  • they increase maintenance and support costs by a lot for the generator maintainer.
  • they are cumbersome and hard to "understand" for generator users.

Another example of that same problem is TypeScript. There are 3 module types currently actively supported, dozens of language features that can be turned on or off, etc... Suddenly we'd have hundreds of combinations for the language for a same API description as an input.

@martinothamar
Copy link
Author

Thanks for your detailed explanations. I definitely understand the reasoning and need for limiting complexity.

I do feel like the lack of NRT support will severely limit the applicability of kiota for myself though. The current best practice on .NET library/SDK development as far as I can tell is to include nullable annotations, and the .NET BCL/runtime teams themselves have invested a lot of time into adopting this. I do see that for example the Azure SDKs haven't enabled NRT and have no immediate plans to do so, but I'm really hoping both Kiota and Azure SDKs will transition along with the rest of the ecosystem to support NRT in the future.

NRT is a key feature for my team in enabling us to properly express our domain model and minimizing NREs. For context, I work on a platform team where we have a lot of complex integration code, where a lot of it targets Azure APIs, Kubernetes API and other internal APIs. We have a tendency to lose nullability context across service boundaries and integrations towards 3rd parties which detracts from the value of NRT. In other parts of the platform where we do control both ends of the integrations, we have gotten NSwag to properly generate DTOs with correct nullable annotations, and it allows for much more precise domain modelling and understanding of our APIs.

Another key reason for why I'm evaluating Kiota as opposed to NSwag are the defaults around status code mapping. NSwag currently throws expceptions for 204 No content status codes for example, which leads to really ugly consumer code, especially when the generated code otherwise can be configured to express nullability.

As for what to do about this, I would consider if an all-or-nothing policy for language specific configuration really is the way to go. Maybe complexity is contained well enough if the bar for supporting configuration is sufficiently high?

Aside from this I would personally vote for dropping .NET framework support, as all the projects I work on are .NET 6+. 😄

@baywet baywet added enhancement New feature or request needs more information Csharp Pull requests that update .net code and removed question labels Dec 16, 2022
@baywet
Copy link
Member

baywet commented Dec 16, 2022

Thanks a lot for the additional context here! Customer evidence like this are super helpful when making judgement decisions like these.
Let me loop in @maisarissi the PM for our dotnet experience and @andrueastman the lead engineer on the dotnet experience.
Andrew: is there a way we could tell today with our telemetry who's on .net framework and who's on dotnet core? We should probably only consider the current major since older versions are probably legacy applications and would bias the dataset.
Maisa: probing our preview programs to understand the proportion of apps being built would also help.

@baywet
Copy link
Member

baywet commented Dec 16, 2022

Another option I just thought off could be to use conditional compilation and effectively duplicate the generated code for anything that references a nullable type with a #if NETSTANDARD2_1_OR_GREATER directive.
This would make for ugly code, and increase the maintenance burden a lot for us, but it'd enable the feature without loosing compatibility.

@baywet
Copy link
Member

baywet commented Dec 16, 2022

After bringing this up with the architect during our weekly planning meeting we agreed we should try to implement it we conditional compilation. Queuing this to the backlog.

@baywet baywet added generator Issues or improvements relater to generation capabilities. and removed needs more information labels Dec 16, 2022
@baywet baywet added this to the Kiota GA milestone Dec 16, 2022
@baywet baywet added this to Kiota Dec 16, 2022
@baywet baywet moved this to Todo in Kiota Dec 16, 2022
@martinothamar
Copy link
Author

Great news for us, awesome 😄 I'd be happy to do some testing on our APIs once a prerelease is out

@IanKemp
Copy link

IanKemp commented Jan 8, 2023

I don't understand why you are wasting our and your time with .NET Framework support. Framework is as dead as dead can be without being declared dead... I know it, you know it, the industry knows it. Nobody doing new work is doing it in Framework and if they are, it's work they don't need something advanced like Kiota for.

So please, ditch the unnecessary .NET Framework support and start using new C# features that make code better and safer. Developers will thank you for it.

@baywet
Copy link
Member

baywet commented Jan 17, 2023

Hi everyone,
As you can see with the PR closing this issue, we're implemented support for nullable reference types while keeping compatibility with .net framework applications.
To address comments about the pertinence of maintaining compatibility with .net framework applications: telemetry from Visual Studio and other places shows a non-negligeable portion of users are still creating new .net framework projects to this date.
You can test this change by pulling the repo and building kiota or using the nightly tag on the docker image for now, and we'll release it soon.
Let us know your experience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code enhancement New feature or request generator Issues or improvements relater to generation capabilities.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants