Skip to content

Annotate Microsoft.AspNetCore.Http with nullable attributes #22928

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

Merged
2 commits merged into from
Jun 15, 2020
Merged

Annotate Microsoft.AspNetCore.Http with nullable attributes #22928

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
628edba
Annotate Microsoft.AspNetCore.Http with nullable attributes
pranavkm Jun 14, 2020
bc78cd9
Annotate Hosting with nullable
pranavkm Jun 14, 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
1 change: 1 addition & 0 deletions src/Hosting/Abstractions/ref/Microsoft.AspNetCore.Hosting.Abstractions.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(DefaultNetCoreTargetFramework)</TargetFrameworks>
<Nullable>annotations</Nullable>
</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == '$(DefaultNetCoreTargetFramework)'">
<Compile Include="Microsoft.AspNetCore.Hosting.Abstractions.netcoreapp.cs" />
Expand Down
4 changes: 2 additions & 2 deletions src/Hosting/Abstractions/ref/Microsoft.AspNetCore.Hosting.Abstractions.netcoreapp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ public partial interface IWebHostBuilder
Microsoft.AspNetCore.Hosting.IWebHostBuilder ConfigureAppConfiguration(System.Action<Microsoft.AspNetCore.Hosting.WebHostBuilderContext, Microsoft.Extensions.Configuration.IConfigurationBuilder> configureDelegate);
Microsoft.AspNetCore.Hosting.IWebHostBuilder ConfigureServices(System.Action<Microsoft.AspNetCore.Hosting.WebHostBuilderContext, Microsoft.Extensions.DependencyInjection.IServiceCollection> configureServices);
Microsoft.AspNetCore.Hosting.IWebHostBuilder ConfigureServices(System.Action<Microsoft.Extensions.DependencyInjection.IServiceCollection> configureServices);
string GetSetting(string key);
Microsoft.AspNetCore.Hosting.IWebHostBuilder UseSetting(string key, string value);
string? GetSetting(string key);
Microsoft.AspNetCore.Hosting.IWebHostBuilder UseSetting(string key, string? value);
}
public partial interface IWebHostEnvironment : Microsoft.Extensions.Hosting.IHostEnvironment
{
Expand Down
6 changes: 3 additions & 3 deletions src/Hosting/Abstractions/src/IWebHostBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ public interface IWebHostBuilder
/// </summary>
/// <param name="key">The key of the setting to look up.</param>
/// <returns>The value the setting currently contains.</returns>
string GetSetting(string key);
string? GetSetting(string key);

/// <summary>
/// Add or replace a setting in the configuration.
/// </summary>
/// <param name="key">The key of the setting to add or replace.</param>
/// <param name="value">The value of the setting to add or replace.</param>
/// <returns>The <see cref="IWebHostBuilder"/>.</returns>
IWebHostBuilder UseSetting(string key, string value);
IWebHostBuilder UseSetting(string key, string? value);
}
}
}
1 change: 1 addition & 0 deletions src/Hosting/Abstractions/src/Microsoft.AspNetCore.Hosting.Abstractions.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;hosting</PackageTags>
<IsPackable>false</IsPackable>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand Down
4 changes: 2 additions & 2 deletions src/Hosting/Abstractions/src/WebHostBuilderContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ public class WebHostBuilderContext
/// <summary>
/// The <see cref="IWebHostEnvironment" /> initialized by the <see cref="IWebHost" />.
/// </summary>
public IWebHostEnvironment HostingEnvironment { get; set; }
public IWebHostEnvironment HostingEnvironment { get; set; } = default!;

/// <summary>
/// The <see cref="IConfiguration" /> containing the merged configuration of the application and the <see cref="IWebHost" />.
/// </summary>
public IConfiguration Configuration { get; set; }
public IConfiguration Configuration { get; set; } = default!;
}
}
14 changes: 7 additions & 7 deletions src/Http/Http.Abstractions/ref/Microsoft.AspNetCore.Http.Abstractions.netcoreapp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ protected EndpointBuilder() { }
public partial interface IApplicationBuilder
{
System.IServiceProvider ApplicationServices { get; set; }
System.Collections.Generic.IDictionary<string, object> Properties { get; }
System.Collections.Generic.IDictionary<string, object?> Properties { get; }
Microsoft.AspNetCore.Http.Features.IFeatureCollection ServerFeatures { get; }
Microsoft.AspNetCore.Http.RequestDelegate Build();
Microsoft.AspNetCore.Builder.IApplicationBuilder New();
Expand Down Expand Up @@ -108,13 +108,13 @@ public BadHttpRequestException(string message, int statusCode, System.Exception
public abstract partial class ConnectionInfo
{
protected ConnectionInfo() { }
public abstract System.Security.Cryptography.X509Certificates.X509Certificate2 ClientCertificate { get; set; }
public abstract System.Security.Cryptography.X509Certificates.X509Certificate2? ClientCertificate { get; set; }
public abstract string Id { get; set; }
public abstract System.Net.IPAddress LocalIpAddress { get; set; }
public abstract System.Net.IPAddress? LocalIpAddress { get; set; }
public abstract int LocalPort { get; set; }
public abstract System.Net.IPAddress RemoteIpAddress { get; set; }
public abstract System.Net.IPAddress? RemoteIpAddress { get; set; }
public abstract int RemotePort { get; set; }
public abstract System.Threading.Tasks.Task<System.Security.Cryptography.X509Certificates.X509Certificate2> GetClientCertificateAsync(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
public abstract System.Threading.Tasks.Task<System.Security.Cryptography.X509Certificates.X509Certificate2?> GetClientCertificateAsync(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
}
public partial class CookieBuilder
{
Expand Down Expand Up @@ -231,7 +231,7 @@ protected HttpContext() { }
public abstract System.Threading.CancellationToken RequestAborted { get; set; }
public abstract System.IServiceProvider RequestServices { get; set; }
public abstract Microsoft.AspNetCore.Http.HttpResponse Response { get; }
public abstract Microsoft.AspNetCore.Http.ISession? Session { get; set; }
public abstract Microsoft.AspNetCore.Http.ISession Session { get; set; }
public abstract string TraceIdentifier { get; set; }
public abstract System.Security.Claims.ClaimsPrincipal User { get; set; }
public abstract Microsoft.AspNetCore.Http.WebSocketManager WebSockets { get; }
Expand Down Expand Up @@ -326,7 +326,7 @@ public static partial class HttpResponseWritingExtensions
}
public partial interface IHttpContextAccessor
{
Microsoft.AspNetCore.Http.HttpContext HttpContext { get; set; }
Microsoft.AspNetCore.Http.HttpContext? HttpContext { get; set; }
}
public partial interface IHttpContextFactory
{
Expand Down
8 changes: 4 additions & 4 deletions src/Http/Http.Abstractions/src/ConnectionInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@ public abstract class ConnectionInfo
/// </summary>
public abstract string Id { get; set; }

public abstract IPAddress RemoteIpAddress { get; set; }
public abstract IPAddress? RemoteIpAddress { get; set; }

public abstract int RemotePort { get; set; }

public abstract IPAddress LocalIpAddress { get; set; }
public abstract IPAddress? LocalIpAddress { get; set; }

public abstract int LocalPort { get; set; }

public abstract X509Certificate2 ClientCertificate { get; set; }
public abstract X509Certificate2? ClientCertificate { get; set; }

public abstract Task<X509Certificate2> GetClientCertificateAsync(CancellationToken cancellationToken = new CancellationToken());
public abstract Task<X509Certificate2?> GetClientCertificateAsync(CancellationToken cancellationToken = new CancellationToken());
}
}
2 changes: 1 addition & 1 deletion src/Http/Http.Abstractions/src/HttpContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public abstract class HttpContext
/// <summary>
/// Gets or sets the object used to manage user session data for this request.
/// </summary>
public abstract ISession? Session { get; set; }
public abstract ISession Session { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

? Nothing prevents Session from returning null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the feature throws if it hasn't been set up.

Copy link
Member

Choose a reason for hiding this comment

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

DefaultHttpContext throws if the feature is missing, but it doesn't ensure anything about the actual ISession value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess. But that's true of nearly all of the features, no? We don't verify that the feature has been configured weirdly. In theory, most features could be implemented to return null values, but it would be incredibly cumbersome to use these APIs if they were all marked as nullable. In the ordinary case, it's either non-null or throws, no?

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to apply some pragmatism with nullable attributes.

Yes something could technically be null if an app was setup to do an unusual thing, but I think we need to focus on the 99% usage.

Copy link
Member

Choose a reason for hiding this comment

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

The question is if these annotations are trying to communicate "SHOULD NOT" or "MUST NOT" be null (RFC terms). If we stick with marking the common nullable cases that should help people avoid common mistakes.

If we find later we're too loose or too strict with these annotations, how breaking is it to change them? Doesn't it introduce new warnings either way?


/// <summary>
/// Aborts the connection underlying this request.
Expand Down
2 changes: 1 addition & 1 deletion src/Http/Http.Abstractions/src/IApplicationBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public interface IApplicationBuilder
/// <summary>
/// Gets a key/value collection that can be used to share data between middleware.
/// </summary>
IDictionary<string, object> Properties { get; }
IDictionary<string, object?> Properties { get; }

/// <summary>
/// Adds a middleware delegate to the application's request pipeline.
Expand Down
2 changes: 1 addition & 1 deletion src/Http/Http.Abstractions/src/IHttpContextAccessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@ public interface IHttpContextAccessor
/// <summary>
/// Gets or sets the current <see cref="HttpContext"/>. Returns <see langword="null" /> if there is no active <see cref="HttpContext" />.
/// </summary>
HttpContext HttpContext { get; set; }
HttpContext? HttpContext { get; set; }
}
}
28 changes: 14 additions & 14 deletions src/Http/Http.Features/ref/Microsoft.AspNetCore.Http.Features.netcoreapp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ public partial interface IFormFile
}
public partial interface IFormFileCollection : System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Http.IFormFile>, System.Collections.Generic.IReadOnlyCollection<Microsoft.AspNetCore.Http.IFormFile>, System.Collections.Generic.IReadOnlyList<Microsoft.AspNetCore.Http.IFormFile>, System.Collections.IEnumerable
{
Microsoft.AspNetCore.Http.IFormFile this[string name] { get; }
Microsoft.AspNetCore.Http.IFormFile GetFile(string name);
Microsoft.AspNetCore.Http.IFormFile? this[string name] { get; }
Microsoft.AspNetCore.Http.IFormFile? GetFile(string name);
System.Collections.Generic.IReadOnlyList<Microsoft.AspNetCore.Http.IFormFile> GetFiles(string name);
}
public partial interface IHeaderDictionary : System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<string, Microsoft.Extensions.Primitives.StringValues>>, System.Collections.Generic.IDictionary<string, Microsoft.Extensions.Primitives.StringValues>, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Microsoft.Extensions.Primitives.StringValues>>, System.Collections.IEnumerable
Expand All @@ -58,10 +58,10 @@ public partial interface IQueryCollection : System.Collections.Generic.IEnumerab
public partial interface IRequestCookieCollection : System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, string>>, System.Collections.IEnumerable
{
int Count { get; }
string this[string key] { get; }
string? this[string key] { get; }
System.Collections.Generic.ICollection<string> Keys { get; }
bool ContainsKey(string key);
bool TryGetValue(string key, out string value);
bool TryGetValue(string key, [System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute(false)] out string? value);
}
public partial interface IResponseCookies
{
Expand Down Expand Up @@ -121,8 +121,8 @@ public partial struct FeatureReferences<TCache>
public FeatureReferences(Microsoft.AspNetCore.Http.Features.IFeatureCollection collection) { throw null; }
public Microsoft.AspNetCore.Http.Features.IFeatureCollection Collection { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } }
public int Revision { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } }
public TFeature Fetch<TFeature>([System.Diagnostics.CodeAnalysis.AllowNullAttribute, System.Diagnostics.CodeAnalysis.MaybeNullAttribute] ref TFeature cached, System.Func<Microsoft.AspNetCore.Http.Features.IFeatureCollection, TFeature> factory) where TFeature : class { throw null; }
[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]public TFeature Fetch<TFeature, TState>([System.Diagnostics.CodeAnalysis.AllowNullAttribute, System.Diagnostics.CodeAnalysis.MaybeNullAttribute] ref TFeature cached, TState state, System.Func<TState, TFeature> factory) where TFeature : class { throw null; }
public TFeature Fetch<TFeature>([System.Diagnostics.CodeAnalysis.AllowNullAttribute, System.Diagnostics.CodeAnalysis.MaybeNullAttribute] ref TFeature cached, System.Func<Microsoft.AspNetCore.Http.Features.IFeatureCollection, TFeature> factory) where TFeature : class? { throw null; }
[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]public TFeature Fetch<TFeature, TState>([System.Diagnostics.CodeAnalysis.AllowNullAttribute, System.Diagnostics.CodeAnalysis.MaybeNullAttribute] ref TFeature cached, TState state, System.Func<TState, TFeature> factory) where TFeature : class? { throw null; }
[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]public void Initalize(Microsoft.AspNetCore.Http.Features.IFeatureCollection collection) { }
[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]public void Initalize(Microsoft.AspNetCore.Http.Features.IFeatureCollection collection, int revision) { }
}
Expand Down Expand Up @@ -152,7 +152,7 @@ public partial interface IFeatureCollection : System.Collections.Generic.IEnumer
}
public partial interface IFormFeature
{
Microsoft.AspNetCore.Http.IFormCollection Form { get; set; }
Microsoft.AspNetCore.Http.IFormCollection? Form { get; set; }
bool HasFormContentType { get; }
Microsoft.AspNetCore.Http.IFormCollection ReadForm();
System.Threading.Tasks.Task<Microsoft.AspNetCore.Http.IFormCollection> ReadFormAsync(System.Threading.CancellationToken cancellationToken);
Expand All @@ -170,9 +170,9 @@ public partial interface IHttpBufferingFeature
public partial interface IHttpConnectionFeature
{
string ConnectionId { get; set; }
System.Net.IPAddress LocalIpAddress { get; set; }
System.Net.IPAddress? LocalIpAddress { get; set; }
int LocalPort { get; set; }
System.Net.IPAddress RemoteIpAddress { get; set; }
System.Net.IPAddress? RemoteIpAddress { get; set; }
int RemotePort { get; set; }
}
public partial interface IHttpMaxRequestBodySizeFeature
Expand Down Expand Up @@ -225,7 +225,7 @@ public partial interface IHttpResponseFeature
System.IO.Stream Body { get; set; }
bool HasStarted { get; }
Microsoft.AspNetCore.Http.IHeaderDictionary Headers { get; set; }
string ReasonPhrase { get; set; }
string? ReasonPhrase { get; set; }
int StatusCode { get; set; }
void OnCompleted(System.Func<object, System.Threading.Tasks.Task> callback, object state);
void OnStarting(System.Func<object, System.Threading.Tasks.Task> callback, object state);
Expand Down Expand Up @@ -255,7 +255,7 @@ public partial interface IHttpWebSocketFeature
}
public partial interface IItemsFeature
{
System.Collections.Generic.IDictionary<object, object> Items { get; set; }
System.Collections.Generic.IDictionary<object, object?> Items { get; set; }
}
public partial interface IQueryFeature
{
Expand Down Expand Up @@ -287,8 +287,8 @@ public partial interface ISessionFeature
}
public partial interface ITlsConnectionFeature
{
System.Security.Cryptography.X509Certificates.X509Certificate2 ClientCertificate { get; set; }
System.Threading.Tasks.Task<System.Security.Cryptography.X509Certificates.X509Certificate2> GetClientCertificateAsync(System.Threading.CancellationToken cancellationToken);
System.Security.Cryptography.X509Certificates.X509Certificate2? ClientCertificate { get; set; }
System.Threading.Tasks.Task<System.Security.Cryptography.X509Certificates.X509Certificate2?> GetClientCertificateAsync(System.Threading.CancellationToken cancellationToken);
}
public partial interface ITlsTokenBindingFeature
{
Expand All @@ -309,6 +309,6 @@ namespace Microsoft.AspNetCore.Http.Features.Authentication
{
public partial interface IHttpAuthenticationFeature
{
System.Security.Claims.ClaimsPrincipal User { get; set; }
System.Security.Claims.ClaimsPrincipal? User { get; set; }
}
}
Loading