C# generator: Generate C# 8 Nullable Reference Type (NRT) attributes

[RicoSuter]

Add new setting for this feature.

[adamjones1]

It would be great to have this. If I understand right, this issue is only for client generation, so shouldn't the implementation of this not need to meddle with the NRT attributes at all and simply need to know when to syntactically add a ? to a type name like it already does with structs? This should be quite easy since the logic is presumably already in place for structs (but ignored for reference types), right?

[RicoSuter]

If I understand right, this issue is only for client generation, so shouldn't the implementation of this not need to meddle with the NRT attributes at all and simply need to know when to syntactically add a ? to a type name like it already does with structs?

Exactly, it’s probably not super trivial but it will be an opt-in setting so it will no break existing user and it doesnt matter if its not working 100% at the beginning.

[jeremyVignelles]

Any update on this? I was considering redistributing the generated C# client in an assembly, but was wondering if I should enable nullable reference types there or not.

It would be easier in the meantime to generate #nullable disable at the top of the file so that we can set <Nullable>enable</Nullable> at the project level, what do you think ?

[RicoSuter]

Good idea, however adding #nullable disable is probably a breaking change when not using C# 8, right?

[jeremyVignelles]

Didn't think about that. Probably worth having an option to disable it, you're right.

We then have four states:

• Disabled
• Explicitely disabled
• Enabled
• Explicitely enabled

Or maybe it's better to have an option like "C# output language level" and use #nullable enable only for C#8 and above?
That would pave the way for C#9 records/init only, right?

[RicoSuter]

Or maybe it's better to have an option like "C# output language level" and use #nullable enable only for C#8 and above?

Good idea but not sure about that, I think NRT would be the only thing which would check that - and then it's probably better to a have a specific option... for everything else i think we can just generate eg C# 6 code and there is just no downside (unlike in TS).

That would pave the way for C#9 records/init only, right?

I think it would be better to have another class style here:

(e.g. NativeRecord :-)

[jeremyVignelles]

I was thinking about it more as a safeguard to avoid using settings unavailable for the target language level, but OK for your NativeRecord suggestion.

About the #nullable disable, what would you suggest ? A drop down with the 4 options I described above? (Or 3 if we decide that implicitely Enabled option is useless)

[RicoSuter]

What about a boolean setting "GenerateNullableReferenceTypes" (default: false) and for now it just generates #nullable disable and then we can just implement it? How bad is that?

[jeremyVignelles]

Probably a bit misleading if NRT are not really implemented.

How hard would it be to implement NRT directly behind the flag ?

[RicoSuter]

How hard would it be to implement NRT directly behind the flag ?

I think it should be quite simple to implement - we already have it for typescript.. at least for the public api contract - not sure whether we have problems “inside” the methods etc but there we could just disable the checks at the beginning

[RicoSuter]

I think we only need to add a ? if the reference type is nullable and the setting is enabled here:
https://github.com/RicoSuter/NJsonSchema/blob/master/src/NJsonSchema.CodeGeneration.CSharp/CSharpTypeResolver.cs#L137

I think the others in this method use Nullable<> and are already handled correctly - or what do you think?

[RicoSuter]

Not sure but this might already be enough:
#1193

[jeremyVignelles]

I don't see any other case where this might break something:

• nullable parameters are probably already checked for nullability
• nullable return is likely already handled as well
• nullable properties : You don't handle them directly in the generated code, right? so no problem here.

Problems will arise when used in a project if the code using that doesn't match the intent, but that's the whole point of enabling that, and it's opt-in.

I don't see anything else problematic

[RicoSuter]

My thinking was that if you have warnaserror enabled and the flow analysis within the method gives warnings it would turn them into errors and thus wont compile - but i think we disable all warnings in the generated code so this might also be fine...

[jeremyVignelles]

I don't understand : If that's an opt-in feature, it would only break if you tell it to enable NRT, right ?
What kind of flow analysis would break anything?

[RicoSuter]

My PR above probably is already enough because this resolver is used for method params, return param and POCO property types... the "problem" is - also with NRT in general - that it's only a compile time feature - so if the server returns wrong JSON (ie null but the property is not-nullable) then there will be no exception at runtime, this could be solved with this:

https://github.com/RicoSuter/Namotion.Reflection#validate-nullability-c-8

but this is a topic we won't cover in this issue probably.

[RicoSuter]

What kind of flow analysis would break anything?

For example if NRT is enabled and your param is non nullable but in the generated method we still check it against null this might give a warning (eg check not needed) which might be a problem... but it's something we need to try out and improve over time I think...

[jeremyVignelles]

Isn't there any check in the Json serializer that checks the [Required] and [JsonProperty] attributes and checks whether the deserialized json is valid w.r.t the specified attributes?

[RicoSuter]

Can you check my PR above (i.e. go through the changed class).

Does it look good?
Would you be willing to add some unit tests for that later?

[RicoSuter]

Isn't there any check in the Json serializer that checks the [Required] and [JsonProperty] attributes and checks whether the deserialized json is valid w.r.t the specified attributes?

Ah you are right!
I was thinking about own written code where you dont have JsonProperty attributes but only the NRT info - there you'd have this problem... in our generated code the NRT and JsonProperty attribute would be in sync and the serializer even checks - so probably all good.

[RicoSuter]

Should we go with this PR? As its a new, experimental setting (opt-in) I can merge and release that now and you can try it out and then we add tests + improve on that... what do you think?

[jeremyVignelles]

I've read your PR and didn't find anything wrong, but I don't have your knowledge of this project, and I don't know what impact it has.

Should we go with this PR? As its a new, experimental setting (opt-in) I can merge and release that now and you can try it out and then we add tests + improve on that... what do you think?

OK, but I won't be able to test before Monday.

[jeremyVignelles]

Also, for my use case (NSwag code gen), would I need a new release of NSwag too ?

[RicoSuter]

Also, for my use case (NSwag code gen), would I need a new release of NSwag too ?

I’d release njs + nswag with the update

[RicoSuter]

Ok do you think the name GenerateNullableReferenceTypes is good?

[jeremyVignelles]

I can't find a better name, and "Nullable Reference Types" is the way that feature is named in the C# language. I'm ok with that naming.

[RicoSuter]

v13.6.1

[RicoSuter]

Ref: #1195

[jeremyVignelles]

Oops, I completely missed that the PR you were sending didn't include #nullable annotations, sorry

[jeremyVignelles]

Since this has been released, can it be closed now ?

[adamjones1]

I get a whole bunch of compiler warnings from the generated ConvertToString method enabling this (happy to raise this as a separate issue if you prefer).

Almost all of them can be resolved as marking the method with [return: NotNullIfNotNull("value")], and adding a little sanity check within the method to default to the empty string for the rare case when value is non-null but the method otherwise returns null because value.ToString() is null.

The rest of them are when there's a query parameter list of nullables, eg. IEnumerable<string?>. It tries to URI-escape each of the strings but fails when one is null. Probably sensible to default to QueryNullValue from the NSwag config file in this case.

[jeremyVignelles]

@adamjones1 : The ConvertToString issues are covered by RicoSuter/NSwag#2959

I don't know about the IEnumerable<string?> ones though.

[adamjones1]

Great, thanks. For the list ones, I get code generated like this for a method:

public async System.Threading.Tasks.Task<System.Collections.Generic.ICollection<MyResultDto>> GetResultsAsync(System.Collections.Generic.IEnumerable<string?>? ids = null)
{
    var urlBuilder_ = new System.Text.StringBuilder();
    urlBuilder_.Append(BaseUrl != null ? BaseUrl.TrimEnd('/') : "").Append("/getResults?");
    if (ids != null) 
    {
        foreach (var item_ in ids) { urlBuilder_.Append(System.Uri.EscapeDataString("ids") + "=").Append(System.Uri.EscapeDataString(ConvertToString(item_, System.Globalization.CultureInfo.InvariantCulture))).Append("&"); }     
    }

    ...
}

Here what's going in to ConvertToString is nullable so the output is legitimately nullable and there's still a warning at EscapeDataString. Would be great if you could add a null-coalesce to that in your PR!

[jeremyVignelles]

Implemented in the PR. Do you have the ability to try in my PR and tell me if it work ?
There's the NSwag.ConsoleCore that you can run with the nswag.json file you want.

[adamjones1]

Just tried it, I get compiler errors because item_ and "" are of different types in some cases (in my example it was an IEnumerable<string?> but it could in general be an enumerable of anything). An upcast to object? should fix it.

[jeremyVignelles]

You're right, might have missed that. Let me fix that

[jeremyVignelles]

Does it work better now ?

[adamjones1]

Looks good, no compiler or runtime errors/warnings for me 👍

[RicoSuter]

Closed as implemented and released, please create new issues if there are bugs/problems with the current implementation.