v13 vs. v14: C# client generates a lot more warnings in v14 than in v13.

[vvdb-architecture]

After upgrading our .NET 6 project from v13 to v14, we notice two things:

(1) Issue #4467 is still valid. The warnings:

Warning	CS8765	Nullability of type of parameter 'value' doesn't match overridden member (possibly because of nullability attributes).	
Warning	CS8765	Nullability of type of parameter 'existingValue' doesn't match overridden member (possibly because of nullability attributes).

... are still issued because the methods WriteJson and ReadJson in `Newtonsoft.Json.JsonConverter are defined as:

        public abstract void WriteJson(JsonWriter writer, object? value, JsonSerializer serializer);
        public abstract object? ReadJson(JsonReader reader, Type objectType, object? existingValue, JsonSerializer serializer);

... whereas in the generated code, the methods are overriden with the following signature:

public override void WriteJson(Newtonsoft.Json.JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
public override object ReadJson(Newtonsoft.Json.JsonReader reader, System.Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)

(2) All client types generate the following warning in their constructor:

Warning	CS8618	Non-nullable field '_baseUrl' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.

This is because the field _baseUrl was defined as follows in v13:

        private string _baseUrl = "https://";

... whereas in v14, it's just:

        private string _baseUrl;

But since the constructor doesn't reference it, a warning is issued since it's a non-nullable reference.
The removal of the initialization string was a good move since it was confusing. To get rid of the error, one can use a "null forgiving operator":

        private string _baseUrl = default!;

This will get rid of the warning.

[RicoSuter]

@paulomorgado are these regressions of your latest changes?

_baseUrl has been changed here:
https://github.com/RicoSuter/NSwag/pull/4541/files

can you fix this?

[paulomorgado]

Do we have a unit test for this? If not, where/how can it be added?

[paulomorgado]

Should we be concerned about these warnings in the generated code?

It's fine to just get rid of the warning with the null forgiving operator. But only if we are sure that it won't be null on first use. And, in that case, we can just ignore the warning.

[vvdb-architecture]

v13 always initialized _baseUrl with "{{ BaseUrl }}".
If I understand @RicoSuter well, the the pull request he refers to and became part of v14 simply removed that initialization. Hence the warning appears.

As for the concern for warnings, strictly speaking there is no concern since "they are only warnings".
But if you work for an employer that strictly enforces "zero warnings" at the quality gate, and wants you to explain why some warnings are explicitly disabled instead of writing the code correctly in the first place, I would not classify them as "concern", but more of a "minor bother". Like a mosquito during summer teasing you.

[ViRuSTriNiTy]

You should never ignore warnings as they are hinting to possible errors. You have 3 options:

• Initialize the field correctly, default! is not an option because it just masks the warning
• Define the field as string? when nullable reference types are enabled
• Disable nullable reference types for the project owning the field

[paulomorgado]

You should never ignore warnings as they are hinting to possible errors. You have 3 options:

• Initialize the field correctly, default! is not an option because it just masks the warning
• Define the field as string? when nullable reference types are enabled
• Disable nullable reference types for the project owning the field

@ViRuSTriNiTy,

Ignoring errors in non-generated code is always my advise.

None of these options are viable options.

• If there's no value to initialize at construction time, using a #pargma or initializing with the default value for the type asserting that it's fine is a perfectly good option.
• Defining the type as string? would require a null check on every use. That would be adding unnecessary complexity.
• Disabling nullable reference types for a whole project just because of one line of code is one of the worst possibe "solutions".

[paulomorgado]

v13 always initialized _baseUrl with "{{ BaseUrl }}". If I understand @RicoSuter well, the the pull request he refers to and became part of v14 simply removed that initialization. Hence the warning appears.

As for the concern for warnings, strictly speaking there is no concern since "they are only warnings". But if you work for an employer that strictly enforces "zero warnings" at the quality gate, and wants you to explain why some warnings are explicitly disabled instead of writing the code correctly in the first place, I would not classify them as "concern", but more of a "minor bother". Like a mosquito during summer teasing you.

@vvdb-architecture,

That field is not directly accessed anymore and its value is initialized in the constructor via the BaseUrl property.

I understand why you are running into an issue here and I can relate to it, although generated code should be checked for in the context of the generator and not the generated code. In fact, like you just did by reporting it here and not by checking generated code.

I'll add a #pragma with a comment to disable the warning at that line.

[paulomorgado]

Added

#pragma warning disable 8618 // Set by constructor via BaseUrl property
    private string _baseUrl;
#pragma restore disable 8618 // Set by constructor via BaseUrl property

to PR #4634.

[paulomorgado]

v13 always initialized _baseUrl with "{{ BaseUrl }}". If I understand @RicoSuter well, the the pull request he refers to and became part of v14 simply removed that initialization. Hence the warning appears.

As for the concern for warnings, strictly speaking there is no concern since "they are only warnings". But if you work for an employer that strictly enforces "zero warnings" at the quality gate, and wants you to explain why some warnings are explicitly disabled instead of writing the code correctly in the first place, I would not classify them as "concern", but more of a "minor bother". Like a mosquito during summer teasing you.

I'm always concerned about warnings, because they hint to possible run-time issues.

It's an illusion to think one can enforce whatever policies on code you don't control. If this was a library or a tool generating IL, that would not be source checked. Or would it be?

What would be the way of "writing the code correctly" in this case?

[klemmchr]

Added

#pragma warning disable 8618 // Set by constructor via BaseUrl property
    private string _baseUrl;
#pragma restore disable 8618 // Set by constructor via BaseUrl property

to PR #4634.

This does not solve the warning because the warning is raised on the constructor, not on the property. Additionally this prevents users from suppressing this warning (and others) using a custom file header template as well.

In general it is better to suppress such warnings using the null forgiving operator rather than supressing. This code does the same trick and is less verbose.

private string _baseUrl = null!;

[paulomorgado]

This does not solve the warning because the warning is raised on the constructor, not on the property. Additionally this prevents users from suppressing this warning (and others) using a custom file header template as well.

@klemmchr, can you provide practical examples of this?

[klemmchr]

This does not solve the warning because the warning is raised on the constructor, not on the property. Additionally this prevents users from suppressing this warning (and others) using a custom file header template as well.

@klemmchr, can you provide practical examples of this?

We are setting TreatWarningsAsErrors during build so we need to suppress every warning in the generated client. In v13 the client had nullability warnings that we disabled using a custom file header template. However, this is just a dirty fix. A generated client should never ever produce any warnings or errors. They are not useful for the user because the user has literally no impact on it.