ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/aspnetcore
File: src/Components/Components/src/ComponentsMetrics.cs (C#)

Code before:
    public void FailParametersSync(Exception ex, long startTimestamp, string? componentType)
    {
        var duration = Stopwatch.GetElapsedTime(startTimestamp);
        var tags = new TagList
        {
            { "aspnetcore.components.type", componentType ?? "unknown" },
            { "error.type", ex.GetType().FullName ?? "unknown" }
        };
        _parametersDuration.Record(duration.TotalSeconds, tags);
    }

    public async Task CaptureBatchDuration(Task task, long startTimestamp, int diffLength)
    {
        var tags = new TagList();

        try
        {
            await task;
        }
        catch (Exception ex)
        {
            tags.Add("error.type", ex.GetType().FullName ?? "unknown");
        }
        var duration = Stopwatch.GetElapsedTime(startTimestamp);
        _batchDuration.Record(duration.TotalSeconds, tags);
        _batchSize.Record(diffLength);
    }

    public void FailBatchSync(Exception ex, long startTimestamp)
    {
        var duration = Stopwatch.GetElapsedTime(startTimestamp);
        var tags = new TagList
            {
                { "error.type", ex.GetType().FullName ?? "unknown" }
            };
        _batchDuration.Record(duration.TotalSeconds, tags);
    }

    public void Dispose()
    {
        _meter.Dispose();
        _lifeCycleMeter.Dispose();
    }
}


Review comment (performance):
```suggestion
        _batchSize.Record(diffLength, tags);
```

Code after:
    public void FailParametersSync(Exception ex, long startTimestamp, string? componentType)
    {
        var duration = Stopwatch.GetElapsedTime(startTimestamp);
        var tags = new TagList
        {
            { "aspnetcore.components.type", componentType ?? "unknown" },
            { "error.type", ex.GetType().FullName ?? "unknown" }
        };
        _parametersDuration.Record(duration.TotalSeconds, tags);
    }

    public async Task CaptureBatchDuration(Task task, long startTimestamp, int diffLength)
    {
        var tags = new TagList();

        try
        {
            await task;
        }
        catch (Exception ex)
        {
            tags.Add("error.type", ex.GetType().FullName ?? "unknown");
        }
        var duration = Stopwatch.GetElapsedTime(startTimestamp);
        _batchDuration.Record(duration.TotalSeconds, tags);
        _batchSize.Record(diffLength, tags);
    }

    public void FailBatchSync(Exception ex, long startTimestamp)
    {
        var duration = Stopwatch.GetElapsedTime(startTimestamp);
        var tags = new TagList
            {
                { "error.type", ex.GetType().FullName ?? "unknown" }
            };
        _batchDuration.Record(duration.TotalSeconds, tags);
    }

    public void Dispose()
    {
        _meter.Dispose();
        _lifeCycleMeter.Dispose();
    }
}
Repository: dotnet/aspnetcore
File: src/Mvc/Mvc.Razor.RuntimeCompilation/src/DependencyInjection/MvcRazorRuntimeCompilationOptionsSetup.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.Hosting;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation;

internal sealed class MvcRazorRuntimeCompilationOptionsSetup : IConfigureOptions<MvcRazorRuntimeCompilationOptions>
{
    private readonly IWebHostEnvironment _hostingEnvironment;

    public MvcRazorRuntimeCompilationOptionsSetup(IWebHostEnvironment hostingEnvironment)
    {
        _hostingEnvironment = hostingEnvironment ?? throw new ArgumentNullException(nameof(hostingEnvironment));
    }

    public void Configure(MvcRazorRuntimeCompilationOptions options)
    {
        ArgumentNullException.ThrowIfNull(options);

        options.FileProviders.Add(_hostingEnvironment.ContentRootFileProvider);
    }
}


Review comment (suggestion):
Are you thinking we'll suppress the warning and only remove these references once the types are removed?

Asking because I was looking at deprecating IActionContextAccessor and it's much easier if we don't remove our own use of the API during the obsoletion. It does mean we have to pay that cost later when we actually remove these types.

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.Hosting;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation;

#pragma warning disable ASPDEPR003 // Type or member is obsolete
internal sealed class MvcRazorRuntimeCompilationOptionsSetup : IConfigureOptions<MvcRazorRuntimeCompilationOptions>
#pragma warning restore ASPDEPR003 // Type or member is obsolete
{
    private readonly IWebHostEnvironment _hostingEnvironment;

    public MvcRazorRuntimeCompilationOptionsSetup(IWebHostEnvironment hostingEnvironment)
    {
        _hostingEnvironment = hostingEnvironment ?? throw new ArgumentNullException(nameof(hostingEnvironment));
    }

#pragma warning disable ASPDEPR003 // Type or member is obsolete
    public void Configure(MvcRazorRuntimeCompilationOptions options)
#pragma warning restore ASPDEPR003 // Type or member is obsolete
    {
        ArgumentNullException.ThrowIfNull(options);

        options.FileProviders.Add(_hostingEnvironment.ContentRootFileProvider);
    }
}
Repository: dotnet/aspnetcore
File: src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/test/NotFailingQuickGrid.cs (C#)

Code before:

    /// <summary>
    /// Task that completes when OnAfterRenderAsync has finished executing.
    /// This allows tests to wait deterministically for the race condition to occur.
    /// </summary>
    public Task OnAfterRenderCompleted => _onAfterRenderCompleted.Task;

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        try
        {
            if (firstRender)
            {
                await base.OnAfterRenderAsync(firstRender);
                _onAfterRenderCompleted.TrySetResult();
                _completionSignaled = true;
                return;
            }
        }
        finally
        {
            if (firstRender && _completionSignaled)
            {
                _onAfterRenderCompleted.TrySetResult();
                _completionSignaled = true;
            }
        }
    }
}


Review comment (suggestion):
The condition `firstRender && _completionSignaled` in the finally block will never be true because `_completionSignaled` is only set to true within the try block when `firstRender` is true. This creates unreachable code that will never execute.
```suggestion

```

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;
using System.Collections.Generic;
using System.Text;
using Microsoft.JSInterop;

namespace Microsoft.AspNetCore.Components.QuickGrid.Tests;
/// <summary>
/// A QuickGrid implementation that uses the same implementation of the basic QuickGrid with the additions of the OnAfterRenderCompleted task.
/// </summary>
/// /// <typeparam name="TGridItem">The type of data represented by each row in the grid.</typeparam>
internal class NotFailingGrid<TGridItem> : QuickGrid<TGridItem>
{
    [Inject] private IJSRuntime JS { get; set; } = default!;

    private readonly TaskCompletionSource _onAfterRenderCompleted = new();

    /// <summary>
    /// Task that completes when OnAfterRenderAsync has finished executing.
    /// This allows tests to wait deterministically for the race condition to occur.
    /// </summary>
    public Task OnAfterRenderCompleted => _onAfterRenderCompleted.Task;

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        try
        {
            if (firstRender)
            {
                await base.OnAfterRenderAsync(firstRender);
            }
        }
        finally
        {
            if (firstRender)
            {
                _onAfterRenderCompleted.TrySetResult();
            }
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/test/NotFailingQuickGrid.cs (C#)

Code before:

    private readonly TaskCompletionSource _onAfterRenderCompleted = new();

    private bool _completionSignaled;

    /// <summary>
    /// Task that completes when OnAfterRenderAsync has finished executing.
    /// This allows tests to wait deterministically for the race condition to occur.
    /// </summary>
    public Task OnAfterRenderCompleted => _onAfterRenderCompleted.Task;

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        try
        {
            if (firstRender)
            {
                await base.OnAfterRenderAsync(firstRender);
                _onAfterRenderCompleted.TrySetResult();
                _completionSignaled = true;
                return;
            }
        }
        finally
        {
            if (firstRender && _completionSignaled)
            {
                _onAfterRenderCompleted.TrySetResult();
                _completionSignaled = true;
            }
        }
    }
}


Review comment (suggestion):
The logic should be `if (firstRender && !_completionSignaled)` to ensure the task completion is signaled if an exception occurs before reaching line 35.
```suggestion
            if (firstRender && !_completionSignaled)
```

Code after:

    private readonly TaskCompletionSource _onAfterRenderCompleted = new();

    /// <summary>
    /// Task that completes when OnAfterRenderAsync has finished executing.
    /// This allows tests to wait deterministically for the race condition to occur.
    /// </summary>
    public Task OnAfterRenderCompleted => _onAfterRenderCompleted.Task;

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        try
        {
            if (firstRender)
            {
                await base.OnAfterRenderAsync(firstRender);
            }
        }
        finally
        {
            if (firstRender)
            {
                _onAfterRenderCompleted.TrySetResult();
            }
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/test/NotFailingQuickGrid.cs (C#)

Code before:
/// <summary>
/// A QuickGrid implementation that uses the same implementation of the basic QuickGrid with the additions of the OnAfterRenderCompleted task.
/// </summary>
/// /// <typeparam name="TGridItem">The type of data represented by each row in the grid.</typeparam>
internal class NotFailingGrid<TGridItem> : QuickGrid<TGridItem>
{
    [Inject] private IJSRuntime JS { get; set; } = default!;

    private readonly TaskCompletionSource _onAfterRenderCompleted = new();

    private bool _completionSignaled;

    /// <summary>
    /// Task that completes when OnAfterRenderAsync has finished executing.
    /// This allows tests to wait deterministically for the race condition to occur.
    /// </summary>
    public Task OnAfterRenderCompleted => _onAfterRenderCompleted.Task;

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        try
        {
            if (firstRender)
            {
                await base.OnAfterRenderAsync(firstRender);
                if (!_completionSignaled)
                {
                    _completionSignaled = true;
                    _onAfterRenderCompleted.TrySetResult();
                }
                ;
                return;
            }
        }
        finally
        {
            if (firstRender && !_completionSignaled)
            {
                _onAfterRenderCompleted.TrySetResult();
                _completionSignaled = true;
            }
        }
    }
}


Review comment (bug):
This is a duplicate of finally action. If we throw in `OnAfterRenderAsync` -> we go to finally. If we don't throw -> we execute this condition and by default flow, we should go to finally. `return` is redundant.

Remove this one and use `finally` only. We have the same construction in `FailingQuickGrid`.

Code after:
/// <summary>
/// A QuickGrid implementation that uses the same implementation of the basic QuickGrid with the additions of the OnAfterRenderCompleted task.
/// </summary>
/// /// <typeparam name="TGridItem">The type of data represented by each row in the grid.</typeparam>
internal class NotFailingGrid<TGridItem> : QuickGrid<TGridItem>
{
    [Inject] private IJSRuntime JS { get; set; } = default!;

    private readonly TaskCompletionSource _onAfterRenderCompleted = new();

    /// <summary>
    /// Task that completes when OnAfterRenderAsync has finished executing.
    /// This allows tests to wait deterministically for the race condition to occur.
    /// </summary>
    public Task OnAfterRenderCompleted => _onAfterRenderCompleted.Task;

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        try
        {
            if (firstRender)
            {
                await base.OnAfterRenderAsync(firstRender);
            }
        }
        finally
        {
            if (firstRender)
            {
                _onAfterRenderCompleted.TrySetResult();
            }
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/test/FailingQuickGrid.cs (C#)

Code before:
    public bool IsWasDisposedFalse()
    {
        var field = typeof(QuickGrid<TGridItem>).GetField("_wasDisposed", BindingFlags.NonPublic | BindingFlags.Instance);
        return field?.GetValue(this) is false;
    }

    /// <summary>
    /// Override OnAfterRenderAsync to simulate the race condition by NOT checking _disposeBool.
    /// This exactly replicates the code path that existed before the race condition fix.
    /// </summary>
    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        try
        {
            if (firstRender)
            {
                if (JS != null)
                {
                    // Import the JS module (this will trigger our TestJsRuntime's import logic)
                    var jsModule = await JS.InvokeAsync<IJSObjectReference>("import",
                        "./_content/Microsoft.AspNetCore.Components.QuickGrid/QuickGrid.razor.js");

                    await jsModule.InvokeAsync<IJSObjectReference>("init", new object());

                    // Signal completion only after the init call has completed, and only once
                    if (!_completionSignaled)
                    {
                        _completionSignaled = true;
                        _onAfterRenderCompleted.TrySetResult();
                    }
                    return;
                }
            }
        }
        finally
        {
            // Only signal completion if we haven't already done it and this is the first render
            if (firstRender && !_completionSignaled)
            {
                _completionSignaled = true;
                _onAfterRenderCompleted.TrySetResult();
            }
        }
    }
}


Review comment (refactor):
Remove this condition, the one from `finally` block is enough.

Code after:
    {
        var field = typeof(QuickGrid<TGridItem>).GetField("_wasDisposed", BindingFlags.NonPublic | BindingFlags.Instance);
        return field?.GetValue(this) is false;
    }

    /// <summary>
    /// Override OnAfterRenderAsync to simulate the race condition by NOT checking _disposeBool.
    /// This exactly replicates the code path that existed before the race condition fix.
    /// </summary>
    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        try
        {
            if (firstRender)
            {
                if (JS != null)
                {
                    // Import the JS module (this will trigger our TestJsRuntime's import logic)
                    var jsModule = await JS.InvokeAsync<IJSObjectReference>("import",
                        "./_content/Microsoft.AspNetCore.Components.QuickGrid/QuickGrid.razor.js");
                    await jsModule.InvokeAsync<IJSObjectReference>("init", new object());
                }
            }
        }
        finally
        {
            if (firstRender)
            {
                _onAfterRenderCompleted.TrySetResult();
            }
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/ConverterTests/JsonConverterReadTests.cs (C#)

Code before:
    [Fact]
    public void MapKeyBool()
    {
        var json = @"{
  ""mapKeybool"": {
    ""true"": ""value1"",
    ""false"": ""value2""
  }
}";

        AssertReadJson<HelloRequest>(json);
    }

    [Fact]
    public void MapKeyBool_CaseInsensitive()
    {
        var json = @"{
  ""mapKeybool"": {
    ""TRUE"": ""value1"",
    ""FALSE"": ""value2""
  }
}";

        AssertReadJson<HelloRequest>(json, serializeOld: false);
    }

    [Fact]
    public void MapKeyInt()
    {
        var json = @"{
  ""mapKeyint"": {
    ""-1"": ""value1"",
    ""0"": ""value3""
  }
}";

        AssertReadJson<HelloRequest>(json);
    }

    [Fact]
    public void OneOf_Success()
    {
        var json = @"{
  ""oneofName1"": ""test""
}";

        AssertReadJson<HelloRequest>(json);
    }

    [Fact]
    public void OneOf_CaseInsensitive_Success()

Review comment (security):
```suggestion
        AssertReadJson<HelloRequest>(json, serializeOld: false, settings: new GrpcJsonSettings { FieldNamesCaseInsensitive = true });
```

Code after:
    [Fact]
    public void MapKeyBool()
    {
        var json = @"{
  ""mapKeybool"": {
    ""true"": ""value1"",
    ""false"": ""value2""
  }
}";

        AssertReadJson<HelloRequest>(json);
    }

    [Fact]
    public void MapKeyBool_CaseInsensitive()
    {
        var json = @"{
  ""mapKeybool"": {
    ""TRUE"": ""value1"",
    ""FALSE"": ""value2""
  }
}";

        // Note: JSON property names here are keys in a dictionary, not fields. So FieldNamesCaseInsensitive doesn't apply.
        // The new serializer supports converting true/false to boolean keys while ignoring case.
        AssertReadJson<HelloRequest>(json, serializeOld: false);
    }

    [Fact]
    public void MapKeyInt()
    {
        var json = @"{
  ""mapKeyint"": {
    ""-1"": ""value1"",
    ""0"": ""value3""
  }
}";

        AssertReadJson<HelloRequest>(json);
    }

    [Fact]
    public void OneOf_Success()
    {
        var json = @"{
  ""oneofName1"": ""test""
}";

        AssertReadJson<HelloRequest>(json);
    }
Repository: dotnet/aspnetcore
File: src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/EnumNameHelpers.cs (C#)

Code before:
        {
            return value;
        }

        while (valueIndex < value.Length && value[valueIndex] == '_')
        {
            valueIndex++;
        }

        return valueIndex == value.Length ? value : value.Substring(valueIndex);
    }

    private static string GetEnumValueName(string enumName, string valueName)
    {
        var result = TryRemovePrefix(enumName, valueName);
        return char.IsDigit(result[0]) ? $"_{result}" : result;
    }

    private sealed class EnumMapping
    {
        public required Dictionary<object, NameMapping> WriteMapping { get; init; }
        public required Dictionary<string, string> StripEnumPrefixMapping { get; init; }
    }

    private sealed class NameMapping
    {
        public required string OriginalName { get; init; }
        public required string StripEnumPrefixName { get; init; }
    }
}


Review comment (suggestion):
This code will throw an IndexOutOfRangeException if result is an empty string. The method should check if result.Length > 0 before accessing result[0].
```suggestion
        var result = TryRemovePrefix(enumName, valueName);
        if (result.Length == 0)
        {
            return result;
        }
```

Code after:

            if (char.ToLowerInvariant(value[valueIndex]) != normalizedPrefix[prefixIndex])
            {
                return value;
            }

            prefixIndex++;
            valueIndex++;
        }

        if (prefixIndex < normalizedPrefix.Length)
        {
            return value;
        }

        while (valueIndex < value.Length && value[valueIndex] == '_')
        {
            valueIndex++;
        }

        return valueIndex == value.Length ? value : value.Substring(valueIndex);
    }

    private static string GetEnumValueName(string enumName, string valueName)
    {
        var result = TryRemovePrefix(enumName, valueName);

        // Prefix name starting with a digit with an underscore to ensure it is a valid identifier.
        return result.Length > 0 && char.IsDigit(result[0])
            ? $"_{result}"
            : result;
    }

    private sealed class EnumMapping
    {
        public required Dictionary<object, NameMapping> WriteMapping { get; init; }
        public required Dictionary<string, string> RemoveEnumPrefixMapping { get; init; }
    }

    private sealed class NameMapping
    {
        public required object Value { get; init; }
        public required string OriginalName { get; init; }
        public required string RemoveEnumPrefixName { get; init; }
    }
}
Repository: dotnet/aspnetcore
File: src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/EnumNameHelpers.cs (C#)

Code before:
            prefixIndex++;
            valueIndex++;
        }

        if (prefixIndex < normalizedPrefix.Length)
        {
            return value;
        }

        while (valueIndex < value.Length && value[valueIndex] == '_')
        {
            valueIndex++;
        }

        return valueIndex == value.Length ? value : value.Substring(valueIndex);
    }

    private static string GetEnumValueName(string enumName, string valueName)
    {
        var result = TryRemovePrefix(enumName, valueName);
        return char.IsDigit(result[0]) ? $"_{result}" : result;
    }

    private sealed class EnumMapping
    {
        public required Dictionary<object, NameMapping> WriteMapping { get; init; }
        public required Dictionary<string, string> StripEnumPrefixMapping { get; init; }
    }

    private sealed class NameMapping
    {
        public required string OriginalName { get; init; }
        public required string StripEnumPrefixName { get; init; }
    }
}


Review comment (suggestion):
This line can return an empty string when valueIndex equals value.Length, which will cause the IndexOutOfRangeException in GetEnumValueName. Consider returning the original value when the stripped result would be empty.
```suggestion
        return valueIndex == value.Length ? value : (valueIndex < value.Length ? value.Substring(valueIndex) : value);
```

Code after:
            if (value[valueIndex] == '_')
            {
                valueIndex++;
                continue;
            }

            if (char.ToLowerInvariant(value[valueIndex]) != normalizedPrefix[prefixIndex])
            {
                return value;
            }

            prefixIndex++;
            valueIndex++;
        }

        if (prefixIndex < normalizedPrefix.Length)
        {
            return value;
        }

        while (valueIndex < value.Length && value[valueIndex] == '_')
        {
            valueIndex++;
        }

        return valueIndex == value.Length ? value : value.Substring(valueIndex);
    }

    private static string GetEnumValueName(string enumName, string valueName)
    {
        var result = TryRemovePrefix(enumName, valueName);

        // Prefix name starting with a digit with an underscore to ensure it is a valid identifier.
        return result.Length > 0 && char.IsDigit(result[0])
            ? $"_{result}"
            : result;
    }

    private sealed class EnumMapping
    {
        public required Dictionary<object, NameMapping> WriteMapping { get; init; }
        public required Dictionary<string, string> RemoveEnumPrefixMapping { get; init; }
    }

    private sealed class NameMapping
    {
        public required object Value { get; init; }
        public required string OriginalName { get; init; }
        public required string RemoveEnumPrefixName { get; init; }
    }
}