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

JamesNK · 2020-01-02T09:06:53Z

Addresses #5680

Annotations on projects at the bottom of the dependency tree. Because I haven't converted the tests there may be some types that are not null but should be. Should discover those as more projects are annotated.

…ions.Abstractions

halter73 · 2020-01-02T19:54:42Z

src/Http/Headers/src/BaseHeaderParser.cs

@@ -14,9 +15,9 @@ protected BaseHeaderParser(bool supportsMultipleValues)

        protected abstract int GetParsedValueLength(StringSegment value, int startIndex, out T parsedValue);

-        public sealed override bool TryParseValue(StringSegment value, ref int index, out T parsedValue)
+        public sealed override bool TryParseValue(StringSegment value, ref int index, [MaybeNullWhen(false)]out T parsedValue)


It looks like TryParseValue can return true even in cases where parsedValue is never set if SupportsMultipleValues is also true.

halter73 · 2020-01-02T20:01:06Z

src/Http/Headers/src/HttpHeaderParser.cs

@@ -27,7 +28,7 @@ public bool SupportsMultipleValues
        // pointing to the next non-whitespace character after a delimiter. E.g. if called with a start index of 0
        // for string "value , second_value", then after the call completes, 'index' must point to 's', i.e. the first
        // non-whitespace after the separator ','.
-        public abstract bool TryParseValue(StringSegment value, ref int index, out T parsedValue);
+        public abstract bool TryParseValue(StringSegment value, ref int index, [NotNullWhen(true)]out T parsedValue);


Shouldn't the type be marked nullable when using attributes like NotNullWhen?

Tratcher

How do you plan to handle microsoft.extensions.activatorutilities.sources?

Tratcher · 2020-01-02T18:32:09Z

src/Http/Headers/src/CacheControlHeaderValue.cs

@@ -31,12 +31,12 @@ public class CacheControlHeaderValue
        // Cache-Control headers, only one instance of CacheControlHeaderValue is created (if all headers contain valid
        // values, otherwise we may have multiple strings containing the invalid values).
        private static readonly HttpHeaderParser<CacheControlHeaderValue> Parser
-            = new GenericHeaderParser<CacheControlHeaderValue>(true, GetCacheControlLength);
+            = new GenericHeaderParser<CacheControlHeaderValue>(true, GetCacheControlLength!);


Error CS8622 Nullability of reference types in type of parameter 'parsedValue' of 'int CacheControlHeaderValue.GetCacheControlLength(StringSegment input, int startIndex, out CacheControlHeaderValue? parsedValue)' doesn't match the target delegate 'GenericHeaderParser<CacheControlHeaderValue>.GetParsedValueLengthDelegate'. Microsoft.Net.Http.Headers D:\github\AspNetCore\src\Http\Headers\src\CacheControlHeaderValue.cs 34 Active

This seems like something the tooling should be able to handle?

internal delegate int GetParsedValueLengthDelegate(StringSegment value, int startIndex, [MaybeNull]out T parsedValue);

int GetCacheControlLength(StringSegment input, int startIndex, out CacheControlHeaderValue? parsedValue)

Or is the issue that T is unconstrained and could be a value type?

Tratcher · 2020-01-02T18:35:37Z

src/Http/Headers/src/CacheControlHeaderValue.cs

@@ -605,7 +604,7 @@ private static bool TrySetTimeSpan(NameValueHeaderValue nameValue, ref TimeSpan?
        {
            Contract.Requires(nameValue != null);

-            if (nameValue.Value == null)
+            if (nameValue!.Value == null)


The warning goes away if you remove Contract.Requires. These are redundant now and could be removed.

Tratcher · 2020-01-02T18:42:36Z

src/Http/Headers/src/ContentDispositionHeaderValue.cs

@@ -142,7 +143,7 @@ public StringSegment FileNameStar
                else
                {
                    string sizeString = value.GetValueOrDefault().ToString(CultureInfo.InvariantCulture);
-                    _parameters.Add(new NameValueHeaderValue(SizeString, sizeString));
+                    _parameters!.Add(new NameValueHeaderValue(SizeString, sizeString));


This is an actual bug. NameValueHeaderValue.Find allows _parameters to be null. new ContentDispositionHeaderValue("foo") { Size = 1 } throws

System.NullReferenceException : Object reference not set to an instance of an object. Stack Trace: ContentDispositionHeaderValue.set_Size(Nullable`1 value) line 146

The fix would be to use Parameters.Add instead.

Tratcher · 2020-01-02T18:48:14Z

src/Http/Headers/src/HeaderUtilities.cs

@@ -41,15 +41,15 @@ internal static void SetQuality(IList<NameValueHeaderValue> parameters, double?
                }
                else
                {
-                    parameters.Add(new NameValueHeaderValue(QualityName, qualityString));
+                    parameters!.Add(new NameValueHeaderValue(QualityName, qualityString));


Same issue with Contract.Requires.

Tratcher · 2020-01-02T18:54:13Z

src/Http/Headers/src/HttpHeaderParser.cs

@@ -166,7 +167,7 @@ public virtual string ToString(object value)
        {
            Contract.Requires(value != null);

-            return value.ToString();
+            return value!.ToString()!;


Same issue with Contract.Requires.

Tratcher · 2020-01-02T20:25:47Z

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); }


GetQuality is wrong in this case, it works just fine if parameters is null, the Contract.Requires can be removed and paramters can be marked as nullable. There's no point in allocating an empty Paramaters collection just to search it for a property we know isn't there.

Tratcher · 2020-01-02T20:32:38Z

src/Http/Headers/src/NameValueHeaderValue.cs


                if (nameValueLength == 0)
                {
                    // There may be a trailing ';'
                    return current - startIndex;
                }

-                nameValueCollection.Add(parameter);
+                nameValueCollection!.Add(parameter!);


Same Contract.Requires issue.

Tratcher · 2020-01-02T20:35:15Z

src/Http/Headers/src/RangeItemHeaderValue.cs

@@ -121,7 +121,7 @@ public override int GetHashCode()
                    return 0;
                }

-                rangeCollection.Add(range);
+                rangeCollection!.Add(range!);


Contract.Requires

analogrelay · 2020-01-02T17:27:10Z

src/Http/Headers/src/BaseHeaderParser.cs

@@ -14,9 +15,9 @@ protected BaseHeaderParser(bool supportsMultipleValues)

        protected abstract int GetParsedValueLength(StringSegment value, int startIndex, out T parsedValue);

-        public sealed override bool TryParseValue(StringSegment value, ref int index, out T parsedValue)
+        public sealed override bool TryParseValue(StringSegment value, ref int index, [MaybeNullWhen(false)]out T parsedValue)


Suggested change

public sealed override bool TryParseValue(StringSegment value, ref int index, [MaybeNullWhen(false)]out T parsedValue)

public sealed override bool TryParseValue(StringSegment value, ref int index, [MaybeNullWhen(false)] out T parsedValue)

nit: I prefer a space here, though I'm willing to be overruled

rynowak · 2020-01-07T00:01:31Z

API review YO

analogrelay · 2020-02-19T21:25:16Z

Sounds like there are some build issues with using nullable annotations here. @JamesNK you can check in with @dotnet/aspnet-build and see if we can figure this out?

wtgodbe · 2020-02-19T21:27:27Z

/azp run

azure-pipelines · 2020-02-19T21:27:32Z

Pull request contains merge conflicts.

dougbu · 2020-02-20T00:34:43Z

Sounds like there are some build issues with using nullable annotations here.

@anurse something beyond the missing Contract.Requires(...) calls that @Tratcher mentioned? Could you be a bit more specific?

analogrelay · 2020-02-20T17:45:51Z

That's just what @JamesNK said in Triage ;).

JamesNK · 2020-02-24T22:34:14Z

No immediate plans to look at this. Can re-open when I have time in the future.

Add nullable annotations to Http.Abstractions, Http.Features, Connect…

0193d73

…ions.Abstractions

JamesNK requested review from davidfowl and rynowak January 2, 2020 09:06

JamesNK requested review from analogrelay, halter73, jkotalik and Tratcher as code owners January 2, 2020 09:06

github-actions bot added the area-servers label Jan 2, 2020

halter73 reviewed Jan 2, 2020

View reviewed changes

Tratcher requested changes Jan 2, 2020

View reviewed changes

Fix build

9308303

rynowak added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 6, 2020

analogrelay reviewed Jan 6, 2020

View reviewed changes

analogrelay added this to the 5.0.0-preview2 milestone Feb 19, 2020

halter73 closed this Feb 24, 2020

dougbu deleted the jamesnk/nullable-httpabstractions branch May 18, 2020 19:44

JamesNK mentioned this pull request May 27, 2020

Add nullability to HtmlAbstractions #22275

Merged

pranavkm mentioned this pull request Jun 3, 2020

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

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JamesNK commented Jan 2, 2020

halter73 Jan 2, 2020

halter73 Jan 2, 2020

Tratcher left a comment

Tratcher Jan 2, 2020

Tratcher Jan 2, 2020

Tratcher Jan 2, 2020

Tratcher Jan 2, 2020

Tratcher Jan 2, 2020

Tratcher Jan 2, 2020

Tratcher Jan 2, 2020

Tratcher Jan 2, 2020

analogrelay Jan 2, 2020

rynowak commented Jan 7, 2020

analogrelay commented Feb 19, 2020

wtgodbe commented Feb 19, 2020

azure-pipelines bot commented Feb 19, 2020

dougbu commented Feb 20, 2020

analogrelay commented Feb 20, 2020 •

edited

Loading

JamesNK commented Feb 24, 2020

	public sealed override bool TryParseValue(StringSegment value, ref int index, [MaybeNullWhen(false)]out T parsedValue)
	public sealed override bool TryParseValue(StringSegment value, ref int index, [MaybeNullWhen(false)] out T parsedValue)

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

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

Conversation

JamesNK commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Jan 7, 2020

analogrelay commented Feb 19, 2020

wtgodbe commented Feb 19, 2020

azure-pipelines bot commented Feb 19, 2020

dougbu commented Feb 20, 2020

analogrelay commented Feb 20, 2020 • edited Loading

JamesNK commented Feb 24, 2020

analogrelay commented Feb 20, 2020 •

edited

Loading