RCS1242 Reports false positives.

[James-Jackson-South]

Product and Version Used: Visual Studio Enterprise 16.5.0. Roslynator 2.9.0 Steps to Reproduce:

Given the following class.

internal sealed class PixelRowDelegateProcessor<TPixel, TDelegate> : ImageProcessor<TPixel>
    where TPixel : unmanaged, IPixel<TPixel>
    where TDelegate : struct, IPixelRowDelegate
{
    private readonly TDelegate rowDelegate;

    /// <summary>
    /// The <see cref="PixelConversionModifiers"/> to apply during the pixel conversions.
    /// </summary>
    private readonly PixelConversionModifiers modifiers;

    /// <summary>
    /// Initializes a new instance of the <see cref="PixelRowDelegateProcessor{TPixel,TDelegate}"/> class.
    /// </summary>
    /// <param name="rowDelegate">The row processor to use to process each pixel row</param>
    /// <param name="configuration">The configuration which allows altering default behaviour or extending the library.</param>
    /// <param name="modifiers">The <see cref="PixelConversionModifiers"/> to apply during the pixel conversions.</param>
    /// <param name="source">The source <see cref="Image{TPixel}"/> for the current processor instance.</param>
    /// <param name="sourceRectangle">The source area to process for the current processor instance.</param>
    public PixelRowDelegateProcessor(
        in TDelegate rowDelegate,
        Configuration configuration,
        PixelConversionModifiers modifiers,
        Image<TPixel> source,
        Rectangle sourceRectangle)
        : base(configuration, source, sourceRectangle)
    {
        this.rowDelegate = rowDelegate;
        this.modifiers = modifiers;
    }

    /// <inheritdoc/>
    protected override void OnFrameApply(ImageFrame<TPixel> source)
    {
        // Content removed for brevity.
    }
}

Actual Behavior: Roslynator will flag the following parameter as breaking RCS1242

in TDelegate rowDelegate

This, I imagine is due to the lack of readonly type constraint to enforce a rule, rather convention would be applied to manage the correct behavior. However I expected the analyzer to inspect calls to the constructor to determine validation (This could be a misunderstanding of how analyzers work on my part).

Expected Behavior: No error would be raised due to analysis of caller.

[josefpihrt]

I created simple example:

SharpLab

public class C<T> where T : struct
{
    private readonly T x;

    public C(T x)
    {
        this.x = x;
    }
}

public class C2<T> where T : struct
{
    private readonly T x;

    public C2(in T x)
    {
        this.x = x;
    }
}

If you look at IL instructions there is an extra instructions when parameter is passed by read-only reference:

IL_000a: ldobj !T

This is unnecessary copy of the parameter value and the purpose of the analyzer is to remove this unnecessary copy.

[antoinebj]

Hi,

This is unnecessary copy of the parameter value and the purpose of the analyzer is to remove this unnecessary copy.

That copy apparently doesn't always occur though.

The compiler seems to be able to determine whether the defensive copy needs to be created or not depending on what is done with the input parameter.

Also, as can be seen by comparing the Create3() methods, the additional ldobj instruction is not the entire picture. On the caller side, there is a ldloca instruction which only loads the address vs ldloc with loads the entire struct on the stack.
If the reference is just going to be passed around from method to method down the stack until one method does something with it, I assume that you save on many copies to the stack, where even the ldobj would be more than offset.
But even 1 level deep, if you have a large struct (which admittedly is usually not good practice, but you may not have control if it comes from a 3rd party library), being able to pass by read-only reference when you know that your implementation is only going to read from allows for best performance.
And finally, there's the case where you're subscribing to a callback from a 3rd party library that has defined the parameters with the in modifier despite the types being non-read-only structs. There is then no choice but to conform to that signature. That's actually the scenario where I got a warning on that rule, and those structs happened to also be pretty big (Win32 stuff).

I don't know how all that should be taken into account by the implementation of RCS1242, but I think that at the very least, the documentation should give much more information on why it's a code smell, and when it should be safe to suppress the warning.