Codecov Report

Merging #947 (1220efb) into master (66f7cab) will decrease coverage by 0.14%.
The diff coverage is 98.42%.

❗ Current head 1220efb differs from pull request most recent head 9a3f721. Consider uploading reports for the commit 9a3f721 to get more accurate results

@@            Coverage Diff             @@
##           master     #947      +/-   ##
==========================================
- Coverage   92.63%   92.49%   -0.14%     
==========================================
  Files         113      113              
  Lines        3421     3464      +43     
  Branches     1056      571     -485     
==========================================
+ Hits         3169     3204      +35     
- Misses        189      195       +6     
- Partials       63       65       +2     

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

I'm therefore leaning towards it being simpler to just embed these into the project instead of taking an additional dependency. A downside might be eventual divergence with time, but I don't expect these to change much and it would also enable some flexibility for tailoring.

If you want to take the time to copy them from CommunityToolkit and implement them yourself, I won't disagree. That said, a) I'm far too lazy to redo what someone else has already done, and b) I think this goes against principles of working with libraries in the first place.

Namely if every library copies these methods, then now these methods are implemented in each library, increasing dll size and JIT size since they all have to be JIT'd separately, especially for generic methods which need to be genericized separately for each implementation.

Using a single library (CommunityToolkit for cross-TFM, BCL for net8+ which has many of these now implemented) means that this code is shared across all consumers.

For example, I wonder if the Guard methods shouldn't be changed to return the valid argument, for some future-readiness.

I used to think that too, when I started with the Throw library (from amantinband) and switched to CommunityToolkit. However, I've found as I've used Guard.Xxx more, I now prefer the non-return version because it encourages better code readability, specifically, that contractual obligations (not-null, not-null/empty/whitespace, numeric range) are specified and checked at the start of the method.

Do we really need a throw helper for InvalidOperationException? I think you had doubts about the value of this too?

The advantage of this specifically is two-fold:

1. the calling code reduces JIT code as the throw creates a fairly large footprint

2. calling code is no-longer allowed to be inlined (inlining is turned off for any method that includes a throw anywhere in the method). This may not be as big of a deal for a throw in the middle of an iterator method, but definitely has an impact on methods that simply return a value. Obviously all of the outer methods that MoreLinq uses are more likely to be inlined, switching to Guard.Xxx, which means many of them may turn into direct calls to the Core method if the JIT can prove that the values are not-null, etc.

   Whether this matters in the cases of the various ThrowHelper calls is less certain, but definitely TryAsCollectionLike() and AggregateRight() will benefit from it. For me, I've grown used to it, so I now actually prefer ThrowHelper. instead of throw new, but I'll concede that to be a usage/personal preference.

For targets where the helpers are unavailable, we can copy over the 1-2 line implementations from .NET and light them up conditionally. These will only need to be maintained until those targets have been dropped, although I don't expect there'll any maintenance to do since they can't be expected to change much at all with time.

This is not a bad suggestion. I'll have to think about how to implement in a way that a) isn't too complicated and b) doesn't require tfm based evaluations at the call site.

What are your thoughts about using global usings to hide the complexity. Could do something like:

<ItemGroup Condition="$(TargetFramework) != 'net8.0'">
  <Using Include="MoreLinq.Exceptions.ArgumentNullException" Static="true"/>
</ItemGroup>
<ItemGroup Condition="$(TargetFramework) == 'net8.0'">
  <Using Include="System.ArgumentNullException" Static="true"/>
</ItemGroup>

public static IEnumerable<TSource> Operator<TSource>(this IEnumerable<TSource> source)
{
  ThrowIfNull(source);
  // ...
}

What are your thoughts about using global usings to hide the complexity.

@viceroypenguin I was thinking something along the very same lines! However, instead of statically importing methods, I'd like to try and maintain the type name qualification. Taking the example of one of the Pad overloads, ideally the code looks as you'd write it in .NET 8:

public static IEnumerable<TSource> Pad<TSource>(this IEnumerable<TSource> source, int width,
                                                Func<int, TSource> paddingSelector)
{
    ArgumentNullException.ThrowIfNull(source);
    ArgumentNullException.ThrowIfNull(paddingSelector);
    ArgumentOutOfRangeException.ThrowIfLessThan(width, 0);

    return PadImpl(source, width, default, paddingSelector);
}

The targets that won't have those methods on types could have masking definitions in the MoreLinq namespace (which could be made to take precedence), like so:

#if EXCEPTION_HELPERS

namespace MoreLinq
{
    using System.Diagnostics.CodeAnalysis;
    using System.Runtime.CompilerServices;

    static class ArgumentNullException
    {
        public static void ThrowIfNull([NotNull] object? argument,
                                       [CallerArgumentExpression(nameof(argument))]
                                       string? paramName = null)
        {
            if (argument is null)
                Throw(paramName);
        }

        [DoesNotReturn]
        static void Throw(string? paramName) =>
            throw new System.ArgumentNullException(paramName);
    }
}

#endif

A conditional symbol like EXCEPTION_HELPERS could then be defined in the project file based on the target framework. This is just a rough idea from the head and a little bit of experimentation may be needed to find the sweet spot.

This does mean however that you can't just write throw new ArgumentNullException() anymore, but that may not be a bad thing!

I would also limit this just to the main library project. I don't think we need to extend this approach, for example, to the test project.

What would be the cherry on top is to add an integration test that exercises the inlining (although as I said previously, tiered compilation means you don't always get the expected optimisations). I have some ideas for how this could be pulled off.

@viceroypenguin As a pulse check, do you plan to follow-up here based on latest discussion? I'm just asking to triage open issues and PRs.

Yes, but will probably be a while. That's a bit of more work than I have time for right now.

@atifaziz Question on which path you want to go here.

Currently, the following code does not work:

#if !NET6_0_OR_GREATER
global using ArgumentNullException = MoreLinq.Exceptions.ArgumentNullException;
#endif

The reason is that each namespace MoreLinq { using System; } creates a tighter binding for ArgumentNullException to System than to the global alias.

I don't have this problem in SuperLinq, because I migrated to implicit usings and file-scoped namespaces. Since these are the recommended patterns, I'd recommend we do a separate PR to do these migrations.

Alternatively, each file can include

#if !NET6_0_OR_GREATER
global using ArgumentNullException = MoreLinq.Exceptions.ArgumentNullException;
#endif

at the top of the file, below the using System;. I think this path is a huge detriment to the usability of each file.

Your call, though.