Merge pull request #40 from edespong/Issue8_ConcatOfOptimizedValueTypes

Do not report boxing for optimized value types (#8)
microsoft · Mar 26, 2018 · 77d965d · 77d965d
2 parents 1bbe15d + 9fbab1c
commit 77d965d
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 28 deletions.
diff --git a/ClrHeapAllocationsAnalyzer.Test/ConcatenationAllocationAnalyzerTests.cs b/ClrHeapAllocationsAnalyzer.Test/ConcatenationAllocationAnalyzerTests.cs
@@ -19,10 +19,26 @@ public void ConcatenationAllocation_Basic() {
 
             Assert.AreEqual(0, info0.Allocations.Count(d => d.Id == ConcatenationAllocationAnalyzer.StringConcatenationAllocationRule.Id));
             Assert.AreEqual(1, info1.Allocations.Count(d => d.Id == ConcatenationAllocationAnalyzer.StringConcatenationAllocationRule.Id));
-
             AssertEx.ContainsDiagnostic(info1.Allocations, id: ConcatenationAllocationAnalyzer.StringConcatenationAllocationRule.Id, line: 1, character: 13);
         }
 
+        [TestMethod]
+        public void ConcatenationAllocation_DoNotWarnForOptimizedValueTypes() {
+            var snippets = new[]
+            {
+                @"string s0 = nameof(System.String) + '-';",
+                @"string s0 = nameof(System.String) + true;",
+                @"string s0 = nameof(System.String) + new System.IntPtr();",
+                @"string s0 = nameof(System.String) + new System.UIntPtr();"
+            };
+
+            var analyser = new ConcatenationAllocationAnalyzer();
+            foreach (var snippet in snippets) {
+                var info = ProcessCode(analyser, snippet, ImmutableArray.Create(SyntaxKind.AddExpression, SyntaxKind.AddAssignmentExpression));
+                Assert.AreEqual(0, info.Allocations.Count(x => x.Id == ConcatenationAllocationAnalyzer.ValueTypeToReferenceTypeInAStringConcatenationRule.Id));
+            }
+        }
+
         [TestMethod]
         public void ConcatenationAllocation_DoNotWarnForConst() {
             var snippets = new[]

diff --git a/ClrHeapAllocationsAnalyzer.Test/StackOverflowAnswerTests.cs b/ClrHeapAllocationsAnalyzer.Test/StackOverflowAnswerTests.cs
@@ -78,7 +78,7 @@ struct S : I { }
         public void Non_constant_value_types_in_CSharp_string_concatenation()
         {
             var @script =
-                @"char c = 'c'; //F();
+                @"System.DateTime c = System.DateTime.Now;;
                 string s1 = ""char value will box"" + c;";
             var analyser = new ConcatenationAllocationAnalyzer();
             var info = ProcessCode(analyser, @script, ImmutableArray.Create(SyntaxKind.AddExpression, SyntaxKind.AddAssignmentExpression));

diff --git a/ClrHeapAllocationsAnalyzer.Test/TypeConversionAllocationAnalyzerTests.cs b/ClrHeapAllocationsAnalyzer.Test/TypeConversionAllocationAnalyzerTests.cs
@@ -1,4 +1,5 @@
-using ClrHeapAllocationAnalyzer;
+using System;
+using ClrHeapAllocationAnalyzer;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using System.Collections.Immutable;
@@ -433,9 +434,9 @@ public static implicit operator string(AStruct astruct)
 
             var info0 = ProcessCode(analyzer, programWithoutImplicitCastOperator, ImmutableArray.Create(SyntaxKind.Argument));
             AssertEx.ContainsDiagnostic(info0.Allocations, id: TypeConversionAllocationAnalyzer.ValueTypeToReferenceTypeConversionRule.Id, line: 6, character: 50);
-
+            
             var info1 = ProcessCode(analyzer, programWithImplicitCastOperator, ImmutableArray.Create(SyntaxKind.Argument));
-            Assert.AreEqual(0, info1.Allocations.Count);
+            Assert.AreEqual(0, info1.Allocations.Count, info1.Allocations[0].Id);
         }
 
 

diff --git a/ClrHeapAllocationsAnalyzer/ConcatenationAllocationAnalyzer.cs b/ClrHeapAllocationsAnalyzer/ConcatenationAllocationAnalyzer.cs
@@ -1,5 +1,4 @@
-namespace ClrHeapAllocationAnalyzer
-{
+namespace ClrHeapAllocationAnalyzer {
     using System;
     using System.Collections.Immutable;
     using System.Linq;
@@ -9,8 +8,7 @@
     using Microsoft.CodeAnalysis.Diagnostics;
 
     [DiagnosticAnalyzer(LanguageNames.CSharp)]
-    public sealed class ConcatenationAllocationAnalyzer : DiagnosticAnalyzer
-    {
+    public sealed class ConcatenationAllocationAnalyzer : DiagnosticAnalyzer {
         public static DiagnosticDescriptor StringConcatenationAllocationRule = new DiagnosticDescriptor("HAA0201", "Implicit string concatenation allocation", "Considering using StringBuilder", "Performance", DiagnosticSeverity.Warning, true, string.Empty, "http://msdn.microsoft.com/en-us/library/2839d5h5(v=vs.110).aspx");
 
         public static DiagnosticDescriptor ValueTypeToReferenceTypeInAStringConcatenationRule = new DiagnosticDescriptor("HAA0202", "Value type to reference type conversion allocation for string concatenation", "Value type ({0}) is being boxed to a reference type for a string concatenation.", "Performance", DiagnosticSeverity.Warning, true, string.Empty, "http://msdn.microsoft.com/en-us/library/yz2be5wk.aspx");
@@ -19,13 +17,11 @@ public sealed class ConcatenationAllocationAnalyzer : DiagnosticAnalyzer
 
         public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(StringConcatenationAllocationRule, ValueTypeToReferenceTypeInAStringConcatenationRule);
 
-        public override void Initialize(AnalysisContext context)
-        {
+        public override void Initialize(AnalysisContext context) {
             context.RegisterSyntaxNodeAction(AnalyzeNode, SyntaxKind.AddExpression, SyntaxKind.AddAssignmentExpression);
         }
 
-        private static void AnalyzeNode(SyntaxNodeAnalysisContext context)
-        {
+        private static void AnalyzeNode(SyntaxNodeAnalysisContext context) {
             var node = context.Node;
             var semanticModel = context.SemanticModel;
             Action<Diagnostic> reportDiagnostic = context.ReportDiagnostic;
@@ -34,28 +30,26 @@ private static void AnalyzeNode(SyntaxNodeAnalysisContext context)
             var binaryExpressions = node.DescendantNodesAndSelf().OfType<BinaryExpressionSyntax>().Reverse(); // need inner most expressions
 
             int stringConcatenationCount = 0;
-            foreach (var binaryExpression in binaryExpressions)
-            {
-                if (binaryExpression.Left == null || binaryExpression.Right == null)
-                {
+            foreach (var binaryExpression in binaryExpressions) {
+                if (binaryExpression.Left == null || binaryExpression.Right == null) {
                     continue;
                 }
 
                 bool isConstant = semanticModel.GetConstantValue(binaryExpression, cancellationToken).HasValue;
-                if (isConstant)
-                {
+                if (isConstant) {
                     continue;
                 }
 
                 var left = semanticModel.GetTypeInfo(binaryExpression.Left, cancellationToken);
-                CheckForTypeConversion(binaryExpression.Left, left, reportDiagnostic, filePath);
+                var leftConversion = semanticModel.GetConversion(binaryExpression.Left, cancellationToken);
+                CheckTypeConversion(left, leftConversion, reportDiagnostic, binaryExpression.Left.GetLocation(), filePath);
 
                 var right = semanticModel.GetTypeInfo(binaryExpression.Right, cancellationToken);
-                CheckForTypeConversion(binaryExpression.Right, right, reportDiagnostic, filePath);
+                var rightConversion = semanticModel.GetConversion(binaryExpression.Right, cancellationToken);
+                CheckTypeConversion(right, rightConversion, reportDiagnostic, binaryExpression.Right.GetLocation(), filePath);
 
                 // regular string allocation
-                if (left.Type?.SpecialType == SpecialType.System_String || right.Type?.SpecialType == SpecialType.System_String)
-                {
+                if (left.Type?.SpecialType == SpecialType.System_String || right.Type?.SpecialType == SpecialType.System_String) {
                       stringConcatenationCount++;   
                 }
             }
@@ -67,11 +61,16 @@ private static void AnalyzeNode(SyntaxNodeAnalysisContext context)
             }
         }
 
-        private static void CheckForTypeConversion(ExpressionSyntax expression, TypeInfo typeInfo, Action<Diagnostic> reportDiagnostic, string filePath)
-        {
-            if (typeInfo.Type != null && typeInfo.Type.IsValueType && typeInfo.ConvertedType != null && !typeInfo.ConvertedType.IsValueType)
-            {
-                reportDiagnostic(Diagnostic.Create(ValueTypeToReferenceTypeInAStringConcatenationRule, expression.GetLocation(), new object[] { typeInfo.Type.ToDisplayString() }));
+        private static void CheckTypeConversion(TypeInfo typeInfo, Conversion conversionInfo, Action<Diagnostic> reportDiagnostic, Location location, string filePath) {
+            bool IsOptimizedValueType(ITypeSymbol type) {
+                return type.SpecialType == SpecialType.System_Boolean ||
+                       type.SpecialType == SpecialType.System_Char ||
+                       type.SpecialType == SpecialType.System_IntPtr ||
+                       type.SpecialType == SpecialType.System_UIntPtr;
+            }
+
+            if (conversionInfo.IsBoxing && !IsOptimizedValueType(typeInfo.Type)) {
+                reportDiagnostic(Diagnostic.Create(ValueTypeToReferenceTypeInAStringConcatenationRule, location, new[] { typeInfo.Type.ToDisplayString() }));
                 HeapAllocationAnalyzerEventSource.Logger.BoxingAllocationInStringConcatenation(filePath);
             }
         }