Support C# 13 params collections (IEnumerable<T>, List<T>)#478
Conversation
…ructs return null
nblumhardt
left a comment
There was a problem hiding this comment.
Looks good! Just one comment on diagnostics.
| { | ||
| return Activator.CreateInstance(paramType); | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
In the catch block here, it might be worth pointing out:
which describes the cases in which Activator.CreateInstance is unlikely to succeed, and also the reflection-driven API we'd need to use in order to extend capability to them.
A Serilog.Debugging.SelfLog.WriteLine(...) call that includes the exception message and as much information as possible about the parameter being constructed would help users figure out why these might fail.
There was a problem hiding this comment.
Hello,
I updated the PR, here is what the log produces:
Unable to create an implicit instance of the params collection type 'Serilog.Settings.Configuration.Tests.BrokenCollection`1[System.String]' for parameter 'list' on method 'DummyBrokenCollection'.
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
---> System.InvalidOperationException: I am broken by design!
at Serilog.Settings.Configuration.Tests.BrokenCollection`1..ctor() in C:\Development\serilog-settings-configuration\test\Serilog.Settings.Configuration.Tests\DummyLoggerConfigurationExtensions.cs:line 72
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean wrapExceptions)
--- End of inner exception stack trace ---
at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean wrapExceptions)
at Serilog.Settings.Configuration.ConfigurationReader.GetImplicitValueForNotSpecifiedKey(ParameterInfo parameter, MethodInfo methodToInvoke) in C:\Development\serilog-settings-configuration\src\Serilog.Settings.Configuration\Settings\Configuration\ConfigurationReader.cs:line 464
Let me know if this works for you.
nblumhardt
left a comment
There was a problem hiding this comment.
Thanks for the follow-up, just a couple of minor notes.
| var result = reader.GetImplicitValueForNotSpecifiedKey(param, method); | ||
|
|
||
| Assert.Null(result); | ||
| Assert.True(logs.Count > 0, "SelfLog count was 0. The catch block was never entered!"); |
There was a problem hiding this comment.
The cause of the assertion failing could be unrelated to the catch block. Better to just assert the condition and drop the diagnostic. Assert.NotEmpty(logs) should do the job.
| { | ||
| // Activator.CreateInstance is unlikely to succeed for collections lacking a parameterless constructor, | ||
| // or those relying on the [CollectionBuilder] attribute for initialization. | ||
| SelfLog.WriteLine($"Unable to create an implicit instance of the params collection type '{paramType}' for parameter '{parameter.Name}' on method '{methodToInvoke.Name}'.\n{ex}"); |
There was a problem hiding this comment.
SelfLog message styles are a bit inconsistent through the codebase, but the ones in Serilog generally use format strings, backticks or no delimiters around params, and colon before trailing exception:
SelfLog.WriteLine($"Unable to create an implicit instance of the params collection type `{0}` for parameter `{1}` on method `{2}`: {3}", paramType, parameter.Name, methodToInvoke.Name, ex);
SelfLog.WriteLine has no overload for accepting FormattableString or a builder, so the $ syntax will cause formatting to occur prematurely here.
There was a problem hiding this comment.
Thanks for the comments.
The signature or Selflog.WriteLine:
public static void WriteLine(string format, object? arg0 = null, object? arg1 = null, object? arg2 = null)
A) So I either collapse everything into that first format argument with $ or string.Format:
SelfLog.WriteLine($"Unable to create an implicit instance of the params collection type `{paramType}` for parameter `{parameter.Name}` on method `{methodToInvoke.Name}`: {ex}");
B) Or I extend the WriteLine function with a 4th optional arg3 = null parameter and do no $:
SelfLog.WriteLine("Unable to create an implicit instance of the params collection type `{0}` for parameter `{1}` on method `{2}`: {3}", paramType, parameter.Name, methodToInvoke.Name, ex);
In this serilog-settings-configuration repository option A is used everywhere.
In the serilog repository option B is used everywhere.
How do you want to move forward?
Let me know if I'm not seeing something here.
There was a problem hiding this comment.
Ah I see, thanks. Option A might be the way to go 👍
Might be worth us looking at adding explicit support to SelfLog for formattable strings, sometime down the track.
There was a problem hiding this comment.
Just out of curiosity about maintaining all these repos, when you say:
Might be worth us looking at adding explicit support to SelfLog for formattable strings, sometime down the track.
Do you track this anywhere, so when there is a major version update you noted down this should be implemented?
Hypotheticals:
Btw this would be a performance optimization, right? We only create the string when there is an active listener.
How do you do these migrations on breaking change you just completely remove this signature and just give a new one, if they want to upgrade they would have to change their Selflogs, or do you do a 2 phase migration, first declare it obsolete, provide a newer solution and next you remove that old signature?
There was a problem hiding this comment.
We only create the string when there is an active listener.
That's correct.
How do you do these migrations on breaking change
At this point, so many downstream sinks use those APIs that deprecation/removal isn't really a possibility. Anything we added would likely just be additive.
There probably isn't a huge pressing need to improve this, especially for this package where most code runs only once at start-up, and we only write those messages on an unlikely error path. Just mentally bookmarking it :-)
|
Thanks! 👍 |
Resolves #476
Following up on the discussion in the issue, this PR implements support for C# 13 generic
paramscollections purely for the ergonomic and aesthetic benefits, while intentionally bypassingReadOnlySpanto avoid reflection complexity.Changes Included:
params IEnumerable<T>and safely satisfies the method contract by returning an empty array.params List<T>orHashSet<T>and usesActivator.CreateInstanceto generate an empty collection.ref structEvasion: Added a framework-agnostic check usingIsByRefLikeAttributeto catch types likeReadOnlySpan<T>. This ensures we don't attempt to pass aref structinto theMethodInfo.InvokeReflection pipeline, allowing it to safely fall back tonullacross all supported target frameworks.IEnumerable,List, andReadOnlySpan.