SA1402 does not take topLevelType setting into account for records

[appel1]

1.2.0-beta.507

In our stylecop json we have:

"maintainabilityRules": {
   "topLevelTypes": [ "class" ]
}

But still it warns about files containing more than one record. It's fine if the default for topLevelTypes has changed to include records but not being able to configure it at all is inconvenient.

[bjornhellander]

The behaviour you are experiencing is what was decided in #3542. Record, record class and record struct were simply ignore before that, if I am not mistaken.

Can you describe why you think it would be good to be able to configure the behaviour for class and record declarations separately?

[appel1]

We primarily use records (and rarely record struct) for small dtos with very little logic if any. So we treat them as other small things like delegates, small interfaces, small structs and the like and put the together with the class that produces or consumes them or many of them in a separate file if that makes more sense. It just clutters things up to put these one liners into separate files.

If record and record struct were separate and the default for topLevelTypes was changed to [ "class", "record" ] instead you could get both the current behavior and allow us to configure it in the way we prefer. Right now our only sensible option is to turn off SA1402, or suppress it all over the place.

[appel1]

It doesn't really make sense to me to allow structs in the same file as other non-class declarations and a class but not record.

Struct:

    public readonly struct Something : IEquatable<Something>
    {
        public Something(int a, int b, int c)
        {
            A = a;
            B = b;
            C = c;
        }

        public int A { get; }

        public int B { get; }

        public int C { get; }

        public override bool Equals(object? obj)
        {
            return obj is Something something && Equals(something);
        }

        public bool Equals(Something other)
        {
            return A == other.A &&
                   B == other.B &&
                   C == other.C;
        }

        public override int GetHashCode()
        {
            int hashCode = 793064651;
            hashCode = hashCode * -1521134295 + A.GetHashCode();
            hashCode = hashCode * -1521134295 + B.GetHashCode();
            hashCode = hashCode * -1521134295 + C.GetHashCode();
            return hashCode;
        }

        public static bool operator ==(Something left, Something right)
        {
            return left.Equals(right);
        }

        public static bool operator !=(Something left, Something right)
        {
            return !(left == right);
        }
    }

Record:

public readonly record Something(int A, int B, int C);

[sharwell]

For now, I'm comfortable with the current approach. Occasionally there are short records like the one shown above, but in practice they often end up with one or more members added which make them more like small types.

A DiagnosticSuppressor could be created in your solution if you wanted to programmatically suppress warnings reported for specific cases that appear in your projects.

[appel1]

By that logic struct should also be handled as class since they always have members. The default should also be changed to include both class and struct.

I'm fine with that but would like a setting to exclude struct, record and record struct.

If it is impossible to add new fields to topLevelTypes perhaps a new excludeFromTopLevelTypes can be added that is empty by default and where you can add struct, record and recordstruct.

[sharwell]

The default should also be changed to include both class and struct.

I would have absolutely agreed with this if we created this rule. However, it was grandfathered in from StyleCop Classic so we kept the default behavior from prior work to ease migration.

[appel1]

A way to configure this would be nice either way.

Until then we'll have to turn it off because fixing the violations would lead to an unwieldy amount of one liner files.

[sharwell]

You could put a #pragma warning disable SA1402 at the top of the (presumably) small number of files with multiple declarations.

[appel1]

@sharwell We opted to disable SA1402 and create our own that only requires class and struct to be in separate files instead of hundreds of #pragmas. If you change your mind in the future we can just revert that change.

[lennartb-]

Yeah, more configuration options would have been great. We have many command handler classes which include the commands as records with 1- 3 members (i.e. one-liners), adding a suppression to each file is not nice. I guess we'll disable SA1402 for these particular folders in .editorconfig.

[danadesrosiers]

I think the change made to SA1402 in #3542 was sensible, but it should only apply to records with a body. Positional records with no body, like the example given by @appel1 above, should not be subject to this rule. In many cases, it's silly to put one line of code in a separate file.

EDIT: As of C#12, classes can also use primary constructors. They're a little different, but they are also allowed to have no body. I would expect these to be exempt from SA1402 as well.