Skip to content

Add nullable annotations to Http.Abstractions, Http.Features, Connections.Abstractions #22337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 5, 2020
Merged

Add nullable annotations to Http.Abstractions, Http.Features, Connections.Abstractions #22337

merged 3 commits into from
Jun 5, 2020

Conversation

pranavkm
Copy link
Contributor

No description provided.

@pranavkm
Copy link
Contributor Author

I'll undo the MVC bits, but the rest of this should be good to go.

@pranavkm pranavkm marked this pull request as ready for review May 29, 2020 18:50
@pranavkm pranavkm requested a review from JamesNK May 29, 2020 18:50
Base automatically changed from prkrishn/localization to master June 1, 2020 18:32
@pranavkm pranavkm force-pushed the jamesnk/nullable-httpabstractions branch 4 times, most recently from 7cf268c to 7fc367a Compare June 3, 2020 16:09
@pranavkm pranavkm force-pushed the jamesnk/nullable-httpabstractions branch from 7fc367a to ff2f78d Compare June 3, 2020 17:06
@pranavkm
Copy link
Contributor Author

pranavkm commented Jun 3, 2020 •
edited
Loading

This is a continuation of #18084. Hopefully all of the initial comments listed there should be addressed. It'll be cool to get this in soon, since all of the middleware work is blocked on it

JamesNK
JamesNK reviewed Jun 3, 2020
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things I noticed from reviewing public API changes.

src/Http/Headers/src/EntityTagHeaderValue.cs
}

public static IList<EntityTagHeaderValue> ParseList(IList<string> inputs)
public static IList<EntityTagHeaderValue> ParseList(IList<string>? inputs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParseList.ParseList doesn't allow a null list be EntityTagHeaderValue does. Intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean TryParseList? I updated those to also allow nulls since the code that this calls is null tolerant.

JamesNK
JamesNK approved these changes Jun 3, 2020
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but I'm not an expert in many of these APIs. @Tratcher / @halter73 should review before merge.

halter73
halter73 reviewed Jun 4, 2020
View reviewed changes
halter73
halter73 reviewed Jun 4, 2020
View reviewed changes
src/Http/Headers/src/CookieHeaderParser.cs Outdated
@@ -13,7 +14,7 @@ internal CookieHeaderParser(bool supportsMultipleValues)
{
}

public override bool TryParseValue(StringSegment value, ref int index, out CookieHeaderValue parsedValue)
public override bool TryParseValue(StringSegment value, ref int index, [MaybeNull] out CookieHeaderValue? parsedValue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need both [MaybeNull] and ?? Should this also use [NotNullWhen(true)]?

Copy link
Contributor Author

@pranavkm pranavkm Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing MaybeNull would be the way to go. This can return a null value while returning true, so NotNullWhen doesn't quite work.

halter73
halter73 reviewed Jun 4, 2020
View reviewed changes
halter73
halter73 reviewed Jun 4, 2020
View reviewed changes
src/Http/Headers/src/BaseHeaderParser.cs

public sealed override bool TryParseValue(StringSegment value, ref int index, out T parsedValue)
public sealed override bool TryParseValue(StringSegment value, ref int index, [MaybeNull] out T parsedValue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can [NotNullWhen(true)] be applied here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. The SupportsMultipleValues pattern makes nullable annotations really awkward to use .

halter73
halter73 reviewed Jun 4, 2020
View reviewed changes
halter73
halter73 reviewed Jun 4, 2020
View reviewed changes
halter73
halter73 reviewed Jun 4, 2020
View reviewed changes
halter73
halter73 reviewed Jun 4, 2020
View reviewed changes
src/Http/Headers/src/HttpHeaderParser.cs

[return: MaybeNull]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this marked [return: MaybeNull]? It looks like it throws if it's null. If you change this you can remove ! from some of the callsites.

Copy link
Contributor Author

@pranavkm pranavkm Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseHeaderParser.TryParseValue can return null while returning true when SupportsMultipleValue is set. It would really have helped if the single-value parser and the multi-value parser where separate types so you could declare their behavior in code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just throw from this method when it would otherwise return null? Do any of the callers actually handle null appropriately? It looks like most callers would blow up with a NullReferenceException.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some code paths actually handle this well: https://github.com/dotnet/aspnetcore/blob/master/src/Http/Headers/src/CacheControlHeaderValue.cs#L330-L334. Like I said, you really want to separate single and mullti-valued parsers for this to be expressed correctly, but that's well beyond the scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be too big a big change to have the callers that expect the return value could be null to call TryParseValue instead? I get not wanting to make this much bigger though. My browser is really struggling when I go to the files tab of this PR 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing all the null-forgiviness from our src files is certainly desirable. But a) I'm too lazy b) fixing internal implementation, which mostly works well today, has marginal utility. Maybe somebody else would want to give this a go. 😸

@pranavkm
Copy link
Contributor Author

pranavkm commented Jun 4, 2020

🆙 📅

@pranavkm pranavkm merged commit f37fa30 into master Jun 5, 2020
@pranavkm pranavkm deleted the jamesnk/nullable-httpabstractions branch June 5, 2020 13:54
@BrennanConroy BrennanConroy mentioned this pull request Jun 6, 2020
Merged
Tratcher
Tratcher reviewed Jun 12, 2020
View reviewed changes
src/Http/Headers/src/MediaTypeHeaderValue.cs
@@ -205,7 +206,7 @@ public IList<NameValueHeaderValue> Parameters
/// </summary>
public double? Quality
{
get { return HeaderUtilities.GetQuality(_parameters); }
get { return HeaderUtilities.GetQuality(Parameters); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this _parameters to avoid having Parameters allocate if _parameters was null, and GetQuality was null tolerant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I missed addressing this from the original PR: https://github.com/dotnet/aspnetcore/pull/18084/files#r362617412

@JamesNK JamesNK mentioned this pull request Dec 22, 2020
Merged
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tratcher Tratcher Tratcher left review comments

@halter73 halter73 halter73 approved these changes

@JamesNK JamesNK JamesNK approved these changes

@jkotalik jkotalik Awaiting requested review from jkotalik

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pranavkm @halter73 @JamesNK @Tratcher @Pilchie @amcasey