Fix CA2016 when used on instance method with generic types #4974

DavidBoike · 2021-03-23T18:10:07Z

Adds a failing test case for the problem matching the description of CA2016: Code fix removes generic type parameter from message #4870
Adds a test case for a static generic method, which was green before applying the fix, to guard against regressions in that behavior

dnfadmin · 2021-03-23T18:10:21Z

All CLA requirements met.

codecov · 2021-03-23T18:57:46Z

Codecov Report

Merging #4974 (c2bc092) into main (8236e8b) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head c2bc092 differs from pull request most recent head e05098e. Consider uploading reports for the commit e05098e to get more accurate results

@@            Coverage Diff             @@
##             main    #4974      +/-   ##
==========================================
+ Coverage   95.52%   95.58%   +0.05%     
==========================================
  Files        1200     1190      -10     
  Lines      274867   273033    -1834     
  Branches    16622    16486     -136     
==========================================
- Hits       262571   260974    -1597     
+ Misses      10112     9921     -191     
+ Partials     2184     2138      -46

carlossanlop · 2021-03-24T02:29:57Z

...zers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Fixer.cs

-                case IConditionalAccessInstanceOperation:
-                    newInstance = GetConditionalOperationInvocationExpression(invocation.Syntax);
-                    break;
-                default:
-                    if (invocation.Instance.IsImplicit)
-                    {
-                        newInstance = invocation.GetInstanceSyntax()!;
-                    }


Adds a test case for a static generic method, which was green before applying the fix, to guard against regressions in that behavior

The CI is still passing because apparently I forgot to add unit tests for all three cases you removed.

It seems we don't have a conditional operation case in the unit tests. Can you please add one?

I'm trying to remember what IsImplicit is referring to. It's either:

An invocation of a method from an explicit interface implementation, or

An invocation of a method passed as a callback to another method (a Func or an Action).
Independently if it's one or the other, we don't have unit tests for any of them. Would you mind adding one to make sure that functionality is not regressed?

Thanks for adding a method for the static case.

From the IConditionalAccessInstanceOperation Interface docs it sounds like you're referring to a call like someExpression?.Method(). Isn't that covered by CS_Diagnostic_DereferencePossibleNullReference?

IsImplicit seems to refer to the "this" object passed to the method, either the instance or null if it's a static method. In any case, when I tried adding this block to the fixer:

if (invocation.Instance != null && invocation.Instance.IsImplicit) { throw new Exception("Implicit"); }

…the result was that 73 tests failed. First one was CS_Diagnostic_ActionDelegateAwait where MethodAsync() is being called without this., which seems to track. So I think it's safe to say that test case is covered.

While the removal of that code looks scary, I think it makes sense philisophically. The fixer is only trying to adjust what's going on in the ArgumentList. So trying to rebuild the part of the invocation "outside" the parens from scratch based on a number of conditions doesn't really make sense when expression is already going to contain whatever that outside part was originally highlighted by the red squiggly.

To illustrate that point further, I added 2 additional tests CS_Diagnostic_NullCoalescedDelegates and CS_Diagnostic_NullCoalescedDelegatesWithInvoke, both of which have 2 delegates and call either (f1 ?? f2)() or (f1 ?? f2).Invoke(). The current version on main will rewrite (f1 ?? f2)() to (f1 ?? f2).Invoke(c), adding the explicit Invoke where none was there before. I assume it's preferable for a fixer to do the bare minimum to implement the fix without changing any other code, and this PR respects whether or not the explicit Invoke is used in the expression.

carlossanlop · 2021-03-24T02:30:24Z

.../UnitTests/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocationsTests.cs

@@ -2229,6 +2229,78 @@ void LocalMethod()
            return CS8VerifyCodeFixAsync(originalCode, fixedCode);
        }

+        [Fact]
+        [WorkItem(4870, "https://github.com/dotnet/roslyn-analyzers/issues/4870")]
+        public Task CS_Diagnostic_GenericTypeParamOnInstanceMethod()


Can you please also add VB unit tests?

Added VB versions of the test. Dislcaimer: not a VB developer. I leaned on the online Telerik code converter and a bunch of guess & check.

We all use Telerik 😄 .

carlossanlop

Thank you for the change, @DavidBoike . I left a couple of requests.

DavidBoike

@carlossanlop ready for another review.

DavidBoike · 2021-03-24T19:18:37Z

.../UnitTests/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocationsTests.cs

@@ -2229,6 +2229,78 @@ void LocalMethod()
            return CS8VerifyCodeFixAsync(originalCode, fixedCode);
        }

+        [Fact]
+        [WorkItem(4870, "https://github.com/dotnet/roslyn-analyzers/issues/4870")]
+        public Task CS_Diagnostic_GenericTypeParamOnInstanceMethod()


Added VB versions of the test. Dislcaimer: not a VB developer. I leaned on the online Telerik code converter and a bunch of guess & check.

DavidBoike · 2021-03-24T20:27:23Z

...zers/Core/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocations.Fixer.cs

-                case IConditionalAccessInstanceOperation:
-                    newInstance = GetConditionalOperationInvocationExpression(invocation.Syntax);
-                    break;
-                default:
-                    if (invocation.Instance.IsImplicit)
-                    {
-                        newInstance = invocation.GetInstanceSyntax()!;
-                    }


From the IConditionalAccessInstanceOperation Interface docs it sounds like you're referring to a call like someExpression?.Method(). Isn't that covered by CS_Diagnostic_DereferencePossibleNullReference?

IsImplicit seems to refer to the "this" object passed to the method, either the instance or null if it's a static method. In any case, when I tried adding this block to the fixer:

if (invocation.Instance != null && invocation.Instance.IsImplicit) { throw new Exception("Implicit"); }

…the result was that 73 tests failed. First one was CS_Diagnostic_ActionDelegateAwait where MethodAsync() is being called without this., which seems to track. So I think it's safe to say that test case is covered.

While the removal of that code looks scary, I think it makes sense philisophically. The fixer is only trying to adjust what's going on in the ArgumentList. So trying to rebuild the part of the invocation "outside" the parens from scratch based on a number of conditions doesn't really make sense when expression is already going to contain whatever that outside part was originally highlighted by the red squiggly.

To illustrate that point further, I added 2 additional tests CS_Diagnostic_NullCoalescedDelegates and CS_Diagnostic_NullCoalescedDelegatesWithInvoke, both of which have 2 delegates and call either (f1 ?? f2)() or (f1 ?? f2).Invoke(). The current version on main will rewrite (f1 ?? f2)() to (f1 ?? f2).Invoke(c), adding the explicit Invoke where none was there before. I assume it's preferable for a fixer to do the bare minimum to implement the fix without changing any other code, and this PR respects whether or not the explicit Invoke is used in the expression.

DavidBoike · 2021-03-31T13:53:40Z

Anything else I can do here?

mavasani · 2021-05-17T08:33:04Z

Anything else I can do here?

@carlossanlop Can you please help review the PR again? We already have multiple users reporting this bug.

DavidBoike · 2021-05-17T18:13:10Z

I rebased the PR to make it easier to review.

DavidBoike · 2021-06-10T18:07:38Z

Checking in again. What can I do here?

DavidBoike · 2021-06-17T20:32:12Z

@mavasani @carlossanlop @buyaa-n Please let me know how I can help drive this forward. I'm willing to add more test cases or get on a call or whatever, just let me know.

adamralph · 2021-06-30T14:46:49Z

@mavasani @carlossanlop is there anything that I, @DavidBoike, or anyone else can do to help move this forward? It would be a shame to abandon it after the non-trivial effort that's already been made by @DavidBoike and @carlossanlop. And as mentioned above, multiple users have reported this problem.

buyaa-n

The fix looks good to me, I will merge it on Monday if @carlossanlop will not add any other comments/concerns

CC @mavasani

jeffhandley · 2021-07-10T02:11:56Z

@carlossanlop won't be available to review on Monday. LGTM.

adamralph · 2021-07-10T08:58:47Z

🎉 which SDK version is this scheduled for?

jeffhandley · 2021-07-13T17:35:29Z

This will be included in .NET 6 Preview 7.

DavidBoike requested a review from a team as a code owner March 23, 2021 18:10

DavidBoike mentioned this pull request Mar 23, 2021

CA2016: Code fix removes generic type parameter from message #4870

Closed

carlossanlop requested review from buyaa-n and carlossanlop March 24, 2021 02:10

carlossanlop assigned DavidBoike Mar 24, 2021

carlossanlop reviewed Mar 24, 2021

View reviewed changes

DavidBoike commented Mar 24, 2021

View reviewed changes

meziantou mentioned this pull request Apr 22, 2021

CA2016 fixer removes generic parameter #5051

Closed

Prevent CA2016 fixer from removing generic type parameter

e05098e

DavidBoike force-pushed the ca2016-fix branch from 2a35486 to e05098e Compare May 17, 2021 17:26

buyaa-n approved these changes Jul 10, 2021

View reviewed changes

jeffhandley merged commit f64768f into dotnet:main Jul 10, 2021

Youssef1313 mentioned this pull request Jul 10, 2021

[CA2016] Forward the CancellationToken to the method or pass 'CancellationToken.None' doesn't use the right overload for instance methods. dotnet/roslyn#54743

Closed

DavidBoike deleted the ca2016-fix branch July 12, 2021 20:42

jmarolf mentioned this pull request Jan 31, 2022

infrastructure/report test hangs and crashes jmarolf/roslyn-analyzers#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CA2016 when used on instance method with generic types #4974

Fix CA2016 when used on instance method with generic types #4974

DavidBoike commented Mar 23, 2021 •

edited by carlossanlop

dnfadmin commented Mar 23, 2021 •

edited

codecov bot commented Mar 23, 2021 •

edited

carlossanlop Mar 24, 2021

DavidBoike Mar 24, 2021

carlossanlop Mar 24, 2021

DavidBoike Mar 24, 2021

carlossanlop Mar 25, 2021

carlossanlop left a comment

DavidBoike left a comment

DavidBoike Mar 24, 2021

DavidBoike Mar 24, 2021

DavidBoike commented Mar 31, 2021

mavasani commented May 17, 2021

DavidBoike commented May 17, 2021

DavidBoike commented Jun 10, 2021

DavidBoike commented Jun 17, 2021

adamralph commented Jun 30, 2021 •

edited

buyaa-n left a comment

jeffhandley commented Jul 10, 2021

adamralph commented Jul 10, 2021

jeffhandley commented Jul 13, 2021

Fix CA2016 when used on instance method with generic types #4974

Fix CA2016 when used on instance method with generic types #4974

Conversation

DavidBoike commented Mar 23, 2021 • edited by carlossanlop

dnfadmin commented Mar 23, 2021 • edited

codecov bot commented Mar 23, 2021 • edited

Codecov Report

carlossanlop Mar 24, 2021

Choose a reason for hiding this comment

DavidBoike Mar 24, 2021

Choose a reason for hiding this comment

carlossanlop Mar 24, 2021

Choose a reason for hiding this comment

DavidBoike Mar 24, 2021

Choose a reason for hiding this comment

carlossanlop Mar 25, 2021

Choose a reason for hiding this comment

carlossanlop left a comment

Choose a reason for hiding this comment

DavidBoike left a comment

Choose a reason for hiding this comment

DavidBoike Mar 24, 2021

Choose a reason for hiding this comment

DavidBoike Mar 24, 2021

Choose a reason for hiding this comment

DavidBoike commented Mar 31, 2021

mavasani commented May 17, 2021

DavidBoike commented May 17, 2021

DavidBoike commented Jun 10, 2021

DavidBoike commented Jun 17, 2021

adamralph commented Jun 30, 2021 • edited

buyaa-n left a comment

Choose a reason for hiding this comment

jeffhandley commented Jul 10, 2021

adamralph commented Jul 10, 2021

jeffhandley commented Jul 13, 2021

DavidBoike commented Mar 23, 2021 •

edited by carlossanlop

dnfadmin commented Mar 23, 2021 •

edited

codecov bot commented Mar 23, 2021 •

edited

adamralph commented Jun 30, 2021 •

edited