Nullability annotation missing on CacheControlHeaderValue.TryParse

[FreeApophis]

Description

In the namespace System.Net.Http.Headers are many very specific header classes which all have their own TryParse method.

For the following classes TryParse was annotated:
AuthenticationHeaderValue, ContentDispositionHeaderValue, ContentRangeHeaderValue, EntityTagHeaderValue, MediaTypeHeaderValue, MediaTypeWithQualityHeaderValue, NameValueHeaderValue, NameValueWithParametersHeaderValue, ProductHeaderValue, ProductInfoHeaderValue, RangeConditionHeaderValue, RangeHeaderValue, RetryConditionHeaderValue, StringWithQualityHeaderValue, TransferCodingHeaderValue, TransferCodingWithQualityHeaderValue, ViaHeaderValue, WarningHeaderValue

It seems like CacheControlHeaderValue was overlooked.

Reproduction Steps

This does not compile without a null-forgiving operator.

#nullable enable

CacheControlHeaderValue ParseHeaderOrThrow(string candidate)
    => CacheControlHeaderValue.TryParse(candidate, out var result)
        ? result
        : throw Exception("parse failed");

Expected behavior

CacheControlHeaderValue.TryParse should have correct nullability annotations like the others, to avoid the unnecessary null-forgiving operator.

Actual behavior

No nullability annotations.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

In the namespace System.Net.Http.Headers are many very specific header classes which all have their own TryParse method.

For the following classes TryParse was annotated:
AuthenticationHeaderValue, ContentDispositionHeaderValue, ContentRangeHeaderValue, EntityTagHeaderValue, MediaTypeHeaderValue, MediaTypeWithQualityHeaderValue, NameValueHeaderValue, NameValueWithParametersHeaderValue, ProductHeaderValue, ProductInfoHeaderValue, RangeConditionHeaderValue, RangeHeaderValue, RetryConditionHeaderValue, StringWithQualityHeaderValue, TransferCodingHeaderValue, TransferCodingWithQualityHeaderValue, ViaHeaderValue, WarningHeaderValue

It seems like CacheControlHeaderValue was overlooked.

Reproduction Steps

This does not compile without a null-forgiving operator.

#nullable enable

CacheControlHeaderValue ParseHeaderOrThrow(string candidate)
    => CacheControlHeaderValue.TryParse(candidate, out var result)
        ? result
        : throw Exception("parse failed");

Expected behavior

CacheControlHeaderValue.TryParse should have correct nullability annotations like the others, to avoid the unnecessary null-forgiving operator.

Actual behavior

No nullability annotations.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	FreeApophis
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[CarnaViire]

Triage: it is most possibly an oversight, but it is possible there was a reason for it. Nice to have, seems to be the first complaint since introducing nullability annotations. Moving to Future.

[MihaZupan]

Most header values support only a single value via TryParse, but Cache-Control is different.
For example, we will let you parse public,max-age=3600 (two values) and turn that into a single CacheControlHeaderValue.

One side-effect (I don't know whether it was intentional) is that this header will allow parsing empty values.
For example, CacheControlHeaderValue.TryParse(",, ,", out var value) will return true, but value will be null.
But Parse is not annotated correctly. CacheControlHeaderValue.Parse("") will not throw, but just return null.

So I think there are a few ways of looking at this. Either:

• a) Allowing empty values is correct and we should mark Parse as nullable.
• b) Allowing empty values is correct and we should start returning empty CacheControlHeaderValues objects and mark TryParse with NotNullWhen(true).
• c) Empty values shouldn't be allowed and we should start throwing/returning false in those cases and mark TryParse with NotNullWhen(true).

Adding back the untriaged label so we can revisit.

[karelz]

Triage: CacheControlHeaderValue is unique, not like others and its ability to accept empty string makes lives of devs hard for all other headers.
We do not like to change the annotation to Nullable -- it would break users as they would have to change their code.
Throwing for empty string is even more breaking.
Returning empty value instead of null sounds like the best option - least impactful on users. We can also change TryParse annotation to NotNullable (which will match all other headers).

[karelz]

This should be fairly straightforward to do. It is not important for any release, but we would take a contribution.
@FreeApophis are you interested?

[pierrebelin]

Hey @karelz, since this issue is not important and not in a rush, can I take this one as first issue to start contributing?

[karelz]

Great, thanks! Assigning to you @pierrebelin
Let us know if things are more complicated than we expected.

[pierrebelin]

Of course, I'll try to do my best!

I have some issues when I try to build the project. I have installed .NET Runtime/SDK/ASP.NET Core 7.0 preview but it seems that all my dependencies are missing. Do you know what I missed ?

[rzikm]

@pierrebelin in my experience, VS sometimes struggles with the runtime repo, but as long as it builds without errors you should be fine.

If there are build errors, can you post the log here so we can have a better idea how to help you?

Edit: I assume you have read the contributing guide and followed the steps for building libraries

[rzikm]

The last BUILD: Error: line says it all, Visual Studio 2019 or 2022 with C++ tools are required. Did you try importing the .vsconfig in the Visual Studio Installer? Not sure if this thing requires a reboot.

[pierrebelin]

Yes, that's why I deleted my post... I think it's too early to open correctly my eyes. I'm gonna grab a coffee