Don't generate unnecessary properties for WithEvents - fixes #572

GrahamTheCoder · GrahamTheCoder · commit 26bc88e32b2b · 2020-08-08T14:07:24.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 * 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)
+* Don't generate unnecessary properties for WithEvents fields [#572](https://github.com/icsharpcode/CodeConverter/issues/572)
 
 ### C# -> VB
 
diff --git a/CodeConverter/CSharp/CommonConversions.cs b/CodeConverter/CSharp/CommonConversions.cs
@@ -45,6 +45,7 @@ internal class CommonConversions
 
         private readonly CSharpCompilation _csCompilation;
         private readonly ITypeContext _typeContext;
+        public WinformsConversions WinformsConversions { get; }
 
         public CommentConvertingVisitorWrapper TriviaConvertingExpressionVisitor { get; set; }
         public TypeConversionAnalyzer TypeConversionAnalyzer { get; }
@@ -60,6 +61,7 @@ public CommonConversions(Document document, SemanticModel semanticModel,
             _csCompilation = csCompilation;
             _typeContext = typeContext;
             VisualBasicEqualityComparison = visualBasicEqualityComparison;
+            WinformsConversions = new WinformsConversions(typeContext);
         }
 
         public async Task<(IReadOnlyCollection<(VariableDeclarationSyntax Decl, ITypeSymbol Type)> Variables, IReadOnlyCollection<CSharpSyntaxNode> Methods)> SplitVariableDeclarationsAsync(
diff --git a/CodeConverter/CSharp/DeclarationNodeVisitor.cs b/CodeConverter/CSharp/DeclarationNodeVisitor.cs
@@ -216,7 +216,7 @@ private async Task<IEnumerable<MemberDeclarationSyntax>> ConvertMembersAsync(VBS
                     directlyConvertedMembers = directlyConvertedMembers
                         .Concat(methodsWithHandles.CreateDelegatingMethodsRequiredByInitializeComponent());
                     additionalInitializers.AdditionalInstanceInitializers
-                        .AddRange(WinformsConversions.GetNameAssignments(otherPartsOfType));
+                        .AddRange(CommonConversions.WinformsConversions.GetNameAssignments(methodsWithHandles, otherPartsOfType));
                 } else {
                     additionalInitializers.AdditionalInstanceInitializers.AddRange(methodsWithHandles.GetConstructorEventHandlers());
                 }
diff --git a/CodeConverter/CSharp/ExpressionNodeVisitor.cs b/CodeConverter/CSharp/ExpressionNodeVisitor.cs
@@ -269,7 +269,7 @@ public override async Task<CSharpSyntaxNode> VisitLiteralExpression(VBasic.Synta
 
             var val = node.Token.Value;
             var text = node.Token.Text;
-            if (WinformsConversions.ShouldPrefixAssignedNameWithUnderscore(node.Parent as VBSyntax.AssignmentStatementSyntax) && val is string valStr) {
+            if (_typeContext.Any() && CommonConversions.WinformsConversions.ShouldPrefixAssignedNameWithUnderscore(node.Parent as VBSyntax.AssignmentStatementSyntax, _typeContext.MethodsWithHandles) && val is string valStr) {
                 val = "_" + valStr;
                 text = "\"_" + valStr + "\"";
             }
diff --git a/CodeConverter/CSharp/ITypeContext.cs b/CodeConverter/CSharp/ITypeContext.cs
@@ -5,5 +5,6 @@ internal interface ITypeContext
         AdditionalInitializers Initializers { get; }
         MethodsWithHandles MethodsWithHandles { get; }
         HoistedNodeState HoistedState { get; }
+        bool Any();
     }
 }
diff --git a/CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs b/CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs
@@ -197,7 +197,19 @@ private async Task<SyntaxList<StatementSyntax>> ConvertMidAssignmentAsync(VBSynt
         private SyntaxList<StatementSyntax> GetPostAssignmentStatements(VBSyntax.AssignmentStatementSyntax node)
         {
             var potentialPropertySymbol = _semanticModel.GetSymbolInfo(node.Left).ExtractBestMatch<ISymbol>();
-            return _methodsWithHandles.GetPostAssignmentStatements(node, potentialPropertySymbol);
+            return GetPostAssignmentStatements(node, potentialPropertySymbol);
+        }
+
+        /// <summary>
+        /// Make winforms designer work: https://github.com/icsharpcode/CodeConverter/issues/321
+        /// </summary>
+        public SyntaxList<StatementSyntax> GetPostAssignmentStatements(Microsoft.CodeAnalysis.VisualBasic.Syntax.AssignmentStatementSyntax node, ISymbol potentialPropertySymbol)
+        {
+            if (CommonConversions.WinformsConversions.MustInlinePropertyWithEventsAccess(node, potentialPropertySymbol)) {
+                return _methodsWithHandles.GetPostAssignmentStatements(potentialPropertySymbol);
+            }
+
+            return SyntaxFactory.List<StatementSyntax>();
         }
 
         public override async Task<SyntaxList<StatementSyntax>> VisitEraseStatement(VBSyntax.EraseStatementSyntax node)
diff --git a/CodeConverter/CSharp/MethodsWithHandles.cs b/CodeConverter/CSharp/MethodsWithHandles.cs
@@ -1,5 +1,6 @@
 ﻿using System;
 using System.Collections.Generic;
+using System.Collections.Immutable;
 using System.Linq;
 using ICSharpCode.CodeConverter.Util;
 using Microsoft.CodeAnalysis;
@@ -15,47 +16,47 @@ internal class MethodsWithHandles
     {
         private readonly List<MethodWithHandles> _methodWithHandleses;
         private readonly ILookup<string, MethodWithHandles> _handledMethodsFromPropertyWithEventName;
+        private readonly ImmutableHashSet<string> _containerFieldsConvertedToProperties;
 
 
-        public MethodsWithHandles(List<MethodWithHandles> methodWithHandleses, ILookup<string, MethodWithHandles> handledMethodsFromPropertyWithEventName)
+        private MethodsWithHandles(List<MethodWithHandles> methodWithHandleses,
+            ILookup<string, MethodWithHandles> handledMethodsFromPropertyWithEventName,
+            ImmutableHashSet<string> containerFieldsConvertedToProperties)
         {
             _methodWithHandleses = methodWithHandleses;
             _handledMethodsFromPropertyWithEventName = handledMethodsFromPropertyWithEventName;
+            _containerFieldsConvertedToProperties = containerFieldsConvertedToProperties;
         }
 
         public static MethodsWithHandles Create(List<MethodWithHandles> methodWithHandleses)
         {
             var handledMethodsFromPropertyWithEventName = methodWithHandleses
-                .SelectMany(m => m.HandledPropertyEventCSharpIds.Select(h => (EventPropertyName: h.Item1.Text, MethodWithHandles: m)))
+                .SelectMany(m => m.HandledPropertyEventCSharpIds.Select(h => (EventPropertyName: h.EventContainerName.Text, MethodWithHandles: m)))
                 .ToLookup(m => m.EventPropertyName, m => m.MethodWithHandles);
-            return new MethodsWithHandles(methodWithHandleses, handledMethodsFromPropertyWithEventName);
+            var containerFieldsConvertedToProperties = methodWithHandleses
+                .SelectMany(m => m.HandledPropertyEventCSharpIds, (_, handled) => handled.EventContainerName.Text)
+                .ToImmutableHashSet(StringComparer.OrdinalIgnoreCase);
+            return new MethodsWithHandles(methodWithHandleses, handledMethodsFromPropertyWithEventName, containerFieldsConvertedToProperties);
         }
 
-        public bool Any()
-        {
-            return _methodWithHandleses.Any();
-        }
+        public bool Any() => _methodWithHandleses.Any();
+        public bool AnyForPropertyName(string propertyIdentifierText) => _containerFieldsConvertedToProperties.Contains(propertyIdentifierText);
 
         public IEnumerable<MemberDeclarationSyntax> GetDeclarationsForFieldBackedProperty(VariableDeclarationSyntax fieldDecl, SyntaxTokenList convertedModifiers, SyntaxList<AttributeListSyntax> attributes)
         {
             return MethodWithHandles.GetDeclarationsForFieldBackedProperty(fieldDecl, convertedModifiers, attributes,
                 _methodWithHandleses);
         }
 
-
-        /// <summary>
-        /// Make winforms designer work: https://github.com/icsharpcode/CodeConverter/issues/321
-        /// </summary>
-        public SyntaxList<StatementSyntax> GetPostAssignmentStatements(Microsoft.CodeAnalysis.VisualBasic.Syntax.AssignmentStatementSyntax node, ISymbol potentialPropertySymbol)
+        public SyntaxList<StatementSyntax> GetPostAssignmentStatements(ISymbol potentialPropertySymbol)
         {
-            if (WinformsConversions.MustInlinePropertyWithEventsAccess(node, potentialPropertySymbol))
+            var fieldName = SyntaxFactory.IdentifierName("_" + potentialPropertySymbol.Name);
+            var handledMethods = _handledMethodsFromPropertyWithEventName[potentialPropertySymbol.Name].ToArray();
+            if (handledMethods.Any())
             {
-                var fieldName = SyntaxFactory.IdentifierName("_" + potentialPropertySymbol.Name);
-                var handledMethods = _handledMethodsFromPropertyWithEventName[potentialPropertySymbol.Name].ToArray();
-                if (handledMethods.Any())
+                var postAssignmentStatements = handledMethods.SelectMany(h =>
+                    h.GetPostInitializationStatements(potentialPropertySymbol.Name, fieldName));
                 {
-                    var postAssignmentStatements = handledMethods.SelectMany(h =>
-                        h.GetPostInitializationStatements(potentialPropertySymbol.Name, fieldName));
                     return SyntaxFactory.List(postAssignmentStatements);
                 }
             }
diff --git a/CodeConverter/CSharp/TypeContext.cs b/CodeConverter/CSharp/TypeContext.cs
@@ -17,5 +17,6 @@ public void Push(MethodsWithHandles methodWithHandles, AdditionalInitializers ad
         }
 
         public void Pop() => _contextStack.Pop();
+        public bool Any() => _contextStack.Count > 0;
     }
 }
diff --git a/CodeConverter/CSharp/WinformsConversions.cs b/CodeConverter/CSharp/WinformsConversions.cs
@@ -9,15 +9,22 @@
 
 namespace ICSharpCode.CodeConverter.CSharp
 {
-    internal static class WinformsConversions
+    internal class WinformsConversions
     {
+        private readonly ITypeContext _typeContext;
+
+        public WinformsConversions(ITypeContext typeContext)
+        {
+            _typeContext = typeContext;
+        }
+
         /// <remarks>
         /// Co-ordinates inlining property events, see <see cref="MethodBodyExecutableStatementVisitor.GetPostAssignmentStatements"/>
         /// Also see usages of IsDesignerGeneratedTypeWithInitializeComponent
         /// </remarks>
-        public static bool MustInlinePropertyWithEventsAccess(SyntaxNode anyNodePossiblyWithinMethod, ISymbol potentialPropertySymbol)
+        public bool MustInlinePropertyWithEventsAccess(SyntaxNode anyNodePossiblyWithinMethod, ISymbol potentialPropertySymbol)
         {
-            return InMethodCalledInitializeComponent(anyNodePossiblyWithinMethod) && potentialPropertySymbol is IPropertySymbol prop && prop.IsWithEvents;
+            return potentialPropertySymbol != null &&_typeContext.Any() && _typeContext.MethodsWithHandles.AnyForPropertyName(potentialPropertySymbol.Name) && InMethodCalledInitializeComponent(anyNodePossiblyWithinMethod) && potentialPropertySymbol is IPropertySymbol prop && prop.IsWithEvents;
         }
 
         public static bool InMethodCalledInitializeComponent(SyntaxNode anyNodePossiblyWithinMethod)
@@ -34,33 +41,32 @@ private static bool IsInitializeComponent(VBSyntax.MethodBlockSyntax methodBlock
         /// <summary>
         /// We replace a field with a property to handle event subscription, so need to update the name so the winforms designer regenerates the file correctly in future
         /// </summary>
-        /// <returns></returns>
-        public static bool ShouldPrefixAssignedNameWithUnderscore(VBSyntax.StatementSyntax statementOrNull)
+        public bool ShouldPrefixAssignedNameWithUnderscore(VBSyntax.StatementSyntax statementOrNull, MethodsWithHandles typeContextMethodsWithHandles)
         {
             return statementOrNull is VBSyntax.AssignmentStatementSyntax assignment && InMethodCalledInitializeComponent(assignment) &&
                             assignment.Left is VBSyntax.MemberAccessExpressionSyntax maes &&
                                 !(maes.Expression is VBSyntax.MeExpressionSyntax) &&
-                            maes.Name.ToString() == "Name";
-        }
-
-        private static T LastOrDefaultDescendant<T>(this VBasic.VisualBasicSyntaxNode syntaxNode) {
-            return syntaxNode.DescendantNodes().OfType<T>().LastOrDefault();
+                            maes.Name.ToString() == "Name" && 
+                            maes.Expression.LastOrDefaultDescendant<VBSyntax.IdentifierNameSyntax>()?.Identifier.Text is {} propName &&
+                            typeContextMethodsWithHandles.AnyForPropertyName(propName);
         }
 
-        internal static IEnumerable<Assignment> GetNameAssignments((VBSyntax.TypeBlockSyntax Type, SemanticModel SemanticModel)[] otherPartsOfType)
+        public IEnumerable<Assignment> GetNameAssignments(MethodsWithHandles typeContextMethodsWithHandles,
+            (VBSyntax.TypeBlockSyntax Type, SemanticModel SemanticModel)[] otherPartsOfType)
         {
             return otherPartsOfType.SelectMany(typePart => 
                 typePart.Type.Members.OfType<VBSyntax.MethodBlockSyntax>()
                     .Where(IsInitializeComponent)
-                    .SelectMany(GetAssignments)
+                    .SelectMany(syntax => GetAssignments(typeContextMethodsWithHandles, syntax))
             );
         }
 
-        private static IEnumerable<Assignment> GetAssignments(VBSyntax.MethodBlockSyntax initializeComponent)
+        private IEnumerable<Assignment> GetAssignments(MethodsWithHandles typeContextMethodsWithHandles,
+            VBSyntax.MethodBlockSyntax initializeComponent)
         {
             return initializeComponent.Statements
                 .OfType<VBSyntax.AssignmentStatementSyntax>()
-                .Where(ShouldPrefixAssignedNameWithUnderscore)
+                .Where(syntax => ShouldPrefixAssignedNameWithUnderscore(syntax, typeContextMethodsWithHandles))
                 .Select(s => (s.Left as VBSyntax.MemberAccessExpressionSyntax)?.Expression.LastOrDefaultDescendant<VBSyntax.IdentifierNameSyntax>())
                 .Where(s => s != null)
                 .Select(id => {
diff --git a/CodeConverter/Util/SyntaxNodeExtensions.cs b/CodeConverter/Util/SyntaxNodeExtensions.cs
@@ -516,5 +516,9 @@ public static SyntaxToken FindNonZeroWidthToken(this SyntaxNode node, int positi
                 return syntaxToken;
             }
         }
+
+        public static T LastOrDefaultDescendant<T>(this SyntaxNode syntaxNode) {
+            return syntaxNode.DescendantNodes().OfType<T>().LastOrDefault();
+        }
     }
 }
diff --git a/Tests/TestData/MultiFileCharacterization/VBToCSResults/ConvertWholeSolution/WindowsAppVb/Folder/FolderForm.Designer.cs b/Tests/TestData/MultiFileCharacterization/VBToCSResults/ConvertWholeSolution/WindowsAppVb/Folder/FolderForm.Designer.cs
diff --git a/Tests/TestData/MultiFileCharacterization/VBToCSResults/ConvertWholeSolution/WindowsAppVb/Folder/FolderForm.cs b/Tests/TestData/MultiFileCharacterization/VBToCSResults/ConvertWholeSolution/WindowsAppVb/Folder/FolderForm.cs

Original file line number	Diff line number	Diff line change
`@@ -216,7 +216,7 @@ private async Task<IEnumerable<MemberDeclarationSyntax>> ConvertMembersAsync(VBS`
`216`	`216`	`directlyConvertedMembers = directlyConvertedMembers`
`217`	`217`	`.Concat(methodsWithHandles.CreateDelegatingMethodsRequiredByInitializeComponent());`
`218`	`218`	`additionalInitializers.AdditionalInstanceInitializers`
`219`		`- .AddRange(WinformsConversions.GetNameAssignments(otherPartsOfType));`
	`219`	`+ .AddRange(CommonConversions.WinformsConversions.GetNameAssignments(methodsWithHandles, otherPartsOfType));`
`220`	`220`	`} else {`
`221`	`221`	`additionalInitializers.AdditionalInstanceInitializers.AddRange(methodsWithHandles.GetConstructorEventHandlers());`
`222`	`222`	`}`
Original file line number	Diff line number	Diff line change
`@@ -269,7 +269,7 @@ public override async Task<CSharpSyntaxNode> VisitLiteralExpression(VBasic.Synta`
`269`	`269`
`270`	`270`	`var val = node.Token.Value;`
`271`	`271`	`var text = node.Token.Text;`
`272`		`- if (WinformsConversions.ShouldPrefixAssignedNameWithUnderscore(node.Parent as VBSyntax.AssignmentStatementSyntax) && val is string valStr) {`
	`272`	`+ if (_typeContext.Any() && CommonConversions.WinformsConversions.ShouldPrefixAssignedNameWithUnderscore(node.Parent as VBSyntax.AssignmentStatementSyntax, _typeContext.MethodsWithHandles) && val is string valStr) {`
`273`	`273`	`val = "_" + valStr;`
`274`	`274`	`text = "\"_" + valStr + "\"";`
`275`	`275`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,5 +5,6 @@ internal interface ITypeContext`
`5`	`5`	`AdditionalInitializers Initializers { get; }`
`6`	`6`	`MethodsWithHandles MethodsWithHandles { get; }`
`7`	`7`	`HoistedNodeState HoistedState { get; }`
	`8`	`+ bool Any();`
`8`	`9`	`}`
`9`	`10`	`}`
Original file line number	Diff line number	Diff line change
`@@ -17,5 +17,6 @@ public void Push(MethodsWithHandles methodWithHandles, AdditionalInitializers ad`
`17`	`17`	`}`
`18`	`18`
`19`	`19`	`public void Pop() => _contextStack.Pop();`
	`20`	`+ public bool Any() => _contextStack.Count > 0;`
`20`	`21`	`}`
`21`	`22`	`}`
Original file line number	Diff line number	Diff line change
`@@ -516,5 +516,9 @@ public static SyntaxToken FindNonZeroWidthToken(this SyntaxNode node, int positi`
`516`	`516`	`return syntaxToken;`
`517`	`517`	`}`
`518`	`518`	`}`
	`519`	`+`
	`520`	`+ public static T LastOrDefaultDescendant<T>(this SyntaxNode syntaxNode) {`
	`521`	`+ return syntaxNode.DescendantNodes().OfType<T>().LastOrDefault();`
	`522`	`+ }`
`519`	`523`	`}`
`520`	`524`	`}`