@svick did you read the comment #91678 (comment)?

I agree with @ericstj params will be a problematic to support. The semantic of params is treated as multiple variable parameters. Imagine something like:

[LoggerMessage(Message = "{arg1} ... {arg2}", Level = LogLevel.Info)]
static partial void Log1(ILogger logger, params object?[] args);

The logger message expects to have two parameters. at runtime we should get exactly number of parameters match the message, otherwise should fail. It is better to have the source generator produce a diagnostic for this case and fail it instead of trying to work it out and produce runtime failure.

Whatever we do here, we want to ensure the same be applied to https://github.com/dotnet/extensions/tree/main/src/Generators/Microsoft.Gen.Logging.

@tarekgh

The semantic of params is treated as multiple variable parameters.

I don't understand why that behavior would be desirable here.

If you want to have two values, you can add two parameters to the message and two regular parameters to the logging method, there is no need to use params.

If you want to have variable number of values, then with this PR, you can add one parameter to the message, a params parameter to the method and it will format the actual values (because it's internally treated like a regular array).

For example, with the code from the original issue:

internal static partial class LogMessages
{
    [LoggerMessage(Message = "Message: {message}, {args}", Level = LogLevel.Information)]
    internal static partial void MSG(this ILogger logger, string? message, params object?[] args);
}

If you call:

LogMessages.MSG(logger, "message", 1, 2, 3);

Then with this PR and default console logger, you will get:

info: LogMessages[624365154]
      Message: message, 1, 2, 3

What am I missing?

The whole point of params keyword is in C# allows a method to accept a variable number of arguments. This is useful when the number of arguments is not known beforehand. This is different than just passing arrays.

For example, if I write something like the following, would you accept it as valid or invalid?

    [LoggerMessage(Message = "Message: {message}, Index:{args0}  Values:{args1}", Level = LogLevel.Information)]
    internal static partial void MSG(this ILogger logger, string? message, params object?[] args);

Then calling:

LogMessages.MSG(logger, "message", 1, 2, 3);

Logically (following C# concepts), this should be valid. This means if we support it, at run time you have validate and ensure args has at least one argument and format it in the message and then format the rest as one array. That will be very confusing. Additionally, we'll lose matching the parameter names in the message to the passed parameters to the method.

Look at https://learn.microsoft.com/en-us/dotnet/api/system.console.writeline?view=net-10.0#system-console-writeline(system-string-system-object()) as a good example when params is useful. I am not seeing any value it is adding to the logger SG other than not having the caller to manually passing array like LogMessages.MSG(logger, "message", [1, 2, 3]);

In another word, in logging source gen, the number of message arguments is always known beforehand which defeat the params purpose.

@tarekgh

Logically (following C# concepts), this should be valid.

I see the message template to be on the same level as the body of the logging method. There, args is just an array.

So your example would be invalid, and I think it doesn't violate C# concepts.

Or, to be more technical, params means that the number of arguments is variable, but the number of parameters is always the same. And the message template is referencing parameters, not arguments. (This differentiates it from Console.WriteLine or the ILogger.Log methods, whose format strings do reference arguments.)

The C# spec even says:

Except for allowing a variable number of arguments in an invocation, a parameter array is precisely equivalent to a value parameter of the same type.

So I think making it precisely equivalent to a regular array parameter here is fine.

I am not seeing any value it is adding to the logger SG other than not having the caller to manually passing array like LogMessages.MSG(logger, "message", [1, 2, 3]);

That's fair. Personally, I think the value of params over using a collection expression is low, but not zero. So, if we can resolve the conceptual issues above, I don't see a reason not to allow it.

@svick

You are missing important point here. Either we claim we support params or not. As soon as we claim we support it, then you have to address all scenarios supported by params. You cannot say we are partially supported to help users passing individual values of the array instead of passing array. The next ask will come is why we don't fully support it allowing variable number of message arguments? I hope you are seeing where I am heading with that. It is simple and clearer we say we don't support params with the LoggerMessage because it is not adding much value and set the right expectation to the users. Otherwise, with current proposal it is very confusing from my point of view.

@copilot does the support of ref readonly and scoped will work with logger messages accept more than 7 parameters? If so, please add a test for that to validate this scenario.

@copilot please address the test failure (when targeting NET481).

C:\h\w\9CF3089C\w\9958089D\e>xunit.console.exe Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests (app domain = on [no shadow copy], method display = ClassAndMethod, method display options = None)
  Discovered:  Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests (found 112 of 113 test cases)
  Starting:    Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests (parallel test collections = on [2 threads], stop on fail = off)
    Microsoft.Extensions.Logging.Generators.Tests.LoggerMessageGeneratorParserTests.InvalidScopedValueParameter [FAIL]
      Assert.Single() Failure: The collection was empty
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs(988,0): at Microsoft.Extensions.Logging.Generators.Tests.LoggerMessageGeneratorParserTests.<InvalidScopedValueParameter>d__57.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
    Microsoft.Extensions.Logging.Generators.Tests.LoggerMessageGeneratorParserTests.InvalidRefLikeTypeParameter [FAIL]
      Assert.Single() Failure: The collection was empty
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs(1004,0): at Microsoft.Extensions.Logging.Generators.Tests.LoggerMessageGeneratorParserTests.<InvalidRefLikeTypeParameter>d__58.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
  Finished:    Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests

@copilot please address the test failure (when targeting NET481).

...

Fixed in 7ef8ebf. The root cause: scoped ReadOnlySpan<int> (a ScopedValue parameter) is only semantically valid when Roslyn can confirm the type is a ref struct. On net481, if ReadOnlySpan<int> isn't recognized as a ref struct from the available netstandard/System.Runtime references, Roslyn marks it as an error type — hitting the IErrorTypeSymbol early-exit in the parser before SYSLIB1029 can fire.

Both InvalidScopedValueParameter and InvalidRefLikeTypeParameter now define their own ref struct MyRefStruct { } directly in the test source, guaranteeing IsRefLikeType == true regardless of available BCL assemblies.

Now that I think about it, the scoped modifier is only useful for ref struct types, which are not supported, so there is no reason to support it.

@copilot Remove the support for scoped added in this PR and instead produce the SYSLIB1028 diagnostic. Update that diagnostic and any mention of it appropriately.

cc: @tarekgh

Now that I think about it, the scoped modifier is only useful for ref struct types, which are not supported, so there is no reason to support it.

@copilot Remove the support for scoped added ...

Done in 05f16d8. Removed scoped support entirely — IsScoped field, emitter emission, and Roslyn4.0.cs propagation are all gone. scoped parameters now emit SYSLIB1028 (same as params), and the diagnostic message/title has been updated to "unsupported params or scoped parameter modifier". All "ScopedRef*OK" parser tests are converted to error tests, the TestBaseline_TestWithScopedRefParam_Success emitter test and its baseline file are removed, and UseScopedRefParameter is removed from ParameterTestExtensions.cs.