Fix NRE in VisualBasicAddExplicitCastCodeFixProvider by kfechter · Pull Request #62075 · dotnet/roslyn

kfechter · 2022-06-22T15:05:11Z

Fixes issue #61538

NRE no longer occurs when a class attribute is used that triggers BC30519. Cast suggestions are now provided.

CyrusNajmabadi · 2022-06-22T16:02:30Z

...Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb

+                    Dim invocationNodeExpression = spanNode.GetAncestors(Of ExpressionSyntax).FirstOrDefault(
+                        Function(node) Not node.ChildNodes.OfType(Of ArgumentListSyntax).IsEmpty())


Suggested change

Dim invocationNodeExpression = spanNode.GetAncestors(Of ExpressionSyntax).FirstOrDefault(

Function(node) Not node.ChildNodes.OfType(Of ArgumentListSyntax).IsEmpty())

Dim invocationNodeExpression = spanNode.GetAncestors(Of InvocationExpressionSyntax).FirstOrDefault(

Function(node) node.ArgumentList is not nothing)

?

Unclear as to what this change is for.

Not against making the change, Just need clarification on why this is better than the code that was originally there.

instead of checking every arbitrary node, you are only looking at invocations. and because you're only looking at invocatinos, you don't have to enumerate all child nodes looking for argumentlists, you can just directly check if it has an argument list or not.

(the same feedback applies to the check below this for atttributes as well).

thanks!

Gotcha! I'll make that change tonight and test it once I'm off work

thanks.

Code change made and tested locally, just working through adding the unit test.

genlu · 2022-06-22T18:43:02Z

Hey @kfechter, thanks for your contribution! Could you please also add a unittest here to cover the scenario?

kfechter · 2022-06-22T23:51:53Z

Hey @kfechter, thanks for your contribution! Could you please also add a unittest here to cover the scenario?

I'm trying to work through adding this unit test but I'm hitting a bit of a roadblock. I'm positive this is just due to my not understanding how the unit tests work, but I've added one by looking at what the others are doing and trying to replicate that with this scenario, but the test just doesn't seem to be working.

The below is what I am trying to do to implement this test.

        <Fact(), Trait(Traits.Feature, Traits.Features.CodeActionsAddExplicitCast)>
        Public Async Function TestClassAttribute() As Task
            Await TestInRegularAndScriptAsync(
"Option Strict On
Class Program
    <System.Runtime.InteropServices.ClassInterface(|1|)>
    Class Base
    End Class
End Class",
"Option Strict On
Class Program
    <System.Runtime.InteropServices.ClassInterface(CShort(1))>
    Class Base
    End Class
End Class")
        End Function

however, with the above code, I get the following error
System.InvalidOperationException : Sequence contains no elements

This is my first time delving into these unit tests, so I'm positive I'm just missing something super simple

dibarbet · 2022-06-23T00:00:25Z

however, with the above code, I get the following error
System.InvalidOperationException : Sequence contains no elements

This might not be the only cause, but I think the caret location marker is missing brackets. Your code shows

ClassInterface(|1|)

but I believe it should be

ClassInterface([|1|])

e.g. https://github.com/dotnet/roslyn/blob/main/src/Analyzers/VisualBasic/Tests/AddExplicitCast/AddExplicitCastTests.vb#L30

kfechter · 2022-06-23T00:17:40Z

however, with the above code, I get the following error
System.InvalidOperationException : Sequence contains no elements

This might not be the only cause, but I think the caret location marker is missing brackets. Your code shows
ClassInterface(|1|)
but I believe it should be
ClassInterface([|1|])
e.g. https://github.com/dotnet/roslyn/blob/main/src/Analyzers/VisualBasic/Tests/AddExplicitCast/AddExplicitCastTests.vb#L30

ah ha. I'm getting further now. It throws "No action was offered when one was expected." but i'm probably missing something in the class definition that the test framework is expecting.

kfechter · 2022-06-23T00:29:26Z

I think I have it working. Just need to do a tiny bit more testing.

…and adding Unit Test

dibarbet · 2022-06-23T01:45:14Z

src/Analyzers/VisualBasic/Tests/AddExplicitCast/AddExplicitCastTests.vb


+        <Fact(), Trait(Traits.Feature, Traits.Features.CodeActionsAddExplicitCast)>
+        Public Async Function TestClassAttribute() As Task
+            Await TestMissingInRegularAndScriptAsync(


So this tests that the code action is missing, but we should be testing that it is present. The test snippet you posted before I think was the correct way to go.

I did notice though that the diagnostic only shows up when Option Strict Off is set. So for your old test code you may be able to fix it just by changing the On to Off

ah, that explains why I couldn't get the test to fail. I will revert that tomorrow. I left the power adapter for my Dev laptop at work.

Reverted and set Option Strict Off, but now I'm getting

System.Exception : No action was offered when one was expected.

I debugged, and it looks like the CodeAction is null, but i'm not understanding how the test framework actually gets the CodeActions to perform. below is my most recent test code.

<Fact(), Trait(Traits.Feature, Traits.Features.CodeActionsAddExplicitCast)> Public Async Function TestClassAttribute() As Task Await TestInRegularAndScriptAsync( "Option Strict Off Imports System.Runtime.InteropServices <ClassInterface([|1|])> Class Program End Class", "Option Strict Off Imports System.Runtime.InteropServices <ClassInterface(CShort(1))> Class Program End Class") End Function

Hmm - nothing immediately obvious stands out to me as being wrong. Potentially the test workspace doesn't have the right reference for System.Runtime.InteropServices? Not sure that is the case though. I can check out this branch tomorrow (its the end of my work day now) and see if I can help debug where its going wrong and report back.

i'd recommend not testing this exact interface, but rather just include a simple copy of it in the test.

@kfechter I finally got it working - looks like I needed to pass the option strict as a compilation option instead. The change I made is here - dibarbet@5cf383c

I can either update your branch here or I can leave it to you to update the test, whichever you prefer.

I can update the test. I appreciate the help!

CyrusNajmabadi · 2022-06-23T18:59:47Z

...Analyzers/VisualBasic/CodeFixes/AddExplicitCast/VisualBasicAddExplicitCastCodeFixProvider.vb

+                    Dim invocationNodeExpression = spanNode.GetAncestors(Of InvocationExpressionSyntax).FirstOrDefault(
+                        Function(node) node.ArgumentList IsNot Nothing)
+
+                    Dim invocationNodeAttribute = spanNode.GetAncestors(Of AttributeSyntax).FirstOrDefault(


Suggested change

Dim invocationNodeAttribute = spanNode.GetAncestors(Of AttributeSyntax).FirstOrDefault(

Dim attributeNode = spanNode.GetAncestors(Of AttributeSyntax).FirstOrDefault(

dibarbet

lgtm! Our main branch is locked down due to some infra issues, but I'll merge this when it opens. Thanks!!

kfechter added 2 commits June 22, 2022 10:38

Fix NRE in VisualBasicAddExplicitCastCodeFixProvider

cb1c253

Merge branch 'main' of github.com:kfechter/roslyn

b1a3961

kfechter requested a review from a team as a code owner June 22, 2022 15:05

ghost added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jun 22, 2022

CyrusNajmabadi reviewed Jun 22, 2022

View reviewed changes

Refactoring VisualBasicAddExplicitCastCodeFixProvider for efficiency …

7a9a953

…and adding Unit Test

dibarbet reviewed Jun 23, 2022

View reviewed changes

CyrusNajmabadi reviewed Jun 23, 2022

View reviewed changes

kfechter added 2 commits June 23, 2022 21:01

Update variable names to match suggestion

36feccb

Implementing UnitTest for Class Attribute Scenario

6157a38

dibarbet approved these changes Jun 26, 2022

View reviewed changes

dibarbet merged commit 9bf8d8d into dotnet:main Jun 27, 2022

ghost added this to the Next milestone Jun 27, 2022

RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022

CyrusNajmabadi mentioned this pull request Nov 22, 2024

NRE in VisualBasicAddExplicitCastCodeFixProvider #61538

Closed

		Dim invocationNodeExpression = spanNode.GetAncestors(Of ExpressionSyntax).FirstOrDefault(
		Function(node) Not node.ChildNodes.OfType(Of ArgumentListSyntax).IsEmpty())

	Dim invocationNodeAttribute = spanNode.GetAncestors(Of AttributeSyntax).FirstOrDefault(
	Dim attributeNode = spanNode.GetAncestors(Of AttributeSyntax).FirstOrDefault(

Conversation

kfechter commented Jun 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfechter commented Jun 22, 2022

Uh oh!

dibarbet commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfechter commented Jun 23, 2022

Uh oh!

kfechter commented Jun 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfechter Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dibarbet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

genlu commented Jun 22, 2022 •

edited

Loading

dibarbet commented Jun 23, 2022 •

edited

Loading

kfechter Jun 23, 2022 •

edited

Loading

CyrusNajmabadi Jun 23, 2022 •

edited

Loading