Workaround another Roslyn MyBase issue - fixes #600

GrahamTheCoder · GrahamTheCoder · commit 26dc64ea974d · 2020-08-04T21:51:15.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 ### VB -> C#
 * Convert parameterized properties with optional parameters [#597](https://github.com/icsharpcode/CodeConverter/issues/597)
 * Convert bitwise negation [#599](https://github.com/icsharpcode/CodeConverter/issues/599)
+* No longer adds incorrect "base" qualification for virtual method calls [#600](https://github.com/icsharpcode/CodeConverter/issues/600)
 
 ### C# -> VB
 
diff --git a/CodeConverter/CSharp/VbNameExpander.cs b/CodeConverter/CSharp/VbNameExpander.cs
@@ -1,6 +1,7 @@
 ﻿using System.Threading;
 using ICSharpCode.CodeConverter.Shared;
 using ICSharpCode.CodeConverter.Util;
+using ICSharpCode.CodeConverter.Util.FromRoslyn;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Operations;
 using Microsoft.CodeAnalysis.Simplification;
@@ -14,40 +15,38 @@ internal class VbNameExpander : ISyntaxExpander
         private static readonly SyntaxToken _dotToken = Microsoft.CodeAnalysis.VisualBasic.SyntaxFactory.Token(Microsoft.CodeAnalysis.VisualBasic.SyntaxKind.DotToken);
         public static ISyntaxExpander Instance { get; } = new VbNameExpander();
 
-        public bool ShouldExpandWithinNode(SyntaxNode node, SyntaxNode root, SemanticModel semanticModel)
-        {
-            return !ShouldExpandNode(node, root, semanticModel) &&
-                   !IsRoslynInstanceExpressionBug(node as MemberAccessExpressionSyntax); ;
-        }
+        public bool ShouldExpandWithinNode(SyntaxNode node, SyntaxNode root, SemanticModel semanticModel) =>
+            !ShouldExpandNode(node, root, semanticModel) && !IsRoslynInstanceExpressionBug(node as MemberAccessExpressionSyntax);
 
-        public bool ShouldExpandNode(SyntaxNode node, SyntaxNode root, SemanticModel semanticModel)
-        {
-            return ShouldExpandName(node) ||
-                   ShouldExpandMemberAccess(node, root, semanticModel);
-        }
+        public bool ShouldExpandNode(SyntaxNode node, SyntaxNode root, SemanticModel semanticModel) =>
+            ShouldExpandName(node) || ShouldExpandMemberAccess(node, root, semanticModel);
 
         private static bool ShouldExpandMemberAccess(SyntaxNode node, SyntaxNode root, SemanticModel semanticModel)
         {
             return node is MemberAccessExpressionSyntax maes && !IsRoslynInstanceExpressionBug(maes) &&
-                   ShouldBeQualified(node, semanticModel.GetSymbolInfo(node).Symbol, semanticModel, root);
+                   ShouldBeQualified(node, semanticModel.GetSymbolInfo(node).Symbol, semanticModel);
         }
 
-        private static bool ShouldExpandName(SyntaxNode node)
-        {
-            return node is NameSyntax && NameCanBeExpanded(node);
-        }
+        private static bool ShouldExpandName(SyntaxNode node) =>
+            node is NameSyntax && NameCanBeExpanded(node);
 
         public SyntaxNode ExpandNode(SyntaxNode node, SyntaxNode root, SemanticModel semanticModel,
             Workspace workspace)
         {
             var symbol = semanticModel.GetSymbolInfo(node).Symbol;
-            if (node is SimpleNameSyntax sns && IsMyBaseBug(node, symbol, root, semanticModel) && semanticModel.GetOperation(node) is IMemberReferenceOperation mro && mro.Instance != null) {
-                var expressionSyntax = (ExpressionSyntax)mro.Instance.Syntax;
-                return MemberAccess(expressionSyntax, sns);
+            if (node is SimpleNameSyntax sns && GetDefaultImplicitInstance(sns, symbol, semanticModel) is {} defaultImplicitInstance) {
+                if (defaultImplicitInstance.InheritsFromOrEquals(symbol.ContainingType)) {
+                    return MemberAccess(SyntaxFactory.MeExpression(), sns);
+                }
+
+                if (semanticModel.GetOperation(node) is IMemberReferenceOperation { Instance: { Syntax: ExpressionSyntax implicitInstance } }) {
+                    return MemberAccess(implicitInstance, sns);
+                }
             }
-            if (node is MemberAccessExpressionSyntax maes && IsTypePromotion(node, symbol, root, semanticModel) && semanticModel.GetOperation(node) is IMemberReferenceOperation mro2 && mro2.Instance != null) {
-                var expressionSyntax = (ExpressionSyntax)mro2.Instance.Syntax;
-                return MemberAccess(expressionSyntax, SyntaxFactory.IdentifierName(mro2.Member.Name));
+
+            if (node is MemberAccessExpressionSyntax && IsTypePromotion(node, symbol, semanticModel) &&
+                semanticModel.GetOperation(node) is IMemberReferenceOperation { Instance: { Syntax: ExpressionSyntax promotedInstance }, Member: {} member }) {
+                return MemberAccess(promotedInstance, SyntaxFactory.IdentifierName(member.Name));
             }
             return IsOriginalSymbolGenericMethod(semanticModel, node) ? node : Simplifier.Expand(node, semanticModel, workspace);
         }
@@ -59,12 +58,23 @@ public SyntaxNode ExpandNode(SyntaxNode node, SyntaxNode root, SemanticModel sem
         /// This leaves the possibility of not qualifying some instance references which didn't contain
         /// </summary>
         private static bool ShouldBeQualified(SyntaxNode node,
-            ISymbol symbol, SemanticModel semanticModel, SyntaxNode root)
+            ISymbol symbol, SemanticModel semanticModel)
         {
-
-            return symbol?.IsStatic == true || IsMyBaseBug(node, symbol, root, semanticModel) || IsTypePromotion(node, symbol, root, semanticModel);
+            return symbol?.IsStatic == true || CouldBeMyBaseBug(node, symbol, semanticModel) || IsTypePromotion(node, symbol, semanticModel);
         }
 
+        /// <summary>Need to workaround roslyn bug that accidentally uses MyBase instead of Me, or implicit instances like Forms</summary>
+        /// See https://github.com/dotnet/roslyn/blob/97123b393c3a5a91cc798b329db0d7fc38634784/src/Workspaces/VisualBasic/Portable/Simplification/VisualBasicSimplificationService.Expander.vb#L657</returns>
+        private static bool CouldBeMyBaseBug(SyntaxNode node, ISymbol symbol, SemanticModel semanticModel) =>
+            node is SimpleNameSyntax sns && IsLeftMostQualifier(sns) && GetDefaultImplicitInstance(sns, symbol, semanticModel) is {};
+
+        private static bool IsLeftMostQualifier(SimpleNameSyntax node) =>
+            node?.Parent switch {
+                MemberAccessExpressionSyntax maes => maes.Expression == node,
+                ConditionalAccessExpressionSyntax caes => caes.Expression == node,
+                _ => true
+            };
+
         private static bool NameCanBeExpanded(SyntaxNode node)
         {
             // Workaround roslyn bug where it will try to expand something even when the parent node cannot contain the type of the expanded node
@@ -74,22 +84,12 @@ private static bool NameCanBeExpanded(SyntaxNode node)
             return true;
         }
 
-        /// <returns>True iff calling Expand would qualify with MyBase when the symbol isn't in the base type
-        /// See https://github.com/dotnet/roslyn/blob/97123b393c3a5a91cc798b329db0d7fc38634784/src/Workspaces/VisualBasic/Portable/Simplification/VisualBasicSimplificationService.Expander.vb#L657</returns>
-        private static bool IsMyBaseBug(SyntaxNode node, ISymbol symbol, SyntaxNode root, SemanticModel semanticModel)
-        {
-            if (IsInstanceReference(symbol) && node is NameSyntax) {
-                return GetEnclosingNamedType(semanticModel, root, node.SpanStart) is ITypeSymbol
-                           implicitQualifyingSymbol &&
-                       !implicitQualifyingSymbol.ContainsMember(symbol);
-            }
-
-            return false;
-        }
+        private static INamedTypeSymbol GetDefaultImplicitInstance(SimpleNameSyntax node, ISymbol symbol, SemanticModel semanticModel, CancellationToken cancellationToken = default) =>
+            IsQualifiableInstanceReference(symbol) && IsLeftMostQualifier(node) ? semanticModel.GetEnclosingSymbol<INamedTypeSymbol>(node.SpanStart, cancellationToken) : null;
 
-        private static bool IsTypePromotion(SyntaxNode node, ISymbol symbol, SyntaxNode root, SemanticModel semanticModel)
+        private static bool IsTypePromotion(SyntaxNode node, ISymbol symbol, SemanticModel semanticModel)
         {
-            if (IsInstanceReference(symbol) && node is MemberAccessExpressionSyntax maes && maes.Expression != null) {
+            if (IsQualifiableInstanceReference(symbol) && node is MemberAccessExpressionSyntax maes && maes.Expression != null) {
                 var qualifyingType = semanticModel.GetTypeInfo(maes.Expression).Type;
                 return qualifyingType == null || !qualifyingType.ContainsMember(symbol);
             }
@@ -100,56 +100,21 @@ private static bool IsTypePromotion(SyntaxNode node, ISymbol symbol, SyntaxNode
         /// <summary>
         /// Roslyn bug - accidentally expands "New" into an identifier causing compile error
         /// </summary>
-        public static bool IsRoslynInstanceExpressionBug(MemberAccessExpressionSyntax node)
-        {
-            return node?.Expression is InstanceExpressionSyntax;
-        }
+        public static bool IsRoslynInstanceExpressionBug(MemberAccessExpressionSyntax node) =>
+            node?.Expression is InstanceExpressionSyntax;
 
         /// <summary>
         /// Roslyn bug - accidentally expands anonymous types to just "Global."
         /// Since the C# reducer also doesn't seem to reduce generic extension methods, it's best to avoid those too, so let's just avoid all generic methods
         /// </summary>
-        private static bool IsOriginalSymbolGenericMethod(SemanticModel semanticModel, SyntaxNode node)
-        {
-            return semanticModel.GetSymbolInfo(node).Symbol.IsGenericMethod();
-        }
+        private static bool IsOriginalSymbolGenericMethod(SemanticModel semanticModel, SyntaxNode node) =>
+            semanticModel.GetSymbolInfo(node).Symbol.IsGenericMethod();
 
-        private static bool IsInstanceReference(ISymbol symbol)
-        {
-            return symbol?.IsStatic == false && (symbol.Kind == SymbolKind.Method || symbol.Kind ==
-                                                 SymbolKind.Field || symbol.Kind == SymbolKind.Property);
-        }
-
-        private static MemberAccessExpressionSyntax MemberAccess(ExpressionSyntax expressionSyntax, SimpleNameSyntax sns)
-        {
-            return SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression,
-                expressionSyntax,
-                _dotToken,
-                sns);
-        }
+        private static bool IsQualifiableInstanceReference(ISymbol symbol) =>
+            symbol?.IsStatic == false && (symbol.IsKind(SymbolKind.Method) || symbol.IsKind(SymbolKind.Field) ||
+                                          symbol.IsKind(SymbolKind.Property));
 
-        /// <summary>
-        /// Pasted from AbstractGenerateFromMembersCodeRefactoringProvider
-        /// Gets the enclosing named type for the specified position.  We can't use
-        /// <see cref="SemanticModel.GetEnclosingSymbol"/> because that doesn't return
-        /// the type you're current on if you're on the header of a class/interface.
-        /// </summary>
-        private static INamedTypeSymbol GetEnclosingNamedType(
-            SemanticModel semanticModel, SyntaxNode root, int start,
-            CancellationToken cancellationToken = default(CancellationToken))
-        {
-            var token = root.FindToken(start);
-            if (token == ((ICompilationUnitSyntax)root).EndOfFileToken) {
-                token = token.GetPreviousToken();
-            }
-
-            for (var node = token.Parent; node != null; node = node.Parent) {
-                if (semanticModel.GetDeclaredSymbol(node) is INamedTypeSymbol declaration) {
-                    return declaration;
-                }
-            }
-
-            return null;
-        }
+        private static MemberAccessExpressionSyntax MemberAccess(ExpressionSyntax expressionSyntax, SimpleNameSyntax sns) =>
+            SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, expressionSyntax, _dotToken, sns);
     }
 }
diff --git a/CodeConverter/Shared/DocumentExtensions.cs b/CodeConverter/Shared/DocumentExtensions.cs
@@ -82,7 +82,7 @@ private static async Task<Document> ExpandAsync(Document document, ISyntaxExpand
                 );
                 return document.WithSyntaxRoot(newRoot);
             } catch (Exception ex) {
-                var warningText = "Conversion warning: Qualified name reduction failed for this file. " + ex;
+                var warningText = "Conversion warning: Name qualification failed for this file. " + ex;
                 return document.WithSyntaxRoot(WithWarningAnnotation(root, warningText));
             }
         }
diff --git a/Tests/CSharp/NamespaceLevelTests.cs b/Tests/CSharp/NamespaceLevelTests.cs
@@ -982,5 +982,79 @@ public void TestMethod()
 1 source compilation errors:
 BC30614: 'MustOverride' method 'Public MustOverride Property P2 As Integer' cannot be called with 'MyClass'.");
         }
+
+        [Fact]
+        public async Task OverridenMemberCallAsync()
+        {
+            await TestConversionVisualBasicToCSharpAsync(@"
+Module Module1
+    Public Class BaseImpl
+        Protected Overridable Function GetImplName() As String
+            Return NameOf(BaseImpl)
+        End Function
+    End Class
+
+    ''' <summary>
+    ''' The fact that this class doesn't contain a definition for GetImplName is crucial to the repro
+    ''' </summary>
+    Public Class ErrorSite
+        Inherits BaseImpl
+        Public Function PublicGetImplName()
+            ' This must not be qualified with MyBase since the method is overridable
+            Return GetImplName()
+        End Function
+    End Class
+
+    Public Class OverrideImpl
+        Inherits ErrorSite
+        Protected Overrides Function GetImplName() As String
+            Return NameOf(OverrideImpl)
+        End Function
+    End Class
+
+    Sub Main()
+        Dim c As New OverrideImpl
+        Console.WriteLine(c.PublicGetImplName())
+    End Sub
+End Module",
+                @"using System;
+
+internal static partial class Module1
+{
+    public partial class BaseImpl
+    {
+        protected virtual string GetImplName()
+        {
+            return nameof(BaseImpl);
+        }
+    }
+
+    /// <summary>
+    /// The fact that this class doesn't contain a definition for GetImplName is crucial to the repro
+    /// </summary>
+    public partial class ErrorSite : BaseImpl
+    {
+        public object PublicGetImplName()
+        {
+            // This must not be qualified with MyBase since the method is overridable
+            return GetImplName();
+        }
+    }
+
+    public partial class OverrideImpl : ErrorSite
+    {
+        protected override string GetImplName()
+        {
+            return nameof(OverrideImpl);
+        }
+    }
+
+    public static void Main()
+    {
+        var c = new OverrideImpl();
+        Console.WriteLine(c.PublicGetImplName());
+    }
+}");
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,7 @@ private static async Task<Document> ExpandAsync(Document document, ISyntaxExpand`
`82`	`82`	`);`
`83`	`83`	`return document.WithSyntaxRoot(newRoot);`
`84`	`84`	`} catch (Exception ex) {`
`85`		`- var warningText = "Conversion warning: Qualified name reduction failed for this file. " + ex;`
	`85`	`+ var warningText = "Conversion warning: Name qualification failed for this file. " + ex;`
`86`	`86`	`return document.WithSyntaxRoot(WithWarningAnnotation(root, warningText));`
`87`	`87`	`}`
`88`	`88`	`}`