Skip to content

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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0193d73
Add nullable annotations to Http.Abstractions, Http.Features, Connect…
JamesNK Jan 2, 2020
9308303
Fix build
JamesNK Jan 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/Http/Headers/src/BaseHeaderParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Diagnostics.CodeAnalysis;
using Microsoft.Extensions.Primitives;

namespace Microsoft.Net.Http.Headers
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

{
parsedValue = default(T);
parsedValue = default(T)!;

// If multiple values are supported (i.e. list of values), then accept an empty string: The header may
// be added multiple times to the request/response message. E.g.
Expand Down
23 changes: 11 additions & 12 deletions src/Http/Headers/src/CacheControlHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!);
Copy link
Member

Choose a reason for hiding this comment

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

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?


private static readonly Action<StringSegment> CheckIsValidTokenAction = CheckIsValidToken;

private bool _noCache;
private ICollection<StringSegment> _noCacheHeaders;
private ICollection<StringSegment>? _noCacheHeaders;
private bool _noStore;
private TimeSpan? _maxAge;
private TimeSpan? _sharedMaxAge;
Expand All @@ -47,10 +47,10 @@ private static readonly HttpHeaderParser<CacheControlHeaderValue> Parser
private bool _onlyIfCached;
private bool _public;
private bool _private;
private ICollection<StringSegment> _privateHeaders;
private ICollection<StringSegment>? _privateHeaders;
private bool _mustRevalidate;
private bool _proxyRevalidate;
private IList<NameValueHeaderValue> _extensions;
private IList<NameValueHeaderValue>? _extensions;

public CacheControlHeaderValue()
{
Expand Down Expand Up @@ -240,7 +240,7 @@ public override string ToString()
return sb.ToString();
}

public override bool Equals(object obj)
public override bool Equals(object? obj)
{
var other = obj as CacheControlHeaderValue;

Expand Down Expand Up @@ -335,7 +335,7 @@ public static CacheControlHeaderValue Parse(StringSegment input)
return result;
}

public static bool TryParse(StringSegment input, out CacheControlHeaderValue parsedValue)
public static bool TryParse(StringSegment input, out CacheControlHeaderValue? parsedValue)
{
int index = 0;
// Cache-Control is unusual because there are no required values so the parser will succeed for an empty string, but still return null.
Expand All @@ -347,7 +347,7 @@ public static bool TryParse(StringSegment input, out CacheControlHeaderValue par
return false;
}

private static int GetCacheControlLength(StringSegment input, int startIndex, out CacheControlHeaderValue parsedValue)
private static int GetCacheControlLength(StringSegment input, int startIndex, out CacheControlHeaderValue? parsedValue)
{
Contract.Requires(startIndex >= 0);

Expand All @@ -361,11 +361,10 @@ private static int GetCacheControlLength(StringSegment input, int startIndex, ou
// Cache-Control header consists of a list of name/value pairs, where the value is optional. So use an
// instance of NameValueHeaderParser to parse the string.
var current = startIndex;
NameValueHeaderValue nameValue = null;
var nameValueList = new List<NameValueHeaderValue>();
while (current < input.Length)
{
if (!NameValueHeaderValue.MultipleValueParser.TryParseValue(input, ref current, out nameValue))
if (!NameValueHeaderValue.MultipleValueParser.TryParseValue(input, ref current, out var nameValue))
{
return 0;
}
Expand Down Expand Up @@ -539,11 +538,11 @@ private static bool TrySetTokenOnlyValue(NameValueHeaderValue nameValue, ref boo
private static bool TrySetOptionalTokenList(
NameValueHeaderValue nameValue,
ref bool boolField,
ref ICollection<StringSegment> destination)
ref ICollection<StringSegment>? destination)
{
Contract.Requires(nameValue != null);

if (nameValue.Value == null)
if (nameValue!.Value == null)
{
boolField = true;
return true;
Expand Down Expand Up @@ -605,7 +604,7 @@ private static bool TrySetTimeSpan(NameValueHeaderValue nameValue, ref TimeSpan?
{
Contract.Requires(nameValue != null);

if (nameValue.Value == null)
if (nameValue!.Value == null)
Copy link
Member

Choose a reason for hiding this comment

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

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

{
return false;
}
Expand Down
27 changes: 14 additions & 13 deletions src/Http/Headers/src/ContentDispositionHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Buffers;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Contracts;
using System.Globalization;
using System.Linq;
Expand All @@ -26,10 +27,10 @@ public class ContentDispositionHeaderValue
private static readonly char[] SingleQuote = new char[] { '\'' };

private static readonly HttpHeaderParser<ContentDispositionHeaderValue> Parser
= new GenericHeaderParser<ContentDispositionHeaderValue>(false, GetDispositionTypeLength);
= new GenericHeaderParser<ContentDispositionHeaderValue>(false, GetDispositionTypeLength!);

// Use list instead of dictionary since we may have multiple parameters with the same name.
private ObjectCollection<NameValueHeaderValue> _parameters;
private ObjectCollection<NameValueHeaderValue>? _parameters;
private StringSegment _dispositionType;

private ContentDispositionHeaderValue()
Expand Down Expand Up @@ -128,7 +129,7 @@ public long? Size
// Remove parameter
if (sizeParameter != null)
{
_parameters.Remove(sizeParameter);
_parameters!.Remove(sizeParameter);
}
}
else if (value < 0)
Expand All @@ -142,7 +143,7 @@ public long? Size
else
{
string sizeString = value.GetValueOrDefault().ToString(CultureInfo.InvariantCulture);
_parameters.Add(new NameValueHeaderValue(SizeString, sizeString));
_parameters!.Add(new NameValueHeaderValue(SizeString, sizeString));
Copy link
Member

Choose a reason for hiding this comment

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

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.

}
}
}
Expand Down Expand Up @@ -180,7 +181,7 @@ public override string ToString()
return _dispositionType + NameValueHeaderValue.ToString(_parameters, ';', true);
}

public override bool Equals(object obj)
public override bool Equals(object? obj)
{
var other = obj as ContentDispositionHeaderValue;

Expand All @@ -205,13 +206,13 @@ public static ContentDispositionHeaderValue Parse(StringSegment input)
return Parser.ParseValue(input, ref index);
}

public static bool TryParse(StringSegment input, out ContentDispositionHeaderValue parsedValue)
public static bool TryParse(StringSegment input, [NotNullWhen(true)]out ContentDispositionHeaderValue? parsedValue)
{
var index = 0;
return Parser.TryParseValue(input, ref index, out parsedValue);
}

private static int GetDispositionTypeLength(StringSegment input, int startIndex, out ContentDispositionHeaderValue parsedValue)
private static int GetDispositionTypeLength(StringSegment input, int startIndex, out ContentDispositionHeaderValue? parsedValue)
{
Contract.Requires(startIndex >= 0);

Expand Down Expand Up @@ -318,7 +319,7 @@ private void SetDate(string parameter, DateTimeOffset? date)
// Remove parameter
if (dateParameter != null)
{
_parameters.Remove(dateParameter);
_parameters!.Remove(dateParameter);
}
}
else
Expand All @@ -343,7 +344,7 @@ private StringSegment GetName(string parameter)
var nameParameter = NameValueHeaderValue.Find(_parameters, parameter);
if (nameParameter != null)
{
string result;
string? result;
// filename*=utf-8'lang'%7FMyString
if (parameter.EndsWith("*", StringComparison.Ordinal))
{
Expand Down Expand Up @@ -375,7 +376,7 @@ private void SetName(StringSegment parameter, StringSegment value)
// Remove parameter
if (nameParameter != null)
{
_parameters.Remove(nameParameter);
_parameters!.Remove(nameParameter);
}
}
else
Expand Down Expand Up @@ -497,7 +498,7 @@ private unsafe string EncodeMime(StringSegment input)
}

// Attempt to decode MIME encoded strings
private bool TryDecodeMime(StringSegment input, out string output)
private bool TryDecodeMime(StringSegment input, [NotNullWhen(true)]out string? output)
{
Contract.Assert(input != null);

Expand Down Expand Up @@ -582,7 +583,7 @@ private static void HexEscape(StringBuilder builder, char c)

// Attempt to decode using RFC 5987 encoding.
// encoding'language'my%20string
private bool TryDecode5987(StringSegment input, out string output)
private bool TryDecode5987(StringSegment input, [NotNullWhen(true)]out string? output)
{
output = null;

Expand All @@ -593,7 +594,7 @@ private bool TryDecode5987(StringSegment input, out string output)
}

var decoded = new StringBuilder();
byte[] unescapedBytes = null;
byte[]? unescapedBytes = null;
try
{
var encoding = Encoding.GetEncoding(parts[0].ToString());
Expand Down
9 changes: 5 additions & 4 deletions src/Http/Headers/src/ContentRangeHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Contracts;
using System.Globalization;
using System.Text;
Expand All @@ -12,7 +13,7 @@ namespace Microsoft.Net.Http.Headers
public class ContentRangeHeaderValue
{
private static readonly HttpHeaderParser<ContentRangeHeaderValue> Parser
= new GenericHeaderParser<ContentRangeHeaderValue>(false, GetContentRangeLength);
= new GenericHeaderParser<ContentRangeHeaderValue>(false, GetContentRangeLength!);

private StringSegment _unit;
private long? _from;
Expand Down Expand Up @@ -113,7 +114,7 @@ public long? Length
get { return _from != null; }
}

public override bool Equals(object obj)
public override bool Equals(object? obj)
{
var other = obj as ContentRangeHeaderValue;

Expand Down Expand Up @@ -185,7 +186,7 @@ public static bool TryParse(StringSegment input, out ContentRangeHeaderValue par
return Parser.TryParseValue(input, ref index, out parsedValue);
}

private static int GetContentRangeLength(StringSegment input, int startIndex, out ContentRangeHeaderValue parsedValue)
private static int GetContentRangeLength(StringSegment input, int startIndex, out ContentRangeHeaderValue? parsedValue)
{
Contract.Requires(startIndex >= 0);

Expand Down Expand Up @@ -351,7 +352,7 @@ private static bool TryCreateContentRange(
int toLength,
int lengthStartIndex,
int lengthLength,
out ContentRangeHeaderValue parsedValue)
[NotNullWhen(true)]out ContentRangeHeaderValue? parsedValue)
{
parsedValue = null;

Expand Down
7 changes: 5 additions & 2 deletions src/Http/Headers/src/CookieHeaderParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Contracts;
using Microsoft.Extensions.Primitives;

Expand All @@ -13,7 +14,9 @@ internal CookieHeaderParser(bool supportsMultipleValues)
{
}

public override bool TryParseValue(StringSegment value, ref int index, out CookieHeaderValue parsedValue)
#pragma warning disable CS8610 // Nullability of reference types in type of parameter doesn't match overridden member.
public override bool TryParseValue(StringSegment value, ref int index, [NotNullWhen(true)]out CookieHeaderValue? parsedValue)
#pragma warning restore CS8610 // Nullability of reference types in type of parameter doesn't match overridden member.
{
parsedValue = null;

Expand Down Expand Up @@ -43,7 +46,7 @@ public override bool TryParseValue(StringSegment value, ref int index, out Cooki
return SupportsMultipleValues;
}

CookieHeaderValue result = null;
CookieHeaderValue? result = null;
if (!CookieHeaderValue.TryGetCookieLength(value, ref current, out result))
{
return false;
Expand Down
11 changes: 6 additions & 5 deletions src/Http/Headers/src/CookieHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Contracts;
using System.Text;
using Microsoft.Extensions.Primitives;
Expand Down Expand Up @@ -86,7 +87,7 @@ public static CookieHeaderValue Parse(StringSegment input)
return SingleValueParser.ParseValue(input, ref index);
}

public static bool TryParse(StringSegment input, out CookieHeaderValue parsedValue)
public static bool TryParse(StringSegment input, [NotNullWhen(true)]out CookieHeaderValue? parsedValue)
{
var index = 0;
return SingleValueParser.TryParseValue(input, ref index, out parsedValue);
Expand All @@ -102,18 +103,18 @@ public static IList<CookieHeaderValue> ParseStrictList(IList<string> inputs)
return MultipleValueParser.ParseStrictValues(inputs);
}

public static bool TryParseList(IList<string> inputs, out IList<CookieHeaderValue> parsedValues)
public static bool TryParseList(IList<string> inputs, [NotNullWhen(true)]out IList<CookieHeaderValue>? parsedValues)
{
return MultipleValueParser.TryParseValues(inputs, out parsedValues);
}

public static bool TryParseStrictList(IList<string> inputs, out IList<CookieHeaderValue> parsedValues)
public static bool TryParseStrictList(IList<string> inputs, [NotNullWhen(true)]out IList<CookieHeaderValue>? parsedValues)
{
return MultipleValueParser.TryParseStrictValues(inputs, out parsedValues);
}

// name=value; name="value"
internal static bool TryGetCookieLength(StringSegment input, ref int offset, out CookieHeaderValue parsedValue)
internal static bool TryGetCookieLength(StringSegment input, ref int offset, [NotNullWhen(true)]out CookieHeaderValue? parsedValue)
{
Contract.Requires(offset >= 0);

Expand Down Expand Up @@ -256,7 +257,7 @@ internal static void CheckValueFormat(StringSegment value, string parameterName)
}
}

public override bool Equals(object obj)
public override bool Equals(object? obj)
{
var other = obj as CookieHeaderValue;

Expand Down
Loading