Kiota should unconditionally generate C# classes with nullable reference types

[0xced]

Currently, Kiota generates C# classes with conditional compilation to use nullable reference types. For example:

// <auto-generated/>
using Microsoft.Kiota.Abstractions.Serialization;
using System.Collections.Generic;

// […snip…]

        /// <summary>The Notes property</summary>
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public string? Notes { get; set; }
#nullable restore
#else
        public string Notes { get; set; }
#endif
        /// <summary>The subject property</summary>
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public string? Subject { get; set; }
#nullable restore
#else
        public string Subject { get; set; }
#endif

That's a lot of noise making the generated code hard to read. Instead, it should look like this:

// <auto-generated/>
#nullable enable
using Microsoft.Kiota.Abstractions.Serialization;
using System.Collections.Generic;

// […snip…]

        /// <summary>The Notes property</summary>
        public string? Notes { get; set; }
        /// <summary>The subject property</summary>
        public string? Subject { get; set; }

I know this has been already discussed in https://github.com/microsoft/kiota/issues/2594#issuecomment-1607705061 but I think the conclusion was wrong.

Nullable reference types, introduced in C# 8 are a compiler feature. So it's totally fine to use nullable reference types even when targeting lower than .NET Standard 2.1, such as .NET Standard 2.0 or .NET Framework 4.6.2, 4.7.x etc.

In order to make the code with nullable reference types compile, one simply has to explicitly specify the C# language version to at least 8.0 in the csproj file:

<PropertyGroup>
  <TargetFrameworks>netstandard2.0;net462</TargetFrameworks>
  <LangVersion>8.0</LangVersion>
</PropertyGroup>

The requirement for nullable reference types to work is the version of the SDK that is used, not the target framework. C# 8 was introduced along with .NET Core 3.0 meaning that it will work with any SDK from 3.0.0 onwards. And, as stated in Kiota documentation, the .NET 8 SDK is required anyway.

[baywet]

Hi @0xced Thanks for using kiota, bringing this up, and the open PR.

The minimum required version you're quoting from our documentation is for this quick start tutorial, it's not a requirement for all client applications being built. I understand how this can lead to confusions, and I created a dedicated documentation issue

That being said, the minimum language version a kiota client requires is CSharp 7.3, and there's not conditional compilation directive for "is CSharp 8.0 or higher", so we use the runtime as an imperfect proxy. (technically somebody could be targeting net8, but lockdown the language to 7.3, although highly improbable)

We also don't want to require people changing the default configuration when it's not absolutely necessary.

With all that context in mind, we think the current option is the best, although noisy: somebody with a project targeting netFX or netstandard <2.1 is able to generate a client today and use it without any configuration change.

CSharp 8 comes with a lot of new features, some of which requires runtime support. We don't want people to start tweaking their configuration, write code, only to notice that specific feature is not fully supported in their context.

Our telemetry (from a year ago, when we made the decision) shows that 10% of projects that are created are still under netfx, and that doesn't account for existing projects that are being updated, hence the difficult choice we had to made to keep supporting that segment (it'd have been easier to drop it).

Our hope is that by the time we major rev kiota, usage will have dropped, which means we'll be able to clean up a lot of those things, and take advantage of better patterns. (pattern matching, static nullability check, etc....)

[0xced]

I find it disappointing to have this huge wart in an otherwise high quality generated code. Especially since the fix for the vast minority of users is straightforward, i.e. explicitly setting LangVersion to 8 or greater.

CSharp 8 comes with a lot of new features, some of which requires runtime support.

This particular feature, as emphasised in the original post, is a compiler feature and does not require runtime support so projects can target .NET Framework and/or .NET Standard < 2.1 without any problem.

We don't want people to start tweaking their configuration.

Is adding <LangVersion>8</LangVersion> what you consider tweaking the configuration?

I see it as a very little price to pay to have a tidy generated code. Moreover the IDEs are very good at identifying this issue and even propose one click solutions! In Rider you just have to click on the contextual action and the LangVersion is added automatically to the csproj. I think I remember seeing something similar in Visual Studio.

Is there any chance for this to be reconsidered before a major kiota revision?

[baywet]

Is adding <LangVersion>8</LangVersion> what you consider tweaking the configuration?

yes

Is there any chance for this to be reconsidered before a major kiota revision?

Before we focus on the when (version/timeline), I'd like to get clarity on the what (impact of the change), and the why (reason for it).

On the why: could you expand on the reason the sources of the generated code are so important please? (besides maybe simplicity when debugging and stepping through the code)

On the what: consider the following scenario:

1. create a project configured for C# <8 (net fx)
2. generate a client with the current version of kiota.
3. (assume we make the proposed changes and release kiota)
4. update kiota
5. update the client
6. now the application doesn't build anymore.

The evidence would suggest this is a valid scenario, the support policy would prevent us from releasing such a change without a major version. Although I agree this might be a smaller audience (we don't have numbers for people using kiota in netfx projects), it'd still break them unless their lang version is already set to 8 or above in the csproj.

It seems the nullable adds a set of NullableContext and Nullable attributes under the hood so the information can be conveyed between assemblies. Although selecting an older framework results in those attribute definitions being emitted at compilation, probably for compatibility reasons. (switch the default drop down to framework)

Lastly, even if said "it's ok to break that audience, and have them change their lang version", since C# 8 comes with lots of new features, some of which are not netfx compatible, they might end up writing code that does work at runtime.

Do you have evidence that contradicts those conclusions on the impact?

[KennethHoff]

I agree with the idea that there is a lot of bloat in the generated code, especially if - and I'm just guessing here - most people using Kiota will use it for new projects, and I severely hope (for their sake) that they don't start new projects on a legacy tech stack(.Net Framework)

[0xced]

As developers we have enough things breaking everyday. I sincerely appreciate your efforts and won't argue further for a breaking change, no matter how small it might look.

I'll wait for the next major version and I'll use my fork in the meantime. After all, that's the beauty of open source.

I just hope that it will not fall under the radar when the policy service bot will inevitable close this issue.

[baywet]

Thank you for your understanding. Yes, I'll move this issue into the v2 milestone and remove the tags that result in the bot adding those comments/closing the issues.

[bkoelman]

There are bugs all over the place, resulting from the currently weird on/off switching of nullability context all the time. For example, the indexer parameter on endpoints is declared as string, but outside of a #nullable enable region. So the nullability state is effectively unknown (oblivious, in compiler terms). As a result, calling apiClient.MyModels[null].GetAsync() does not result in a compiler warning, while it should.

Therefore, the current state of the generated client is: not nullable aware, except for a few special-cased places. At the places where nullability is turned on, it is often wrong, which is a known issue. Making every property nullable is even worse than a project that is not nullable-aware at all. Because now we need to type thousands of ! for no reason, effectively fighting the intelligence of the compiler. It's better to be nullable unaware initially (so no compiler warnings), then get sane warnings when kiota is fixed and it produces correct nullability annotations. After kiota is fixed, I will need to undo my thousands of ! first, then review the resulting warnings to determine which ones actually need attention.

This issue is pretty easy to fix: Just generate all code as if nullable was enabled project-wide, then suppress the resulting warnings from an included .editorconfig that's generated in the output folder, such as here.

[baywet]

Thanks for the additional information

For reference, here is how indexers are emitted today

public UserItemRequestBuilder this[string position]
// ...

Which is equivalent to this, assuming nullable is enabled on the project.

public UserItemRequestBuilder this[[DisallowNull] string position]
// ...

Are you recommending we change it to that instead?

#nullable enable
public UserItemRequestBuilder this[string position]
#nullable restore
// ...

[bkoelman]

I have the line <Nullable>enable</Nullable> in Directory.Build.props, so it applies to all projects. The #nullable enable in the next screenshot is grayed out, because it is redundant. Yet it doesn't warn on passing null. My project targets .NET 8.

After adding #nullable enable to the top of the code-generated file, it changes to:

This matches the expected behavior described at https://learn.microsoft.com/en-us/dotnet/csharp/nullable-references#nullable-contexts:

The global nullable context does not apply for generated code files. Under either strategy, the nullable context is disabled for any source file marked as generated. This means any APIs in generated files are not annotated.

[baywet]

Thanks for the additional context. In that setup, if you forcibly set the disallow null attribute like so, but remove the directive you manually added, do you still get a warning upon passing null? public UserItemRequestBuilder this[[DisallowNull] string position]

[bkoelman]

I'll give you a tip: The only way to solve this reliably for projects with nullable on/off across all target frameworks is to suppress all of the C# nullability warnings (and drop all #nullable enable/#nullable restore usage) in generated code, such as here.

This still won't turn on the nullable context in generated files. That requires an explicit #nullable enable once at the top of the file. NSwag emits that when passing the /GenerateNullableReferenceTypes:true command line switch. kiota needs a similar switch (but true by default), to conditionally emit the #nullable enable at the top. From a maintenance perspective, adding the switch shouldn't be expensive, as it only controls codegen of a single line per file.

You can learn a lot by taking a closer look at NSwag. They've solved so many things over the years that are still broken/unintuitive in kiota.

[remiX-]

Has this been sorted? If so, how is it done?

This is so horrible xD

[Suchiman]

On the why: could you expand on the reason the sources of the generated code are so important please? (besides maybe simplicity when debugging and stepping through the code)

Because you're asking me to commit the src to version control and aside from being ugly, all that noise quadruples the number of lines being committed and also impacts compiler throughput. That annoys me sufficiently that i'd rather look for a different tool, if it wouldn't be for kiota being the only tool that is seemingly able to correctly parse this openapi definition in question that i'm having to work with. Kiota does go the extra effort of splitting the generated code up into separate files instead of a single Reference.cs but then falls flat on this.

[petriashev]

I 've created a powershell script that removes all noise https://gist.github.com/petriashev/03e3e2ec739560bc457c41366867b55f

$output = "./Generated"

kiota generate `
 --language CSharp `
 --openapi ./openapi.json `
 --output $output

$files = Get-ChildItem -Path $output -Recurse -Include *.cs
foreach ($file in $files)
{
    $content = Get-Content $file -Raw
    # add nullable enable
    $content = ($content -replace '// <auto-generated/>', "// <auto-generated/>`r`n#nullable enable")
    # remove conditional bullshit
    $content = ($content -replace '(?m)#if NETSTANDARD2_1_OR_GREATER .*\r\n#nullable enable\r\n(?<prop1>.*)\r\n#nullable restore\r\n#else\r\n(?<prop2>.*)\r\n#endif', "`$1")
    # new line between property and next summary
    $content = ($content -replace '}\r\n(?<indent>\s+/// <summary>)', "}`r`n`r`n`$1")
    Set-Content -Path $file -Value $content
}

[bkoelman]

I 've created a powershell script that removes all noise https://gist.github.com/petriashev/03e3e2ec739560bc457c41366867b55f

@petriashev The script didn't work for me. I had to change the "conditional bullshit" line from:

$content = ($content -replace '(?m)#if NETSTANDARD2_1_OR_GREATER .*\r\n#nullable enable\r\n(?<prop1>.*)\r\n#nullable restore\r\n#else\r\n(?<prop2>.*)\r\n#endif', "`$1")

to:

$content = ($content -replace '(?s)#if NETSTANDARD2_1_OR_GREATER .*?\r\n#nullable enable\r\n(?<prop1>.*?)\r\n#nullable restore\r\n#else\r\n(?<prop2>.*?)\r\n#endif', "`$1")

So I switched to single-line mode, and match .* in non-greedy mode.

Also updated the script to work on Windows/Linux/macOS and added suppression of a C# warning. Here's my final version:

<#
    Patches C# code generated by kiota.
    Workaround for bug at https://github.com/microsoft/kiota/issues/3944#issuecomment-2597201229.
#>
param (
    # Path to directory where kiota generated C# code.
    [Parameter(Mandatory=$true)]
    [string]$kiotaOutputDir
)

$osLineBreak = [System.Environment]::NewLine
$files = Get-ChildItem -Path $kiotaOutputDir -Recurse -Include *.cs

foreach ($file in $files)
{
    $content = Get-Content $file -Raw

    # add #nullable enable, suppress warning CS8625: Cannot convert null literal to non-nullable reference type.
    $content = ($content -replace '// <auto-generated/>', "// <auto-generated/>$osLineBreak#nullable enable$osLineBreak#pragma warning disable CS8625")

    # remove conditionals
    $content = ($content -replace "(?s)#if NETSTANDARD2_1_OR_GREATER .*?$osLineBreak#nullable enable$osLineBreak(?<ifBody>.*?)$osLineBreak#nullable restore$osLineBreak#else$osLineBreak(?<elseBody>.*?)$osLineBreak#endif", "`$1")

    # insert new line between member and next summary
    $content = ($content -replace "}$osLineBreak(?<indentedSummary>\s+/// <summary>)", "}$osLineBreak$osLineBreak`$1")

    Set-Content -Path $file -Value $content
}

[bkoelman]

Interestingly, the script is actually a BUGFIX because it emits #nullable enable at the top of the file. Doing so is the only way to correctly activate nullable reference types in generated code. It's explained at https://learn.microsoft.com/en-us/dotnet/csharp/nullable-references#nullable-context:

After running the script, proper nullability warnings are being emitted.

Without the script:

After running the script:

[ThaDaVos]

Any update or ETA on this?