Detect unused record parameters #48895

jcouv · 2020-10-24T00:10:25Z

Fixes #48584
FYI @Youssef1313 since relates to a previous PR

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs

RikkiGibson · 2020-10-26T21:38:36Z

@jcouv we just fixed a build break in master. You may want to merge master into your PR branch to get a reasonable result in CI. #Closed

AlekseyTs · 2020-10-27T15:35:02Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6579,4 +6579,10 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_FunctionPointersCannotBeCalledWithNamedArguments" xml:space="preserve">
    <value>A function pointer cannot be called with named arguments.</value>
  </data>
+  <data name="WRN_UnusedRecordParameter" xml:space="preserve">
+    <value>Parameter '{0}' is unread. Did you forget to use it to initialize the property with that name?</value>


Did you forget to use it to initialize the property with that name? [](start = 38, length = 67)

I am not sure the second sentence is necessary. #ByDesign

It's not necessary, but I think it's useful to guide users to the most likely solution. It helps for the primary scenario motivating this change.

In reply to: 512796721 [](ancestors = 512796721)

AlekseyTs · 2020-10-27T15:46:29Z

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs

@@ -560,6 +579,12 @@ private void NoteCaptured(Symbol variable)
                _usedVariables.Add(local);
            }

+            if (variable is ParameterSymbol parameter && CurrentSymbol is SynthesizedRecordConstructor)


variable is ParameterSymbol parameter && CurrentSymbol is SynthesizedRecordConstructor [](start = 16, length = 86)

Where do we check relationship between CurrentSymbol and parameter? #Closed

AlekseyTs · 2020-10-27T15:48:17Z

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs

@@ -1878,6 +1903,10 @@ public override BoundNode VisitParameter(BoundParameter node)
            {
                CheckAssigned(node.ParameterSymbol, node.Syntax);
            }
+            else if (CurrentSymbol is SynthesizedRecordConstructor)
+            {
+                NoteRead(node.ParameterSymbol);


node.ParameterSymbol [](start = 25, length = 20)

How do we know parameter and CurrentSymbol are related? #Closed

In lambda scenarios, CurrentSymbol is the lambda, not the primary constructor.
I'll add tests, but I believe that's the behavior we want (ie. reading the record parameter in a lambda doesn't count).

In reply to: 512809160 [](ancestors = 512809160)

but I believe that's the behavior we want (ie. reading the record parameter in a lambda doesn't count).

I don't know why would this be the behavior we want. Nevertheless, it would be good to clearly specify in the PR description what we are trying to achieve in this PR.

In reply to: 512902651 [](ancestors = 512902651,512809160)

Thinking more about this, maybe the usage inside the lambda should count as well. Made the change. Thanks

In reply to: 512907636 [](ancestors = 512907636,512902651,512809160)

AlekseyTs · 2020-10-27T15:54:21Z

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs

@@ -560,6 +579,12 @@ private void NoteCaptured(Symbol variable)
                _usedVariables.Add(local);
            }

+            if (variable is ParameterSymbol parameter && CurrentSymbol is SynthesizedRecordConstructor)


CurrentSymbol is SynthesizedRecordConstructor [](start = 57, length = 45)

It looks like a parameter can be used in a lambda. Is this condition going to work for this scenario? #Closed

Added tests for lambdas. Thanks for the scenario!

In reply to: 512814777 [](ancestors = 512814777)

AlekseyTs · 2020-10-27T15:58:52Z

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs

@@ -1878,6 +1903,10 @@ public override BoundNode VisitParameter(BoundParameter node)
            {
                CheckAssigned(node.ParameterSymbol, node.Syntax);
            }
+            else if (CurrentSymbol is SynthesizedRecordConstructor)
+            {
+                NoteRead(node.ParameterSymbol);


NoteRead [](start = 16, length = 8)

I would prefer us to not call a method that isn't meant to be called. It is fine to have it to do a little bit of extra work when we call it anyway, it is another thing to call it in a new place when we only interested in that additional extra work and the method does plenty of other things. If the goal is to reuse code, it can be extracted into a helper that can be called from here and from NoteRead #Closed

AlekseyTs · 2020-10-27T16:03:10Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+                    // (2,18): warning CS8907: Parameter 'O1' is unread. Did you forget to use it to initialize the property with that name?
+                    // record C1(object O1, object O2, object O3)
+                    Diagnostic(ErrorCode.WRN_UnusedRecordParameter, "O1").WithArguments("O1").WithLocation(2, 18),
+                    // (2,40): warning CS8907: Parameter 'O3' is unread. Did you forget to use it to initialize the property with that name?


warning CS8907: Parameter 'O3' is unread. Did you forget to use it to initialize the property with that name? [](start = 31, length = 109)

This warning feels unexpected to me. The parameter is used. #Closed

I should have updated the PR title, but didn't want to mess email threading. The main intent for this warning is that people are surprised when they write record C(int X) { public int X { get; init; } } and the property is not initialized. So I think it makes sense to focus on reads.

If one does record C(int X) : Base(X) { public int X { get; init; } };, I think a warning would also be desirable for that reason, but I can see that's debatable.

Started a thread with LDM to clarify expectations.

In reply to: 512822355 [](ancestors = 512822355)

Clarified with LDM. We'll warn on unread parameters. Didn't get feedback on phrasing of warning, but I'm open to proposal (let me know).

In reply to: 512891377 [](ancestors = 512891377,512822355)

AlekseyTs · 2020-10-27T16:04:15Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+{
+    public object O1 { get; init; }
+    public object O2 { get; init; } = M(O2);
+    public object O3 { get; init; } = M(O3 = null);


M(O3 = null) [](start = 38, length = 12)

It would be good to test scenarios when parameter is passed as ref, in, out. #Closed

AlekseyTs · 2020-10-27T16:07:26Z

src/Compilers/CSharp/Test/Syntax/Diagnostics/DiagnosticTest.cs

@@ -415,6 +416,7 @@ public void NullableWarnings()
                    ErrorCode.WRN_RecordNamedDisallowed,
                    ErrorCode.WRN_RecordEqualsWithoutGetHashCode,
                    ErrorCode.WRN_AnalyzerReferencesFramework,
+                    ErrorCode.WRN_UnusedRecordParameter,


ErrorCode.WRN_UnusedRecordParameter, [](start = 20, length = 36)

It looks like the warning is in the C#9 range. Why do we need to add it to this array? #ByDesign

This test catches any warnings in the range 8600 to 9000. The new warning is 8907...
I could adjust the range instead, but figured it is useful to remind us to think about whether the new diagnostics should be considered "nullability" diagnostics.

In reply to: 512825729 [](ancestors = 512825729)

AlekseyTs · 2020-10-27T16:09:45Z

Done with review pass (iteration 5) #Closed

jcouv · 2020-10-30T17:10:01Z

@dotnet/roslyn-compiler for review. Thanks

AlekseyTs

LGTM (iteration 7)

RikkiGibson · 2020-10-30T22:53:06Z

Taking a look

RikkiGibson

LGTM, thanks!

RikkiGibson · 2020-10-30T23:02:52Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+            comp.VerifyDiagnostics(
+                // (11,17): warning CS8907: Parameter 'P1' is unread. Did you forget to use it to initialize the property with that name?
+                // record B(object P1, object P2, object P3, object P4, object P5, object P6) : A
+                Diagnostic(ErrorCode.WRN_UnreadRecordParameter, "P1").WithArguments("P1").WithLocation(11, 17),


It occurs to me that it's not really possible for the user to assign to the base property here. A scenario where we change a base nominal property to a positional parameter in a derived type is going to be unpleasant until we add the ability to include a constructor body for a primary constructor.

jcouv added Area-Compilers Feature - Records Records labels Oct 24, 2020

jcouv self-assigned this Oct 24, 2020

Detect unused record parameters

0f631f6

jcouv force-pushed the unused-parameter branch from aef2643 to 0f631f6 Compare October 24, 2020 07:48

jcouv marked this pull request as ready for review October 24, 2020 22:28

jcouv requested a review from a team as a code owner October 24, 2020 22:28

jcouv added 2 commits October 26, 2020 09:54

Tweak message

acbc503

Missing files

09dd1bc

RikkiGibson self-assigned this Oct 26, 2020

RikkiGibson reviewed Oct 26, 2020

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs Outdated Show resolved Hide resolved

jcouv added 2 commits October 26, 2020 14:42

Merge remote-tracking branch 'dotnet/master' into unused-parameter

0170500

Add test case

40d75ab

jcouv requested a review from RikkiGibson October 26, 2020 23:41

AlekseyTs reviewed Oct 27, 2020

View reviewed changes

jcouv added 2 commits October 27, 2020 11:02

Renamed to Unread. Address feedback from Aleksey

f2a1ae8

Also count reads in lambdas

68d978f

This was referenced Oct 29, 2020

Compilation errors cause CI to fail #49020

Closed

Analyzer diagnostics cause CI to fail #49021

Closed

CSharpUpgradeProjectCodeFixProvider sometimes reports the wrong version #48548

Open

jcouv requested a review from AlekseyTs October 30, 2020 17:09

AlekseyTs approved these changes Oct 30, 2020

View reviewed changes

RikkiGibson approved these changes Oct 30, 2020

View reviewed changes

jcouv merged commit 0fdb64e into dotnet:master Oct 31, 2020

ghost added this to the Next milestone Oct 31, 2020

jcouv deleted the unused-parameter branch October 31, 2020 02:59

jcouv mentioned this pull request Nov 6, 2020

Warning on Unused Record Primary Constructor Parameter #47666

Closed

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect unused record parameters #48895

Detect unused record parameters #48895

jcouv commented Oct 24, 2020 •

edited

Loading

RikkiGibson commented Oct 26, 2020 •

edited by jcouv

Loading

AlekseyTs Oct 27, 2020 •

edited by jcouv

Loading

jcouv Oct 27, 2020

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited

Loading

jcouv Oct 27, 2020

AlekseyTs Oct 27, 2020

jcouv Oct 29, 2020

AlekseyTs Oct 27, 2020 •

edited

Loading

jcouv Oct 27, 2020

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited

Loading

jcouv Oct 27, 2020

jcouv Oct 29, 2020

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited by jcouv

Loading

jcouv Oct 27, 2020

AlekseyTs commented Oct 27, 2020 •

edited

Loading

jcouv commented Oct 30, 2020

AlekseyTs left a comment

RikkiGibson commented Oct 30, 2020

RikkiGibson left a comment

RikkiGibson Oct 30, 2020

Detect unused record parameters #48895

Detect unused record parameters #48895

Conversation

jcouv commented Oct 24, 2020 • edited Loading

RikkiGibson commented Oct 26, 2020 • edited by jcouv Loading

AlekseyTs Oct 27, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Oct 27, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Oct 27, 2020 • edited Loading

jcouv commented Oct 30, 2020

AlekseyTs left a comment

Choose a reason for hiding this comment

RikkiGibson commented Oct 30, 2020

RikkiGibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Oct 24, 2020 •

edited

Loading

RikkiGibson commented Oct 26, 2020 •

edited by jcouv

Loading

AlekseyTs Oct 27, 2020 •

edited by jcouv

Loading

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited

Loading

AlekseyTs Oct 27, 2020 •

edited by jcouv

Loading

AlekseyTs commented Oct 27, 2020 •

edited

Loading