[API Proposal]: Consider making all implementations of ILogger.BeginScope<TState>(TState) return a non-null value

[paulomorgado]

Background and motivation

Even though the contract for ILogger.BeginScope<TState>(TState) states the the return value can be null (IDisposable?), actually returning null should be avoided.

The correct usage of the API should be something like:

var loggingScope = logger.BeginScope(...);
try
{
    // ...
}
finally
{
    loggingScope?.Dispose();
}

API Proposal

using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Grpc.JsonTranscoding.IntegrationTests.Infrastructure;

internal class ForwardingLoggerProvider : ILoggerProvider
{
    private readonly LogMessage _logAction;

    public ForwardingLoggerProvider(LogMessage logAction)
    {
        _logAction = logAction;
    }

    public ILogger CreateLogger(string categoryName)
    {
        return new ForwardingLogger(categoryName, _logAction);
    }

    public void Dispose()
    {
    }

    internal class ForwardingLogger : ILogger
    {
        private readonly string _categoryName;
        private readonly LogMessage _logAction;

        public ForwardingLogger(string categoryName, LogMessage logAction)
        {
            _categoryName = categoryName;
            _logAction = logAction;
        }

        public IDisposable? BeginScope<TState>(TState state) where TState : notnull
        {
            return NullScope.Instance!;
        }

        public bool IsEnabled(LogLevel logLevel)
        {
            return true;
        }

        public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter)
        {
            _logAction(logLevel, _categoryName, eventId, formatter(state, exception), exception);
        }

        private sealed class NullScope : IDisposable
        {
            public static NullScope Instance { get; } = new NullScope();

            private NullScope()
            {
            }

            /// <inheritdoc />
            public void Dispose()
            {
            }
        }
    }
}

API Usage

using var loggingScope = logger.BeginScope(...);

// ...

Alternative Designs

Provide an empty IDisposable implementation in the BCL to avoid reimplementation and external dependencies.

Risks

No response

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-logging See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Background and motivation Even though the contract for `ILogger.BeginScope(TState)` states the the return value can be `null` (`IDisposable?`), actually returning `null` should be avoided. The correct usage of the API should be something like: ```csharp var loggingScope = logger.BeginScope(...); try { // ... } finally { loggingScope?.Dispose(); } ``` ### API Proposal ```csharp using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Grpc.JsonTranscoding.IntegrationTests.Infrastructure; internal class ForwardingLoggerProvider : ILoggerProvider { private readonly LogMessage _logAction; public ForwardingLoggerProvider(LogMessage logAction) { _logAction = logAction; } public ILogger CreateLogger(string categoryName) { return new ForwardingLogger(categoryName, _logAction); } public void Dispose() { } internal class ForwardingLogger : ILogger { private readonly string _categoryName; private readonly LogMessage _logAction; public ForwardingLogger(string categoryName, LogMessage logAction) { _categoryName = categoryName; _logAction = logAction; } public IDisposable? BeginScope(TState state) where TState : notnull { return NullScope.Instance!; } public bool IsEnabled(LogLevel logLevel) { return true; } public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) { _logAction(logLevel, _categoryName, eventId, formatter(state, exception), exception); } private sealed class NullScope : IDisposable { public static NullScope Instance { get; } = new NullScope(); private NullScope() { } /// public void Dispose() { } } } } ``` ### API Usage ```csharp using var loggingScope = logger.BeginScope(...); // ... ``` ### Alternative Designs Provide an empty `IDisposable` implementation in the BCL to avoid reimplementation and external dependencies. ### Risks _No response_

Author:	paulomorgado
Assignees:	-
Labels:	`api-suggestion`, `area-Extensions-Logging`
Milestone:	-

[pinkfloydx33]

I don't understand why this would be necessary. If used in a using the compiler already inserts a null check on the disposable before calling Dispose. SharpLab

[skyoxZ]

Some info there: https://github.com/dotnet/runtime/issues/63867

[qsdfplkj]

Not all loggers do support scopes. This might give the false impression there is a scope so now we have to check if the returned object is a nullscope v.s. checking for null?