Skip to content
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.

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

Soft enable CA1810 #33619

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ dotnet_diagnostic.CA1802.severity = warning
# CA1805: Do not initialize unnecessarily
dotnet_diagnostic.CA1805.severity = warning

# CA1810: Do not initialize unnecessarily
dotnet_diagnostic.CA1810.severity = suggestion

# CA1823: Remove empty Finalizers
dotnet_diagnostic.CA1821.severity = warning

Expand Down
2 changes: 1 addition & 1 deletion eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
<MicrosoftExtensionsHostingAbstractionsVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsHostingAbstractionsVersion>
<MicrosoftExtensionsHostingVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsHostingVersion>
<MicrosoftExtensionsHttpVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsHttpVersion>
<MicrosoftExtensionsLoggingAbstractionsVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsLoggingAbstractionsVersion>
<MicrosoftExtensionsLoggingAbstractionsVersion>6.0.0-preview.7.21316.3</MicrosoftExtensionsLoggingAbstractionsVersion>
Copy link
Member

Choose a reason for hiding this comment

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

This will simply cause conflicts or be overwritten the next time we get versions from dotnet/runtime. Suggest retrying after #33560 is in.

<MicrosoftExtensionsLoggingConfigurationVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsLoggingConfigurationVersion>
<MicrosoftExtensionsLoggingConsoleVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsLoggingConsoleVersion>
<MicrosoftExtensionsLoggingDebugVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsLoggingDebugVersion>
Expand Down
6 changes: 3 additions & 3 deletions src/Components/Web/src/Forms/InputNumber.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ namespace Microsoft.AspNetCore.Components.Forms
/// </summary>
public class InputNumber<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TValue> : InputBase<TValue>
{
private readonly static string _stepAttributeValue; // Null by default, so only allows whole numbers as per HTML spec
private static readonly string _stepAttributeValue = GetStepAttributeValue();

static InputNumber()
private static string GetStepAttributeValue()
{
// Unwrap Nullable<T>, because InputBase already deals with the Nullable aspect
// of it for us. We will only get asked to parse the T for nonempty inputs.
Expand All @@ -28,7 +28,7 @@ static InputNumber()
targetType == typeof(double) ||
targetType == typeof(decimal))
{
_stepAttributeValue = "any";
return "any";
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,8 @@ internal static class RendererRegistry
// as well as rooting them for GC purposes, since nothing would otherwise be referencing
// them even though we might still receive incoming events from JS.

private static readonly Dictionary<int, WebAssemblyRenderer>? _renderers = OperatingSystem.IsBrowser() ? new() : null;
private static int _nextId;
private static Dictionary<int, WebAssemblyRenderer>? _renderers;

static RendererRegistry()
{
if (OperatingSystem.IsBrowser())
{
_renderers = new Dictionary<int, WebAssemblyRenderer>();
}
}

internal static WebAssemblyRenderer Find(int rendererId)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,21 +163,16 @@ protected override void HandleException(Exception exception)

private static class Log
{
private static readonly Action<ILogger, string, Exception> _unhandledExceptionRenderingComponent;
private static readonly Action<ILogger, string, Exception> _unhandledExceptionRenderingComponent = LoggerMessage.Define<string>(
Copy link
Member

Choose a reason for hiding this comment

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

or maybe something like below?

[LoggerMessage(EventId = EventIds.UnhandledExceptionRenderingComponent, Level = LogLevel.Critical, Message = "Unhandled exception rendering component: {Message}")]
public static partial void UnhandledExceptionRenderingComponent(ILogger logger, string message);

LogLevel.Critical,
EventIds.UnhandledExceptionRenderingComponent,
"Unhandled exception rendering component: {Message}");

private static class EventIds
{
public static readonly EventId UnhandledExceptionRenderingComponent = new EventId(100, "ExceptionRenderingComponent");
}

static Log()
{
_unhandledExceptionRenderingComponent = LoggerMessage.Define<string>(
LogLevel.Critical,
EventIds.UnhandledExceptionRenderingComponent,
"Unhandled exception rendering component: {Message}");
}

public static void UnhandledExceptionRenderingComponent(ILogger logger, Exception exception)
{
_unhandledExceptionRenderingComponent(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,13 @@ namespace Microsoft.AspNetCore.Components.WebAssembly.Services
internal class WebAssemblyConsoleLogger<T> : ILogger<T>, ILogger
{
private const string _loglevelPadding = ": ";
private static readonly string _messagePadding;
private static readonly string _newLineWithMessagePadding;
private static readonly string _messagePadding = new(' ', GetLogLevelString(LogLevel.Information).Length + _loglevelPadding.Length);
private static readonly string _newLineWithMessagePadding = Environment.NewLine + _messagePadding;
private static readonly StringBuilder _logBuilder = new StringBuilder();

private readonly string _name;
private readonly WebAssemblyJSRuntime _jsRuntime;

static WebAssemblyConsoleLogger()
{
var logLevelString = GetLogLevelString(LogLevel.Information);
_messagePadding = new string(' ', logLevelString.Length + _loglevelPadding.Length);
_newLineWithMessagePadding = Environment.NewLine + _messagePadding;
}

public WebAssemblyConsoleLogger(IJSRuntime jsRuntime)
: this(string.Empty, (WebAssemblyJSRuntime)jsRuntime) // Cast for DI
{
Expand Down
20 changes: 5 additions & 15 deletions src/Hosting/Hosting/src/Internal/HostingLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,28 +11,18 @@

namespace Microsoft.AspNetCore.Hosting
{
internal static class HostingLoggerExtensions
internal static partial class HostingLoggerExtensions
{
private static readonly Action<ILogger, string, Exception?> _startupAssemblyLoaded =
LoggerMessage.Define<string>(LogLevel.Debug, LoggerEventIds.HostingStartupAssemblyLoaded, "Loaded hosting startup assembly {assemblyName}", skipEnabledCheck: true);

private static readonly Action<ILogger, string, Exception?> _listeningOnAddress =
LoggerMessage.Define<string>(LogLevel.Information, LoggerEventIds.ServerListeningOnAddresses, "Now listening on: {address}");

public static IDisposable RequestScope(this ILogger logger, HttpContext httpContext)
{
return logger.BeginScope(new HostingLogScope(httpContext));
}

public static void ListeningOnAddress(this ILogger logger, string address)
{
_listeningOnAddress(logger, address, null);
}
[LoggerMessage(EventId = LoggerEventIds.ServerListeningOnAddresses, EventName = "ServerListeningOnAddresses", Level = LogLevel.Information, Message = "Now listening on: {address}")]
Copy link
Member

@maryamariyan maryamariyan Jun 18, 2021

Choose a reason for hiding this comment

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

Note, when you don't specify EventName, it would take the method name as the event name for the log method.

Instead of setting EventName, I would rename this method from ListeningOnAddress to ServerListeningOnAddresses.

Copy link
Member

Choose a reason for hiding this comment

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

We want to be explicit about the event name. It is easy for someone to unknowingly change the event name by renaming a method. That would introduce a breaking change.

That is the reason why we have strings today instead of using nameof(MethodName).

public static partial void ListeningOnAddress(this ILogger logger, string address);

public static void StartupAssemblyLoaded(this ILogger logger, string assemblyName)
{
_startupAssemblyLoaded(logger, assemblyName, null);
}
[LoggerMessage(EventId = LoggerEventIds.HostingStartupAssemblyLoaded, EventName = "HostingStartupAssemblyLoaded", Level = LogLevel.Debug, Message = "Loaded hosting startup assembly {assemblyName}")]
public static partial void StartupAssemblyLoaded(this ILogger logger, string assemblyName);

public static void ApplicationError(this ILogger logger, Exception exception)
{
Expand Down
29 changes: 14 additions & 15 deletions src/Hosting/Hosting/src/Internal/LoggerEventIds.cs
Original file line number Diff line number Diff line change
@@ -1,24 +1,23 @@
// 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 Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Hosting
{
internal static class LoggerEventIds
{
public static readonly EventId RequestStarting = new EventId(1, "RequestStarting");
public static readonly EventId RequestFinished = new EventId(2, "RequestFinished");
public static readonly EventId Starting = new EventId(3, "Starting");
public static readonly EventId Started = new EventId(4, "Started");
public static readonly EventId Shutdown = new EventId(5, "Shutdown");
public static readonly EventId ApplicationStartupException = new EventId(6, "ApplicationStartupException");
public static readonly EventId ApplicationStoppingException = new EventId(7, "ApplicationStoppingException");
public static readonly EventId ApplicationStoppedException = new EventId(8, "ApplicationStoppedException");
public static readonly EventId HostedServiceStartException = new EventId(9, "HostedServiceStartException");
public static readonly EventId HostedServiceStopException = new EventId(10, "HostedServiceStopException");
public static readonly EventId HostingStartupAssemblyException = new EventId(11, "HostingStartupAssemblyException");
public static readonly EventId ServerShutdownException = new EventId(12, "ServerShutdownException");
public static readonly EventId HostingStartupAssemblyLoaded = new EventId(13, "HostingStartupAssemblyLoaded");
public static readonly EventId ServerListeningOnAddresses = new EventId(14, "ServerListeningOnAddresses");
public const int RequestStarting = 1;
public const int RequestFinished = 2;
public const int Starting = 3;
public const int Started = 4;
public const int Shutdown = 5;
public const int ApplicationStartupException = 6;
public const int ApplicationStoppingException = 7;
public const int ApplicationStoppedException = 8;
public const int HostedServiceStartException = 9;
public const int HostedServiceStopException = 10;
public const int HostingStartupAssemblyException = 11;
public const int ServerShutdownException = 12;
public const int HostingStartupAssemblyLoaded = 13;
public const int ServerListeningOnAddresses = 14;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Microsoft.AspNetCore.Http.HttpResponse</Description>
<ItemGroup>
<Reference Include="Microsoft.AspNetCore.Http.Features" />
<Reference Include="Microsoft.Net.Http.Headers" />
<Reference Include="Microsoft.Extensions.Logging.Abstractions" />

<Compile Include="$(SharedSourceRoot)ActivatorUtilities\*.cs" />
<Compile Include="$(SharedSourceRoot)ParameterDefaultValue\*.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
<Reference Include="Microsoft.AspNetCore.Http.Abstractions" />
<Reference Include="Microsoft.Net.Http.Headers" />
<Reference Include="Microsoft.Extensions.FileProviders.Abstractions" />
<Reference Include="Microsoft.Extensions.Logging.Abstractions" PrivateAssets="All" />
</ItemGroup>

</Project>
40 changes: 14 additions & 26 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ namespace Microsoft.AspNetCore.Http
/// <summary>
/// Creates <see cref="RequestDelegate"/> implementations from <see cref="Delegate"/> request handlers.
/// </summary>
public static class RequestDelegateFactory
public static partial class RequestDelegateFactory
{
private static readonly MethodInfo ExecuteTaskOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTask), BindingFlags.NonPublic | BindingFlags.Static)!;
private static readonly MethodInfo ExecuteTaskOfStringMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTaskOfString), BindingFlags.NonPublic | BindingFlags.Static)!;
Expand Down Expand Up @@ -809,37 +809,25 @@ private class FactoryContext
public List<(ParameterExpression, Expression)> TryParseParams { get; } = new();
}

private static class Log
private static partial class Log
{
private static readonly Action<ILogger, Exception> _requestBodyIOException = LoggerMessage.Define(
LogLevel.Debug,
new EventId(1, "RequestBodyIOException"),
"Reading the request body failed with an IOException.");
public static void RequestBodyIOException(HttpContext httpContext, Exception exception)
=> RequestBodyIOException(GetLogger(httpContext), exception);

private static readonly Action<ILogger, Exception> _requestBodyInvalidDataException = LoggerMessage.Define(
LogLevel.Debug,
new EventId(2, "RequestBodyInvalidDataException"),
"Reading the request body failed with an InvalidDataException.");

private static readonly Action<ILogger, string, string, string, Exception?> _parameterBindingFailed = LoggerMessage.Define<string, string, string>(
LogLevel.Debug,
new EventId(3, "ParamaterBindingFailed"),
@"Failed to bind parameter ""{ParameterType} {ParameterName}"" from ""{SourceValue}"".");

public static void RequestBodyIOException(HttpContext httpContext, IOException exception)
{
_requestBodyIOException(GetLogger(httpContext), exception);
}
[LoggerMessage(EventId = 1, EventName = "RequestBodyIOException", Level = LogLevel.Debug, Message = "Reading the request body failed with an IOException.")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[LoggerMessage(EventId = 1, EventName = "RequestBodyIOException", Level = LogLevel.Debug, Message = "Reading the request body failed with an IOException.")]
[LoggerMessage(EventId = 1, Level = LogLevel.Debug, Message = "Reading the request body failed with an IOException.")]

public static partial void RequestBodyIOException(ILogger logger, Exception exception);

public static void RequestBodyInvalidDataException(HttpContext httpContext, InvalidDataException exception)
{
_requestBodyInvalidDataException(GetLogger(httpContext), exception);
}
=> RequestBodyInvalidDataException(GetLogger(httpContext), exception);

[LoggerMessage(EventId = 2, EventName = "RequestBodyInvalidDataException", Level = LogLevel.Debug, Message = "Reading the request body failed with an InvalidDataException.")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[LoggerMessage(EventId = 2, EventName = "RequestBodyInvalidDataException", Level = LogLevel.Debug, Message = "Reading the request body failed with an InvalidDataException.")]
[LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = "Reading the request body failed with an InvalidDataException.")]

public static partial void RequestBodyInvalidDataException(ILogger logger, InvalidDataException exception);

public static void ParameterBindingFailed(HttpContext httpContext, string parameterTypeName, string parameterName, string sourceValue)
{
_parameterBindingFailed(GetLogger(httpContext), parameterTypeName, parameterName, sourceValue, null);
}
=> ParameterBindingFailed(GetLogger(httpContext), parameterTypeName, parameterName, sourceValue);

[LoggerMessage(EventId = 3, EventName = "ParamaterBindingFailed", Level = LogLevel.Debug, Message = @"Failed to bind parameter ""{ParameterType} {ParameterName}"" from ""{SourceValue}"".")]
public static partial void ParameterBindingFailed(ILogger logger, string parameterType, string parameterName, string sourceValue);
Copy link
Member

@maryamariyan maryamariyan Jun 18, 2021

Choose a reason for hiding this comment

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

ParamaterBindingFailed vs. ParameterBindingFailed


private static ILogger GetLogger(HttpContext httpContext)
{
Expand Down
15 changes: 4 additions & 11 deletions src/Http/Http/src/Internal/ResponseCookies.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Http
/// <summary>
/// A wrapper for the response Set-Cookie header.
/// </summary>
internal class ResponseCookies : IResponseCookies
internal sealed partial class ResponseCookies : IResponseCookies
{
internal const string EnableCookieNameEncoding = "Microsoft.AspNetCore.Http.EnableCookieNameEncoding";
internal bool _enableCookieNameEncoding = AppContext.TryGetSwitch(EnableCookieNameEncoding, out var enabled) && enabled;
Expand Down Expand Up @@ -212,17 +212,10 @@ public void Delete(string key, CookieOptions options)
});
}

private static class Log
private static partial class Log
{
private static readonly Action<ILogger, string, Exception?> _samesiteNotSecure = LoggerMessage.Define<string>(
LogLevel.Warning,
EventIds.SameSiteNotSecure,
"The cookie '{name}' has set 'SameSite=None' and must also set 'Secure'.");

public static void SameSiteCookieNotSecure(ILogger logger, string name)
{
_samesiteNotSecure(logger, name, null);
}
[LoggerMessage(EventId = 1, EventName = "SameSiteNotSecure", Level = LogLevel.Warning, Message = "The cookie '{name}' has set 'SameSite=None' and must also set 'Secure'.")]
Copy link
Member

Choose a reason for hiding this comment

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

When you don't set EventName explicitly the generator uses the log method's name as the event name. So you if you prefer, just remove , EventName = "SameSiteNotSecure" and rename SameSiteCookieNotSecure(..) to SameSiteNotSecure.

public static partial void SameSiteCookieNotSecure(ILogger logger, string name);
}
}
}
Loading