Avoiding proliferations of ifdefs for nullable types

[Petermarcu]

example here: https://github.com/octokit/dotnet-sdk/blob/main/src/GitHub/Models/Issue.cs

You could consider doing something similar to the Azure SDK where local attributes that the compiler respects are defined and work down to netstandard2.0. This prevents having to emit code with all the conditionals everywhere.

Note, that these local attributes should only ever be internal in shipping libraries or they will cause conflicts with other libraries.

https://github.com/Azure/azure-sdk-for-net/blob/69c969a752cf07ae4ae7c67abdc2ddc8da209693/sdk/core/Azure.Core/src/Shared/NullableAttributes.cs

[baywet]

Hi @Petermarcu,
Thanks for this great suggestion that will allow us to generate clearer code. (has been a customer complaint in the past)
In this scenario, we're thinking about adding the attribute definitions in our abstractions library, maybe with unique names so they can't conflict with framework's ones.
Any objection to that?

[Petermarcu]

The name you use has to be one recognized by the C# compiler. @KrzysztofCwalina is the person I would reach out to to make sure you get it right.

[baywet]

Thanks for the additional information @KrzysztofCwalina could we imagine a situation where we still place those abstractions library with the expected name, but we fully qualify them in the generated code to disambiguate from the default framework ones?

[KrzysztofCwalina]

@jaredpar

If your users use the compiler nullability syntax, I think you can get away with multiple copies of the same named attributes in scope (@jaredpar would know for sure), which can happen if you ship them as public types in a reusable package. But we don't do things like that in .NET libraries because sooner or later users will refer to these attributes directly (some don't have syntax) and that can cause ambiguities or at least confusions when trying to debug.

[baywet]

right, but since the code we're trying to simplify here is generated, we need to place that annotation somewhere.
The alternative being to "generate the annotations along with the code", I'm not sure this is a better alternative than placing them in the abstractions library the generated code already depends on.
We could add BIG CAPS DOC COMMENTS FOR PEOPLE NOT TO USE them, and hopefully they'll follow the advice.
But honestly, this whole thing would have been much easier if the dotnet team had made these annotations compatible all the way to net standard 2 for library developers.

[jaredpar]

#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public string? ActiveLockReason { get; set; }
#nullable restore
#else
        public string ActiveLockReason { get; set; }
#endif

I don't understand the need for this pattern though. Generally the way that most customers approach this is to just do the following:

        public string? ActiveLockReason { get; set; }

Then just include the attributes which aren't autogenerated by the compiler inside #if as @Petermarcu mentioned.

But honestly, this whole thing would have been much easier if the dotnet team had made these annotations compatible all the way to net standard 2 for library developers.

In what way are they not compatible? We use them in the C# compiler just fine with targeting netstanard2.0.

[baywet]

I don't understand the need for this pattern though

Allow me to provide additional context here: people can use kiota to generate code which ends up in a netstandard2.0 project. We need to ensure the projected code will compile properly within that context. (this is also why we don't rely on file scoped namespaces)

As far as I know, netstandard2.0 does not support nullable reference types were introduced with CSharp 8 and the annotations are only available with netstandard2.1 and netcore3.1 or higher

By default:

1. the compiler will warn about using NRT in a netstanard2.0 or netcore3.0 and below context
2. the compiler will warn about NOT using NRT in a netsndard.21 or net3.1 and above context

Unless the source either has:

• // <auto-generated/> (we added that later on, only works for 2)
• .g.cs extension (has other side effects in the editor, undesired)
• #nullable annotations are in use (only works for 1., requires the project lang to be changed, overrides the project settings, undesired)
• Nullable is disabled in the csproj (only works for 2. kiota doesn't touch the csproj to avoid messing up with people's setup)

The warning can turn into an error if the csproj has Treat Warning As Error (many people do so to "block" things in order to avoid building technical debt).

In what way are they not compatible? We use them in the C# compiler just fine with targeting netstanard2.0

the following

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <TreatWarningAsError>true</TreatWarningAsError>
  </PropertyGroup>

</Project>

namespace dotnetlib
{
	public class Class1
	{
		public string? Id {get;set;}
	}
}

will result in error CS8370: Feature 'nullable reference types' is not available in C# 7.3. Please use language version 8.0 or greater

If we can get rid off the if def while maintaining compatibility with a netstandard2.0 project, I'm happy to implement the solutions.

[jaredpar]

the compiler will warn about using NRT in a netstanard2.0 or netcore3.0 and below context

Okay I think I understand the issue a bit better here. Essentially in netstandard2.0 the default language version is 7.3. That version of the language does not support NRT hence you will get warnings / errors about its usage.

If we can get rid off the if def while maintaining compatibility with a netstandard2.0 project, I'm happy to implement the solutions.

Have you considered letting users specify the language version or target framework they are using on the command line? Essentially something like kiota generate -l CSharp -f net6.0? That way rather than having a number of #if within the code you could have a much cleaner file.

[baywet]

Have you considered letting users specify the language version or target framework

Yes, this is something we're fundamentally against as we want to keep the CLI and the maintenance matrix simple.

As an example, typescript has could have about a dozen switches for the level of compatibility of the language, the module types, etc... We want to prevent that sprawl of switches that will degrade the experience.

[0xced]

Hello everyone, there's some additional context on this exact same topic in #3944.

TLDR: should be fixed in kiota v2 (maybe with pull request #3945).

[baywet]

Thanks everyone for the input here. Closing this one since, given our requirements, the dotnet team members on this thread has reached the same conclusion.
Also we already have an issue tracking that breaking change for v2 as pointed out in the previous comment.