Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Incorrect Overrides completion in VB #39592

Closed
AlekseyTs opened this issue Oct 30, 2019 · 9 comments · Fixed by #39629
Closed

Incorrect Overrides completion in VB #39592

AlekseyTs opened this issue Oct 30, 2019 · 9 comments · Fixed by #39629
Assignees
@ivanbasov
Labels
Area-IDE Bug Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone
16.5.P1

Comments

@AlekseyTs
Copy link
Contributor

Version Used:
Microsoft Visual Studio Enterprise 2019 Int Preview
Version 16.5.0 Preview 1.0 [29427.7.master]

Steps to Reproduce:

  1. Add the following API to Compilation class in src\Compilers\Core\Portable\Compilation\Compilation.cs
        public abstract ImmutableArray<MetadataReference> Test();
  1. Go to VisualBasicCompilation class in src\Compilers\VisualBasic\Portable\Compilation\VisualBasicCompilation.vb
  2. Type Overrides, press space and select Test from the list of methods available to override.

Expected Behavior:

        Overrides Function Test() As ImmutableArray(Of MetadataReference)
            Throw New System.NotImplementedException()
        End Function

Actual Behavior:

        Overrides Test()

The following entry in the log is likely related:

  <entry>
    <record>968</record>
    <time>2019/10/30 22:32:45.547</time>
    <type>Error</type>
    <source>Editor or Editor Extension</source>
    <description>System.InvalidCastException: Unable to cast object of type &apos;Microsoft.CodeAnalysis.VisualBasic.Syntax.MemberAccessExpressionSyntax&apos; to type &apos;Microsoft.CodeAnalysis.VisualBasic.Syntax.TypeSyntax&apos;.&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitCrefSignaturePart(CrefSignaturePartSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.CrefSignaturePartSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement[TNode](TNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList[TNode](SeparatedSyntaxList`1 list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitCrefSignature(CrefSignatureSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.CrefSignatureSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitCrefReference(CrefReferenceSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.CrefReferenceSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitXmlCrefAttribute(XmlCrefAttributeSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.XmlCrefAttributeSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement[TNode](TNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList[TNode](SyntaxList`1 list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitXmlEmptyElement(XmlEmptyElementSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.XmlEmptyElementSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement[TNode](TNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList[TNode](SyntaxList`1 list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitXmlElement(XmlElementSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.XmlElementSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement[TNode](TNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList[TNode](SyntaxList`1 list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitDocumentationCommentTrivia(DocumentationCommentTriviaSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.DocumentationCommentTriviaSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitTrivia(SyntaxTrivia trivia)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement(SyntaxTrivia element)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList(SyntaxTriviaList list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitToken(SyntaxToken token)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement(SyntaxToken token)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList(SyntaxTokenList list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitMethodStatement(MethodStatementSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.MethodStatementSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitMethodBlock(MethodBlockSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.MethodBlockSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement[TNode](TNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList[TNode](SyntaxList`1 list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitClassBlock(ClassBlockSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.ClassBlockSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement[TNode](TNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList[TNode](SyntaxList`1 list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitNamespaceBlock(NamespaceBlockSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.NamespaceBlockSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitListElement[TNode](TNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitList[TNode](SyntaxList`1 list)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.VisitCompilationUnit(CompilationUnitSyntax node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Syntax.CompilationUnitSyntax.Accept[TResult](VisualBasicSyntaxVisitor`1 visitor)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.VisualBasicSyntaxRewriter.Visit(SyntaxNode node)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.VisualBasic.Editing.VisualBasicImportAdder.MakeSafeToAddNamespaces(SyntaxNode root, IEnumerable`1 namespaceMembers, IEnumerable`1 extensionMethods, SemanticModel model, Workspace workspace, CancellationToken cancellationToken)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Editing.ImportAdderService.MakeSafeToAddNamespaces(SyntaxNode root, IEnumerable`1 namespaceSymbols, SemanticModel model, Workspace workspace, CancellationToken cancellationToken)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Editing.ImportAdderService.&lt;AddImportsAsync&gt;d__1.MoveNext()&#x000D;&#x000A;--- End of stack trace from previous location where exception was thrown ---&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Editing.ImportAdder.&lt;AddImportsFromSymbolAnnotationAsync&gt;d__8.MoveNext()&#x000D;&#x000A;--- End of stack trace from previous location where exception was thrown ---&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.CodeGeneration.AbstractCodeGenerationService.&lt;GetEditAsync&gt;d__39.MoveNext()&#x000D;&#x000A;--- End of stack trace from previous location where exception was thrown ---&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Completion.Providers.AbstractMemberInsertingCompletionProvider.&lt;GenerateMemberAndUsingsAsync&gt;d__9.MoveNext()&#x000D;&#x000A;--- End of stack trace from previous location where exception was thrown ---&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Completion.Providers.AbstractMemberInsertingCompletionProvider.&lt;DetermineNewDocumentAsync&gt;d__8.MoveNext()&#x000D;&#x000A;--- End of stack trace from previous location where exception was thrown ---&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Completion.Providers.AbstractMemberInsertingCompletionProvider.&lt;GetChangeAsync&gt;d__7.MoveNext()&#x000D;&#x000A;--- End of stack trace from previous location where exception was thrown ---&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)&#x000D;&#x000A;   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Completion.CompletionServiceWithProviders.&lt;GetChangeAsync&gt;d__38.MoveNext()&#x000D;&#x000A;--- End of stack trace from previous location where exception was thrown ---&#x000D;&#x000A;   at Roslyn.Utilities.TaskExtensions.WaitAndGetResult_CanCallOnBackground[T](Task`1 task, CancellationToken cancellationToken)&#x000D;&#x000A;   at Roslyn.Utilities.TaskExtensions.WaitAndGetResult[T](Task`1 task, CancellationToken cancellationToken)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.AsyncCompletion.CommitManager.Commit(Document document, CompletionService completionService, ITextView view, ITextBuffer subjectBuffer, CompletionItem roslynItem, TextSpan completionListSpan, Nullable`1 commitCharacter, ITextSnapshot triggerSnapshot, CompletionRules rules, String filterText, CancellationToken cancellationToken)&#x000D;&#x000A;   at Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.AsyncCompletion.CommitManager.TryCommit(IAsyncCompletionSession session, ITextBuffer subjectBuffer, CompletionItem item, Char typeChar, CancellationToken cancellationToken)&#x000D;&#x000A;   at Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Implementation.AsyncCompletionSession.&lt;&gt;c__DisplayClass78_1.&lt;CommitItem&gt;b__0()&#x000D;&#x000A;   at Microsoft.VisualStudio.Text.Utilities.GuardedOperations.CallExtensionPoint[T](Object errorSource, Func`1 call, T valueOnThrow, Predicate`1 exceptionToIgnore, Predicate`1 exceptionToHandle)&#x000D;&#x000A;--- End of stack trace from previous location where exception was thrown ---&#x000D;&#x000A;   at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)</description>
  </entry>
@AlekseyTs
Copy link
Contributor Author

@jinujoseph FYI

@AlekseyTs AlekseyTs changed the title Incorrect Override completion in VB Incorrect Overrides completion in VB Oct 30, 2019
@jinujoseph jinujoseph added the Bug label Oct 30, 2019
@jinujoseph jinujoseph added this to the 16.4 milestone Oct 30, 2019
@ivanbasov
Copy link
Contributor

Made some debugging and found that the NFW likely causes the completion to fail but the NFW occurs out of the completion path. It happens in the Import Adder (see #37228). I do not know details of triggering this feature.

There is a simpler scenario to get trigger the NFW: go to Go to VisualBasicCompilation class in src\Compilers\VisualBasic\Portable\Compilation\VisualBasicCompilation.vb and hit enter within the class to add a new line. If you are in the debug mode, you can catch the same fault.

Nevertheless, the Import Adder calls VisualBasicSyntaxRewriter which analyzes the VisualBasicCompilation.vb file. When it comes to

roslyn/src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb

Line 2774 in 47215bd

''' SymbolFilter, CancellationToken)"/> when predicate is just a simple string check.

It considers SymbolFilter as an identifier not as a type and fails here:
https://github.com/dotnet/roslyn/blob/master/src/Compilers/VisualBasic/Portable/Generated/Syntax.xml.Main.Generated.vb

     Public Overrides Function VisitCrefSignaturePart(ByVal node As CrefSignaturePartSyntax) As SyntaxNode

            Dim anyChanges As Boolean = False

            Dim newModifier = DirectCast(VisitToken(node.Modifier).Node, InternalSyntax.KeywordSyntax)
            If node.Modifier.Node IsNot newModifier Then anyChanges = True
            Dim newType = DirectCast(Visit(node.Type), TypeSyntax) --- CAN NOT convert member access syntax to type syntax.

Questions:

  1. @AlekseyTs , is it a problem in parsing a comment with a type split into two lines in VB?
  2. @YairHalberstadt , @JoeRobich , should we handle exception happened within Import Adder or should we throw them? Not sure, how this feature works.

@AlekseyTs
Copy link
Contributor Author

It looks like node.Type is a TypeSyntax. Since the cast fails after we visit it, it looks like the node is rewritten to something other than a TypeSyntax. I am not sure what kind of rewrite is happening in the scenario. Note that IdentifierNameSyntax is a TypeSyntax.

@ivanbasov
Copy link
Contributor

Thank you, @AlekseyTs ! So, it means that the rewriter is responsible for the type of the node. In the case, it is MemberAccessExpressionSyntax. @YairHalberstadt and @JoeRobich , could you please take a look? My assumption it was introduced with #37228

@JoeRobich
Copy link
Member

JoeRobich commented Oct 31, 2019 •
edited
Loading

So the issue seems to be that the ImportAdder wants to expand everything and at the point that it is trying to expand the SymbolFilter IdentifierNameSyntax node the parent is a CrefSignaturePartSyntax ( the grandparent is a CrefSignatureSyntax and the great-grandparent is a CrefReferenceSyntax). Causing the else block to execute which returns a MemberAccessExpression instead of a QualifiedNameSyntax.

roslyn/src/Workspaces/VisualBasic/Portable/Simplification/VisualBasicSimplificationService.Expander.vb

Lines 773 to 783 in 5cae62a

If SyntaxFacts.IsInNamespaceOrTypeContext(originalNode) OrElse TypeOf (parent) Is CrefReferenceSyntax Then
Dim right = DirectCast(rewrittenNode, SimpleNameSyntax)
result = rewrittenNode.CopyAnnotationsTo(SyntaxFactory.QualifiedName(DirectCast(left, NameSyntax), right.WithAdditionalAnnotations(Simplifier.SpecialTypeAnnotation)))
Else
result = rewrittenNode.CopyAnnotationsTo(
SyntaxFactory.MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
DirectCast(left, ExpressionSyntax),
SyntaxFactory.Token(SyntaxKind.DotToken),
DirectCast(rewrittenNode, SimpleNameSyntax)))
End If

@YairHalberstadt
Copy link
Contributor

I'll try and take a look if I get the chance.

My gut feeling is the solution is to check that the simplifier returns the correct type, and if not don't expand and add a warning that we couldn't expand it. We already do that for extension methods.

@YairHalberstadt
Copy link
Contributor

YairHalberstadt commented Nov 1, 2019 •
edited
Loading

Causing the else block to execute which returns a MemberAccessExpression instead of a QualifiedNameSyntax.

I have no idea what the purpose of the else block is, and this dates back to before Roslyn was on GitHub, so I can't find out that way.

@YairHalberstadt
Copy link
Contributor

I think the solution is actually to add CrefSignaturePart to IsInTypeOnlyContext:

roslyn/src/Compilers/VisualBasic/Portable/Syntax/SyntaxFacts.vb

Line 70 in 47215bd

Public Shared Function IsInTypeOnlyContext(node As ExpressionSyntax) As Boolean

SyntaxFacts.cs already does this for CRefParameter

YairHalberstadt added a commit to YairHalberstadt/roslyn that referenced this issue Nov 1, 2019
@YairHalberstadt
Add CrefSignaturePartTo SyntaxFacts.IsInTypeOnlyContext to fix dotnet…
7355f34 
…#39592
@YairHalberstadt
Copy link
Contributor

Fix at #39629

@YairHalberstadt YairHalberstadt mentioned this issue Nov 1, 2019
Merged
@jinujoseph jinujoseph added the 4 - In Review A fix for the issue is submitted for review. label Nov 1, 2019
@ivanbasov ivanbasov modified the milestones: 16.4, 16.5.P1 Nov 9, 2019
@jinujoseph jinujoseph added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 4 - In Review A fix for the issue is submitted for review. labels Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ivanbasov ivanbasov

Labels
Area-IDE Bug Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Milestone
16.5.P1
Development

Successfully merging a pull request may close this issue.

Add CrefSignaturePart to SyntaxFacts.IsInTypeOnlyContext YairHalberstadt/roslyn
5 participants
@JoeRobich @ivanbasov @AlekseyTs @jinujoseph @YairHalberstadt