Fix NRE in BasicDoNotUseCountWhenAnyCanBeUsedFixer #3981

Youssef1313 · 2020-08-09T12:35:28Z

Youssef1313 · 2020-08-09T13:03:10Z

This PR just mirrors the original fix made by @mavasani in 2404ec0 to the VB fixer, and added a test to confirm if it works or not.

Youssef1313 · 2020-08-09T14:52:03Z

After mirroring 2404ec0, the NRE is gone, but the fixer isn't working. I'll debug that soon and try to figure out where things goes wrong.

Youssef1313 · 2020-08-09T16:02:17Z

I think the async case might still be broken. I can't think of a good solution.
I'm thinking of refactoring the whole fixer to use semantic analysis instead of syntax analysis, that would be ideal IMO, but very tedious.

Any ideas @mavasani @Evangelink?

codecov · 2020-08-09T16:21:00Z

Codecov Report

Merging #3981 into master will increase coverage by 0.16%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##           master    #3981      +/-   ##
==========================================
+ Coverage   95.61%   95.77%   +0.16%     
==========================================
  Files        1155     1165      +10     
  Lines      254900   264043    +9143     
  Branches    15272    15935     +663     
==========================================
+ Hits       243724   252895    +9171     
+ Misses       9215     9139      -76     
- Partials     1961     2009      +48

Evangelink

I think the async case might still be broken. I can't think of a good solution.
I'm thinking of refactoring the whole fixer to use semantic analysis instead of syntax analysis, that would be ideal IMO, but very tedious.

Not sure to understand which case you are referring to, could you please add a test even if failing?

Evangelink · 2020-08-09T17:22:23Z

...sualBasic/Microsoft.NetCore.Analyzers/Performance/BasicDoNotUseCountWhenAnyCanBeUsedFixer.vb

@@ -104,7 +104,7 @@ Namespace Microsoft.NetCore.VisualBasic.Analyzers.Performance

        End Function

-        Private Shared Sub GetExpressionAndInvocationArguments(sourceExpression As ExpressionSyntax, isAsync As Boolean, ByRef expression As SyntaxNode, ByRef arguments As IEnumerable(Of SyntaxNode))
+        Private Shared Function TryGetExpressionAndInvocationArguments(sourceExpression As ExpressionSyntax, isAsync As Boolean, ByRef expression As SyntaxNode, ByRef arguments As IEnumerable(Of SyntaxNode)) As Boolean


I am not good with vbnet and I have noticed the code was already the same before but isn't it expected to have the Out attribute for the last 2 parameters?

I think OutAttribute is usually used for interop scenarios. But it won't be necessary here.
However, I'm not 100% sure.

Youssef1313 · 2020-08-09T17:55:37Z

@Evangelink I added the failing test for the async case

Evangelink · 2020-08-09T18:49:38Z

...nitTests/Microsoft.NetCore.Analyzers/Performance/DoNotUseCountWhenAnyCanBeUsedTests.Tests.cs

@@ -1051,6 +1054,7 @@ End Function
            b = Not Await GetData().AnyAsync()


That remains as (Await GetData().CountAsync).Equals(IntegerZero)

That is not the case for the parentheses scenario (Await GetData().CountAsync()).Equals(IntegerZero) or am I missing something?

@Jozkee I couldn't get what you mean

I was replying to @Evangelink's suggestion; what I got from his comment was that (Await GetData().CountAsync).Equals(IntegerZero) (CountAsync without parentheses) should not be caught by the analyzer and I said that I disagree given that the analyzer already catches (Await GetData().CountAsync()).Equals(IntegerZero) (CountAsync with parentheses).

We should be consistent.

@Jozkee The comment from @Evangelink was before I had fixed the async case (i.e. before applying your suggestion in #3981 (comment)). He was pointing to the test failure that was expecting Not Await GetData().AnyAsync(), but the actual was (Await GetData().CountAsync).Equals(IntegerZero)

So, that's what I was missing :)
Thanks for clarifying.

Evangelink · 2020-08-09T19:18:33Z

...sualBasic/Microsoft.NetCore.Analyzers/Performance/BasicDoNotUseCountWhenAnyCanBeUsedFixer.vb

-            expression = DirectCast(invocationExpression.Expression, MemberAccessExpressionSyntax).Expression
-            arguments = invocationExpression.ArgumentList.ChildNodes()
+            Dim memberAccessExpression = TryCast(sourceExpression, MemberAccessExpressionSyntax)
+            If memberAccessExpression IsNot Nothing Then ' This case happens for something like: x = GetData().Count where Count method is called without parentheses.


There shall be a similar logic on line 126 (inside of the If isAsync Then block)

Correct, that's why Async scenarios were not working.

Instead of duplicating the code on the async case, you can refactor the conditions to something like this:

' Extract source expression from Async expression if needed. If isAsync Then Dim awaitExpressionSyntax = TryCast(sourceExpression, AwaitExpressionSyntax) If Not awaitExpressionSyntax Is Nothing Then sourceExpression = awaitExpressionSyntax.Expression End If End If ' Try to get source expression as InvocationExpression. Dim invocationExpression As InvocationExpressionSyntax = TryCast(sourceExpression, InvocationExpressionSyntax) If invocationExpression IsNot Nothing Then expression = DirectCast(invocationExpression.Expression, MemberAccessExpressionSyntax).Expression arguments = invocationExpression.ArgumentList.ChildNodes() Return True End If ' Try to get source expression as MemberAccessExpression which is only legal on VB. Dim memberAccessExpression = TryCast(sourceExpression, MemberAccessExpressionSyntax) If memberAccessExpression IsNot Nothing Then expression = memberAccessExpression.Expression arguments = Enumerable.Empty(Of SyntaxNode)() Return True End If

mavasani · 2020-08-20T15:37:53Z

@Jozkee for review

Jozkee

Thanks for the PR, could you please fix the Async scenario that is currently failing?
I left a suggestion next to @Evangelink's comment on how you can fix it.

I also left a few other comments related to to improve tests and some style suggestions.

Jozkee · 2020-09-02T07:35:01Z

...nitTests/Microsoft.NetCore.Analyzers/Performance/DoNotUseCountWhenAnyCanBeUsedTests.Tests.cs

@@ -458,6 +458,7 @@ End Function
    Sub M()
        Dim b As Boolean

+        b = {{|{this.Verifier.DiagnosticId}:GetData().Count.Equals(IntegerZero)|}}


Can you add this non parentheses scenario for 0.Equals(GetData().Count), GetData().Count = 0 and 0 = GetData().Count cases below?

Jozkee · 2020-09-02T07:36:12Z

...nitTests/Microsoft.NetCore.Analyzers/Performance/DoNotUseCountWhenAnyCanBeUsedTests.Tests.cs

@@ -990,6 +992,7 @@ End Function
        Async Sub M()
            Dim b As Boolean

+            b = {{|{this.Verifier.DiagnosticId}:(Await GetData().CountAsync).Equals(IntegerZero)|}}


Same as my previous comment, could you please add this case on the other expressions?

Jozkee · 2020-09-02T07:39:11Z

...sualBasic/Microsoft.NetCore.Analyzers/Performance/BasicDoNotUseCountWhenAnyCanBeUsedFixer.vb

-
-                            Return True
+                                arguments:=arguments) Then
+                                Return True


You can Return TryGetExpressionAndInvocationArguments instead, same for the other cases.

Jozkee · 2020-09-02T07:47:55Z

...sualBasic/Microsoft.NetCore.Analyzers/Performance/BasicDoNotUseCountWhenAnyCanBeUsedFixer.vb

-            expression = DirectCast(invocationExpression.Expression, MemberAccessExpressionSyntax).Expression
-            arguments = invocationExpression.ArgumentList.ChildNodes()
+            Dim memberAccessExpression = TryCast(sourceExpression, MemberAccessExpressionSyntax)
+            If memberAccessExpression IsNot Nothing Then ' This case happens for something like: x = GetData().Count where Count method is called without parentheses.


Move the comment to a new line on top of line 144 so its clear that this is a special scenario.

mavasani · 2020-09-21T14:14:33Z

@Youssef1313 would you be able to resolve the review comments?

mavasani · 2020-09-22T01:52:19Z

@Jozkee can you please take another look?

Jozkee

Otherwise; lgtm, thanks.

...sualBasic/Microsoft.NetCore.Analyzers/Performance/BasicDoNotUseCountWhenAnyCanBeUsedFixer.vb

…rmance/BasicDoNotUseCountWhenAnyCanBeUsedFixer.vb Co-authored-by: David Cantú <dacantu@microsoft.com>

Fix NRE in BasicDoNotUseCountWhenAnyCanBeUsedFixer

63adf7d

Youssef1313 requested a review from a team as a code owner August 9, 2020 12:35

Mirror VB fixer to the C# one

c5b3c57

Youssef1313 changed the title ~~WIP: Fix NRE in BasicDoNotUseCountWhenAnyCanBeUsedFixer~~ Fix NRE in BasicDoNotUseCountWhenAnyCanBeUsedFixer Aug 9, 2020

Youssef1313 and others added 2 commits August 9, 2020 15:13

Update BasicDoNotUseCountWhenAnyCanBeUsedFixer.vb

6eb2a0f

One more fix

b0ebbc6

Fix

3443da0

Evangelink reviewed Aug 9, 2020

View reviewed changes

Add test for the async case

924a80a

Evangelink reviewed Aug 9, 2020

View reviewed changes

mavasani requested a review from Jozkee August 10, 2020 23:27

Jozkee requested changes Sep 2, 2020

View reviewed changes

Youssef1313 added 3 commits September 21, 2020 20:01

Update tests

2702b90

Simplify based on feedback

179e2fd

Fix async cases based on suggestion

d529627

Jozkee approved these changes Sep 23, 2020

View reviewed changes

Youssef1313 and others added 3 commits September 23, 2020 11:31

Simplify

685cce5

Update src/NetAnalyzers/VisualBasic/Microsoft.NetCore.Analyzers/Perfo…

f9f91ae

…rmance/BasicDoNotUseCountWhenAnyCanBeUsedFixer.vb Co-authored-by: David Cantú <dacantu@microsoft.com>

Fix formatting issue

716b3fd

Jozkee merged commit 2b9511d into dotnet:master Sep 23, 2020

Youssef1313 deleted the patch-1 branch September 23, 2020 21:54

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 NRE in BasicDoNotUseCountWhenAnyCanBeUsedFixer #3981

Fix NRE in BasicDoNotUseCountWhenAnyCanBeUsedFixer #3981

Youssef1313 commented Aug 9, 2020

Youssef1313 commented Aug 9, 2020

Youssef1313 commented Aug 9, 2020

Youssef1313 commented Aug 9, 2020

codecov bot commented Aug 9, 2020 •

edited

Evangelink left a comment

Evangelink Aug 9, 2020

Youssef1313 Aug 9, 2020

Youssef1313 commented Aug 9, 2020

Evangelink Aug 9, 2020

Jozkee Sep 23, 2020

Youssef1313 Sep 23, 2020

Jozkee Sep 23, 2020

Youssef1313 Sep 23, 2020 •

edited

Jozkee Sep 23, 2020

Evangelink Aug 9, 2020

Jozkee Sep 2, 2020

mavasani commented Aug 20, 2020

Jozkee left a comment

Jozkee Sep 2, 2020

Jozkee Sep 2, 2020

Jozkee Sep 2, 2020

Jozkee Sep 2, 2020

mavasani commented Sep 21, 2020

mavasani commented Sep 22, 2020

Jozkee left a comment

		@@ -1051,6 +1054,7 @@ End Function
		b = Not Await GetData().AnyAsync()

Fix NRE in BasicDoNotUseCountWhenAnyCanBeUsedFixer #3981

Fix NRE in BasicDoNotUseCountWhenAnyCanBeUsedFixer #3981

Conversation

Youssef1313 commented Aug 9, 2020

Youssef1313 commented Aug 9, 2020

Youssef1313 commented Aug 9, 2020

Youssef1313 commented Aug 9, 2020

codecov bot commented Aug 9, 2020 • edited

Codecov Report

Evangelink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 commented Aug 9, 2020

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

Youssef1313 Sep 23, 2020 • edited

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

mavasani commented Aug 20, 2020

Jozkee left a comment

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

Choose a reason for hiding this comment

mavasani commented Sep 21, 2020

mavasani commented Sep 22, 2020

Jozkee left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2020 •

edited

Youssef1313 Sep 23, 2020 •

edited