Roslyn: Warning on Unused Record Primary Constructor Parameter

[HaloFour]

I was recently playing with positional records and overriding the property declarations, e.g.:

public record Person(string FirstName, string LastName) {
    public string FirstName { get; set; }
    public string LastName { get; set; }
}

I was surprised to find that the constructor that is emitted in this case does not actually wire up the primary constructor parameters with the explicitly declared properties. I understand that this is because the implicit property declarations which are being overridden include an initializer which wires up the property.

In my opinion this would be an easy detail to miss and there is currently no indication that it's happening. I think it would be useful if the compiler would warn (or otherwise indicate) when the primary constructor parameter is unused.

[jnm2]

It would be nice if it also came with a fix:

public record Person(string FirstName, string LastName) {
    public string FirstName { get; set; } = FirstName;
    public string LastName { get; set; } = LastName;
}

[333fred]

@jaredpar if this is something we want to consider (and I think we should) it'll need to fit in the 16.8 time frame.

[jaredpar]

If we can fit it into 16.8 that would be nice but I also think it would be fine to come in in 16.9. There are a few functional bugs I would give priority over this at the moment.

[Youssef1313]

@jnm2 What should the code fix be for the following?

public record A
{
    public string P1 { get; }
}

record C(string P1) : A
{
}

[jnm2]

@Youssef1313 I'm still getting acquainted with records, but I don't think there's a way for C to initialize P1 unless an init or set accessor is added or a constructor is added to A. The fix for this would be drastic enough that I wouldn't really expect there to be a fix offered for this case.

Basically at this point in the example above, A has already created a problem even in isolation from C. There's probably already a warning on A that P1 is never set, right?

[Youssef1313]

There's probably already a warning on A that P1 is never set, right?

That's interesting @jnm2. Currently, there is no warning on P1. I think there should be, because it can never have any other value than null. This warning will need a separate issue as it's different than the constructor parameter case for this issue.

But assuming the declaration in record A was public string P1 { get; set; }, there shouldn't be a warning on A that P1 is never set. But still the same issue, the positional parameter P1 for record C is unused and it seems there is no way for it to be?

[jnm2]

@Youssef1313 In that case I think the necessary change would be:

 public record A
 {
     public string P1 { get; set; }
 }


-record C(string P1) : A
+record C : A
 {
+    public C(string P1)
+    {
+        this.P1 = P1;
+    }
 }

This is still in the territory of "The fix for this would be drastic enough that I wouldn't really expect there to be a fix offered for this case" for me personally, as a Roslyn user.

[jnm2]

However, I would also see a ton of value in a general refactoring from a primary constructor to a regular constructor. I can see wanting to do this even when there are no current code issues.

(When records get validators, explicitly declared constructors might be less good.)

[RikkiGibson]

The compiler team has discussed this and we currently believe that what should happen is the IDE analyzers should mark the parameter as unused by graying it out.

record Rec(int X)
{
    public int X { get; init; }

    public Rec(int X, int Y) : this(X) { } // IDE0060: Remove unused parameter 'Y'
}

Here the bug is that unused parameters are not detected in the primary constructor.

[RikkiGibson]

We may need to check whether it is possible to use the dataflow analysis APIs on initializers in a type to decide if a primary constructor parameter was used (even implicitly).

[Youssef1313]

As mentioned in https://github.com/dotnet/roslyn/pull/47675#discussion_r490479963, it's decided that this be done in the IDE-side as an analyzer warning.

[jcouv]

Moving issue to Area-IDE. Tagging @CyrusNajmabadi as FYI

[jcouv]

This was fixed by https://github.com/dotnet/roslyn/pull/48895
The compiler now warns if the record positional parameters are unread.