dotnet / roslyn-analyzers

MIT License
1.58k stars 464 forks source link

Struct constructor with only optional parameters not used #7179

Open bjornhellander opened 7 months ago

bjornhellander commented 7 months ago

Version Used: Visual Studio 17.8.5

Consider the following program:

using System;

internal class Program
{
    private static void Main()
    {
        var s1 = new S1();
        Console.WriteLine(s1.X);

        var s2 = new S2();
        Console.WriteLine(s2.X);
    }
}

internal struct S1
{
    public S1()
    {
        X = 1;
    }

    public int X { get; }
}

internal struct S2
{
    public S2(int x = 1)
    {
        X = x;
    }

    public int X { get; }
}

The output from this will be

1
0

meaning that the parameterless constructor in S1 is used, but the constructor with optional parameters in S2 is not.

I am assuming now that this is intentional. Please correct me if I am wrong :-) I do however think that this is very unexpected to a lot of programmers. I assume that this behavior has existed more or less always, but I think it has gotten worse after allowing parameterless struct constructors, leaving constructors with optional parameters as an odd case.

Could we get a diagnostic (from compiler or analyzer) when defining a constructor with only optional parameters or when the compiler uses the implicit constructor instead? In places where the implicit constructor is wanted, we are better off using default(T) anyway.

RikkiGibson commented 7 months ago

This seems reasonable to me as a possible CA rule. Moving to roslyn-analyzers.

See also https://github.com/dotnet/roslyn-analyzers/blob/main/GuidelinesForNewRules.md.

I am uncertain whether it is most appropriate to report the diagnostic at the declaration site or the use site. I could imagine a struct constructor with many optional parameters, where usage in practice is to pass arguments for some of the parameters, and that it's fine to use default rather than call the constructor when no arguments are passed.

Perhaps a reasonable solution here is:

then report a warning at the creation site (by default).

Code fix could suggest to either:

bjornhellander commented 2 months ago

Thanks for the response! I managed to miss it somehow.

This sounds very reasonable to me. Triggering on the creation sounds like a better idea than on the constructor declaration. I don't see why it should not trigger if there is an initializer, though. The constructor might still do something that you would expect to happen if you don't have a complete grasp of when the compiler will create a default object instead of using the declared constructor. Could you elaborate on that, @RikkiGibson?