ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/aspnetcore
File: src/Framework/AspNetCoreAnalyzers/test/WebApplicationBuilder/PublicPartialProgramClassTest.cs (C#)

Code before:

var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();
""";

        // Assert
        await VerifyCS.VerifyCodeFixAsync(source, source);
    }

    [Fact]
    public async Task RemovesDeclarationIfItIsFound()
    {
        // Arrange
        var source = """
using Microsoft.AspNetCore.Builder;

var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();

{|#0:public partial class Program { }|}
""";

        var diagnostic = new DiagnosticResult(DiagnosticDescriptors.PublicPartialProgramClassNotRequired)
                .WithLocation(0);

        var fixedSource = """
using Microsoft.AspNetCore.Builder;

var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();

""";

        // Assert
        await VerifyCS.VerifyCodeFixAsync(source, [diagnostic], fixedSource);
    }

    [Fact]
    public async Task DoesNotGeneratesSource_IfProgramDeclaresExplicitInternalAccess()
    {
        var source = """
using Microsoft.AspNetCore.Builder;

Review comment (suggestion):
Two test case suggestions in case they would appear as "not empty":

1. This, but with a semicolon instead of {} (`Program;`) if that actually usually compiles;
1. This, but with a single comment between the braces (because I've done this with the comment explaining why the declaration is there: [example](https://github.com/martincostello/dotnet-minimal-api-integration-testing/blob/95da381be986d3820048846faf646219687ae2ee/src/TodoApp/Program.cs#L74)).

Code after:

var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();
""";

        // Assert
        await VerifyCS.VerifyCodeFixAsync(source, source);
    }

    [Theory]
    [InlineData("public partial class Program { }")]
    [InlineData("public partial class Program { /* This is just for tests */ }")]
    [InlineData("public partial class Program { \n // This is just for tests \n }")]
    [InlineData("public partial class Program;")]
    public async Task RemovesDeclarationIfItIsFound(string declarationStyle)
    {
        // Arrange
        var source = $$"""
using Microsoft.AspNetCore.Builder;

var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();

{|#0:{{declarationStyle}}|}
""";

        var diagnostic = new DiagnosticResult(DiagnosticDescriptors.PublicPartialProgramClassNotRequired)
                .WithLocation(0);

        var fixedSource = """
using Microsoft.AspNetCore.Builder;

var app = WebApplication.Create();

app.MapGet("/", () => "Hello, World!");

app.Run();


""";

        // Assert
        await VerifyCS.VerifyCodeFixAsync(source, [diagnostic], fixedSource);
    }
Repository: dotnet/aspnetcore
File: src/Middleware/Diagnostics/src/DiagnosticsTelemetry.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Http;

namespace Microsoft.AspNetCore.Diagnostics;

internal static class DiagnosticsTelemetry
{
    public static void AddMetricsTags(HttpContext context, Exception ex)
    {
        if (context.Features.Get<IHttpMetricsTagsFeature>() is { } tagsFeature)
        {
            // Multiple exception middleware could be registered that have already added the tag.
            // We don't want to add a duplicate tag here because that breaks some metrics systems.
            tagsFeature.TryAddTag("error.type", ex.GetType().FullName);
        }
    }
}


Review comment (suggestion):
I hadn't considered we were still recording error metrics when `SuppressLoggingCallback` returns true. Would it make sense not to? I imagine people who want to suppress the logs and EventSource events would also want to suppress the "error.type" tag for the given request otel metrics for the exact same reason.

If we wanted to allow the most fine-grained control we could rely on a mutable ExceptionHandlerSuppressLoggingContext. I imagine we might want to reconsider the name of `SuppressLoggingCallback` though. Maybe `SuppressDiagnosticsCallback`? That might be a better name already since this appears to already suppress the EventSource event.

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Diagnostics;

internal static class DiagnosticsTelemetry
{
    public static void ReportUnhandledException(ILogger logger, HttpContext context, Exception ex)
    {
        logger.UnhandledException(ex);

        if (context.Features.Get<IHttpMetricsTagsFeature>() is { } tagsFeature)
        {
            // Multiple exception middleware could be registered that have already added the tag.
            // We don't want to add a duplicate tag here because that breaks some metrics systems.
            tagsFeature.TryAddTag("error.type", ex.GetType().FullName);
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Components/Endpoints/src/DependencyInjection/HttpNavigationManager.cs (C#)

Code before:
using Microsoft.AspNetCore.Components.Routing;

namespace Microsoft.AspNetCore.Components.Endpoints;

internal sealed class HttpNavigationManager : NavigationManager, IHostEnvironmentNavigationManager
{
    private IEndpointHtmlRenderer? _renderer;

    void IHostEnvironmentNavigationManager.Initialize(string baseUri, string uri) => Initialize(baseUri, uri);

    void IHostEnvironmentNavigationManager.Initialize(string baseUri, string uri, IEndpointHtmlRenderer? renderer)
    {
        Initialize(baseUri, uri);
        _renderer = renderer;
    }

    protected override void NavigateToCore(string uri, NavigationOptions options)
    {
        var absoluteUriString = ToAbsoluteUri(uri).AbsoluteUri;
        throw new NavigationException(absoluteUriString);
    }

    protected override void NotFoundCore()
    {
        _renderer?.SetNotFoundResponse();
    }
}


Review comment (suggestion):
Could we avoid the additional `IEndpointHtmlRenderer` and subscribe to the `NavigationManager.NotFound` event from the `EndpointHtmlRenderer` to set a 404 status code when a `NotFound` event is triggered?

(Essentially, reverse the relationship, instead of NavigationManager -> Renderer, Renderer -> NavigationManager) [Here](https://github.com/dotnet/aspnetcore/blob/12d57ddde6bd2f47757364252596cfa89df2ef60/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.cs#L79) subscribe to the event to trigger whatever is needed when a 404 happens

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Components.Routing;

namespace Microsoft.AspNetCore.Components.Endpoints;

internal sealed class HttpNavigationManager : NavigationManager, IHostEnvironmentNavigationManager
{
    void IHostEnvironmentNavigationManager.Initialize(string baseUri, string uri) => Initialize(baseUri, uri);

    protected override void NavigateToCore(string uri, NavigationOptions options)
    {
        var absoluteUriString = ToAbsoluteUri(uri).AbsoluteUri;
        throw new NavigationException(absoluteUriString);
    }
}
Repository: dotnet/aspnetcore
File: src/Shared/HttpSys/NativeInterop/SocketAddress.cs (C#)

Code before:
        }
        else if (Family == AddressFamily.InterNetwork)
        {
            return GetIPv4Address();
        }
        else
        {
            return null;
        }
    }

    private IPAddress GetIpv6Address()
    {
        Contract.Assert(_size >= IPv6AddressSize);
        var bytes = new byte[NumberOfIPv6Labels * 2];
        Array.Copy(_buffer, 8, bytes, 0, NumberOfIPv6Labels * 2);
        return new IPAddress(bytes); // TODO: Does scope id matter?
    }

    private IPAddress GetIPv4Address()
    {
        Contract.Assert(_size >= IPv4AddressSize);
        return new IPAddress(new byte[] { _buffer[4], _buffer[5], _buffer[6], _buffer[7] });
    }

    internal static unsafe SocketAddress? CopyOutAddress(IntPtr address)
    {
        AddressFamily family = (AddressFamily)(*((ushort*)address));
        int? addressSize = family switch
        {
            AddressFamily.InterNetwork => IPv4AddressSize,
            AddressFamily.InterNetworkV6 => IPv6AddressSize,
            _ => null
        };

        if (!addressSize.HasValue)
        {
            return null;
        }

        var sockAddress = new SocketAddress(family, addressSize.Value);
        new ReadOnlySpan<byte>((byte*)address, addressSize.Value).Slice(sizeof(ushort)).CopyTo(sockAddress._buffer.AsSpan(sizeof(ushort)));
        return sockAddress;
    }
}


Review comment (suggestion):
Can the signature be changed to pass in the buffer size (ideally as Span)? This method won't need to be unsafe once you do that. 

Passing buffers without size is against secure coding best practices. 

Code after:
        }

        internal override IPAddress? GetIPAddress()
        {
            // address is network byte order
            return new IPAddress(_sockaddr.sin_addr.S_un.S_addr);
        }
    }

    private sealed class SocketAddressIPv6 : SocketAddress
    {
        private readonly SOCKADDR_IN6 _sockaddr;

        internal SocketAddressIPv6(in SOCKADDR_IN6 sockaddr)
        {
            _sockaddr = sockaddr;
        }

        internal override int GetPort()
        {
            // sin6_port is network byte order
            return IPAddress.NetworkToHostOrder((short)_sockaddr.sin6_port);
        }

        internal override IPAddress? GetIPAddress()
        {
            // address is network byte order
            // when CsWin32 gets support for inline arrays, remove 'AsReadOnlySpan' call below.
            // https://github.com/microsoft/CsWin32/issues/1086
            return new IPAddress(_sockaddr.sin6_addr.u.Byte.AsReadOnlySpan()); // TODO: Does scope id matter?
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Servers/HttpSys/src/NativeInterop/RequestQueue.cs (C#)

Code before:

        var result = PInvoke.HttpSetRequestQueueProperty(Handle,
            HTTP_SERVER_PROPERTY.HttpServer503VerbosityProperty,
            &verbosity, (uint)Marshal.SizeOf<long>());

        if (result != 0)
        {
            throw new HttpSysException((int)result);
        }
    }

    public void Dispose()
    {
        if (_disposed)
        {
            return;
        }

        _disposed = true;

        PInvoke.HttpCloseRequestQueue(Handle);

        BoundHandle.Dispose();
        Handle.Dispose();
    }

    private void CheckDisposed()
    {
        ObjectDisposedException.ThrowIf(_disposed, this);
    }

    private static partial class Log
    {
        [LoggerMessage(LoggerEventIds.AttachedToQueue, LogLevel.Information, "Attached to an existing request queue '{RequestQueueName}', some options do not apply.", EventName = "AttachedToQueue")]
        public static partial void AttachedToQueue(ILogger logger, string? requestQueueName);
    }
}


Review comment (bug):
Seems like a reasonable change but unrelated to this PR. Drive-by bug fix or is it related somehow?

Code after:
    // The listener must be active for this to work.
    internal unsafe void SetRejectionVerbosity(Http503VerbosityLevel verbosity)
    {
        Debug.Assert(Created);
        CheckDisposed();

        var result = PInvoke.HttpSetRequestQueueProperty(Handle,
            HTTP_SERVER_PROPERTY.HttpServer503VerbosityProperty,
            &verbosity, (uint)Marshal.SizeOf<long>());

        if (result != 0)
        {
            throw new HttpSysException((int)result);
        }
    }

    public void Dispose()
    {
        if (_disposed)
        {
            return;
        }

        _disposed = true;

        PInvoke.HttpCloseRequestQueue(Handle);

        BoundHandle.Dispose();
        Handle.Dispose();
    }

    private void CheckDisposed()
    {
        ObjectDisposedException.ThrowIf(_disposed, this);
    }

    private static partial class Log
    {
        [LoggerMessage(LoggerEventIds.AttachedToQueue, LogLevel.Information, "Attached to an existing request queue '{RequestQueueName}', some options do not apply.", EventName = "AttachedToQueue")]
        public static partial void AttachedToQueue(ILogger logger, string? requestQueueName);
    }
}
Repository: dotnet/aspnetcore
File: src/Middleware/HttpLogging/src/ResponseBufferingStream.cs (C#)

Code before:
        {
            var responseBody = GetStringInternal();
            _logger.ResponseBody(responseBody);
        }
    }

    public void LogResponseBody(HttpLoggingInterceptorContext logContext)
    {
        if (_logBody)
        {
            logContext.AddParameter("ResponseBody", GetStringInternal());
        }
    }

    private string GetStringInternal()
    {
        var result = _bodyBeforeClose ?? GetString(_encoding!);
        // Reset the value after its consumption to preserve GetString(encoding) behavior
        _bodyBeforeClose = null;
        return result;
    }

    public override void Close()
    {
        if (_logBody)
        {
            // Subsequent middleware can close the response stream after writing its body
            // Preserving the body for the final GetStringInternal() call.
            _bodyBeforeClose = GetString(_encoding!);
        }

        base.Close();
    }
}


Review comment (suggestion):
`Stream.Dispose()` calls `Close()` which could then re-create the string just to throw it away. Maybe we should set `_logBody` to false once one of the `LogResponseBody()` methods is called.

I think the request stream impl is fine, but we should double-check and maybe add a comment about the assumption.

Code after:
        if (_logBody)
        {
            var responseBody = GetStringInternal();
            _logger.ResponseBody(responseBody);
            _hasLogged = true;
        }
    }

    public void LogResponseBody(HttpLoggingInterceptorContext logContext)
    {
        if (_logBody)
        {
            logContext.AddParameter("ResponseBody", GetStringInternal());
            _hasLogged = true;
        }
    }

    private string GetStringInternal()
    {
        var result = _bodyBeforeClose ?? GetString(_encoding!);
        // Reset the value after its consumption to preserve GetString(encoding) behavior
        _bodyBeforeClose = null;
        return result;
    }

    public override void Close()
    {
        if (_logBody && !_hasLogged)
        {
            // Subsequent middleware can close the response stream after writing its body
            // Preserving the body for the final GetStringInternal() call.
            _bodyBeforeClose = GetString(_encoding!);
        }

        base.Close();
    }
}
Repository: dotnet/aspnetcore
File: src/Middleware/OutputCaching/src/Memory/MemoryOutputCacheStore.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Linq;
using Microsoft.Extensions.Caching.Memory;

namespace Microsoft.AspNetCore.OutputCaching.Memory;

internal sealed class MemoryOutputCacheStore : IOutputCacheStore
{
    private readonly MemoryCache _cache;
    private readonly Dictionary<string, HashSet<Tuple<string,string>>> _taggedEntries = new();
    private readonly object _tagsLock = new();

    internal MemoryOutputCacheStore(MemoryCache cache)
    {
        ArgumentNullException.ThrowIfNull(cache);

        _cache = cache;
    }

    // For testing
    internal Dictionary<string, HashSet<string>> TaggedEntries => _taggedEntries.ToDictionary(kvp => kvp.Key, kvp => kvp.Value.Select(t => t.Item1).ToHashSet());

    public ValueTask EvictByTagAsync(string tag, CancellationToken cancellationToken)
    {
        ArgumentNullException.ThrowIfNull(tag);

        lock (_tagsLock)
        {
            if (_taggedEntries.TryGetValue(tag, out var keys))
            {
                if (keys != null && keys.Count > 0)
                {
                    // If MemoryCache changed to run eviction callbacks inline in Remove, iterating over keys could throw
                    // To prevent allocating a copy of the keys we check if the eviction callback ran,
                    // and if it did we restart the loop.

Review comment (suggestion):
```suggestion
    private readonly Dictionary<string, HashSet<Tuple<string, string>>> _taggedEntries = [];
```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Linq;
using Microsoft.Extensions.Caching.Memory;

namespace Microsoft.AspNetCore.OutputCaching.Memory;

internal sealed class MemoryOutputCacheStore : IOutputCacheStore
{
    private readonly MemoryCache _cache;
    private readonly Dictionary<string, HashSet<TaggedEntry>> _taggedEntries = [];
    private readonly object _tagsLock = new();

    internal MemoryOutputCacheStore(MemoryCache cache)
    {
        ArgumentNullException.ThrowIfNull(cache);

        _cache = cache;
    }

    // For testing
    internal Dictionary<string, HashSet<string>> TaggedEntries => _taggedEntries.ToDictionary(kvp => kvp.Key, kvp => kvp.Value.Select(t => t.Key).ToHashSet());

    public ValueTask EvictByTagAsync(string tag, CancellationToken cancellationToken)
    {
        ArgumentNullException.ThrowIfNull(tag);

        lock (_tagsLock)
        {
            if (_taggedEntries.TryGetValue(tag, out var keys))
            {
                if (keys is { Count: > 0 })
                {
                    // If MemoryCache changed to run eviction callbacks inline in Remove, iterating over keys could throw
                    // To prevent allocating a copy of the keys we check if the eviction callback ran,
                    // and if it did we restart the loop.
Repository: dotnet/aspnetcore
File: src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs (C#)

Code before:
                    var headers = (IDictionary<string, string>)carrier!;
                    headers.TryGetValue(fieldName, out fieldValue);
                },
                operationName,
                kind,
                tags,
                links,
                loggingEnabled);
        }
        else
        {
            activity = activitySource.CreateActivity(operationName, kind, parentId: null, tags: tags, links: links);
        }

        if (activity is not null)
        {
            activity.DisplayName = $"{_fullHubName}/{methodName}";
            activity.Start();
        }

        return activity;
    }

    private static void SetActivityError(Activity? activity, Exception ex)
    {
        activity?.SetTag("error.type", ex.GetType().FullName);
        activity?.SetStatus(ActivityStatusCode.Error);
    }
}

Review comment (suggestion):
Nit: restore the final newline.
```suggestion
}

```

Code after:
                operationName,
                kind,
                tags,
                links,
                loggingEnabled);
        }
        else
        {
            activity = activitySource.CreateActivity(operationName, kind, parentId: null, tags: tags, links: links);
        }

        if (activity is not null)
        {
            activity.DisplayName = $"{_fullHubName}/{methodName}";
            activity.Start();
        }

        return activity;
    }

    private static void SetActivityError(Activity? activity, Exception ex)
    {
        activity?.SetTag("error.type", ex.GetType().FullName);
        activity?.SetStatus(ActivityStatusCode.Error);
    }
}
Repository: dotnet/aspnetcore
File: src/Components/Shared/src/ResourceCollectionProvider.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using Microsoft.AspNetCore.Components.Web;
using Microsoft.JSInterop;

namespace Microsoft.AspNetCore.Components;

using static Microsoft.AspNetCore.Internal.LinkerFlags;

internal class ResourceCollectionProvider
{
    private string? _url;

    [SupplyParameterFromPersistentComponentState]
    public string? ResourceCollectionUrl
    {
        get => _url;
        set => _url = value;
    }
    private ResourceAssetCollection? _resourceCollection;
    private readonly IJSRuntime _jsRuntime;
    public ResourceCollectionProvider(IJSRuntime jsRuntime)
    {
        _jsRuntime = jsRuntime;
    }

    [MemberNotNull(nameof(_url))]
    internal void SetResourceCollectionUrl(string url)
    {
        if (_url != null)
        {
            throw new InvalidOperationException("The resource collection URL has already been set.");
        }
        _url = url;
    }

    internal async Task<ResourceAssetCollection> GetResourceCollection()
    {
        _resourceCollection = _resourceCollection ??= await LoadResourceCollection();
        return _resourceCollection;
    }

    internal void SetResourceCollection(ResourceAssetCollection resourceCollection)
    {

Review comment (suggestion):
```suggestion
    public string? ResourceCollectionUrl { get; set; }
```


Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using Microsoft.AspNetCore.Components.Web;
using Microsoft.JSInterop;

namespace Microsoft.AspNetCore.Components;

using static Microsoft.AspNetCore.Internal.LinkerFlags;

internal class ResourceCollectionProvider
{
    private string? _url;

    [SupplyParameterFromPersistentComponentState]
    public string? ResourceCollectionUrl
    {
        get => _url;
        set
        {
            if (_url != null)
            {
                throw new InvalidOperationException("The resource collection URL has already been set.");
            }
            _url = value;
        }
    }

    private ResourceAssetCollection? _resourceCollection;
    private readonly IJSRuntime _jsRuntime;
    public ResourceCollectionProvider(IJSRuntime jsRuntime)
    {
        _jsRuntime = jsRuntime;
    }

    internal async Task<ResourceAssetCollection> GetResourceCollection()
    {
        _resourceCollection = _resourceCollection ??= await LoadResourceCollection();
        return _resourceCollection;
    }

    internal void SetResourceCollection(ResourceAssetCollection resourceCollection)
    {
        _resourceCollection = resourceCollection;
    }