Added check to allow Array/Enumerable.Empty calls as immutable. (#732)

Added supporting test cases.
Brightspace · Mar 12, 2021 · f8251f3 · f8251f3
1 parent 7e9cde3
commit f8251f3
Show file tree

Hide file tree

Showing 4 changed files with 127 additions and 38 deletions.
diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.Factory.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.Factory.cs
@@ -1,4 +1,5 @@
 using System.Collections.Immutable;
+using System.Linq;
 using Microsoft.CodeAnalysis;
 
 namespace D2L.CodeStyle.Analyzers.Immutability {
@@ -77,24 +78,24 @@ internal partial class ImmutabilityContext {
 			("System.IO.Abstractions.FileSystem", default)
 		);
 
+
+		internal static readonly ImmutableArray<(string TypeName, string MethodName, string AssemblyName)> KnownImmutableReturningMethods = ImmutableArray.Create(
+			( "System.Array", "Empty", default(string) ),
+			( "System.Linq.Enumerable", "Empty", default(string) )
+		);
+
+
 		internal static ImmutabilityContext Create( Compilation compilation, AnnotationsContext annotationsContext ) {
-			ImmutableDictionary<string, IAssemblySymbol> compilationAssmeblies = GetCompilationAssemblies( compilation );
 
-			var builder = ImmutableDictionary.CreateBuilder<INamedTypeSymbol, ImmutableTypeInfo>();
+			ImmutableDictionary<string, IAssemblySymbol> compilationAssemblies = GetCompilationAssemblies( compilation );
 
+			// Generate a dictionary of types that we have specifically determined
+			// should be considered Immutable by the Analyzer.
+			var extraImmutableTypesBuilder = ImmutableDictionary.CreateBuilder<INamedTypeSymbol, ImmutableTypeInfo>();
 			foreach( ( string typeName, string qualifiedAssembly ) in DefaultExtraTypes ) {
-				INamedTypeSymbol type;
-				if( string.IsNullOrEmpty( qualifiedAssembly ) ) {
-					type = compilation.GetTypeByMetadataName( typeName );
-				} else {
-					if( !compilationAssmeblies.TryGetValue( qualifiedAssembly, out IAssemblySymbol assembly ) ) {
-						continue;
-					}
-
-					type = assembly.GetTypeByMetadataName( typeName );
-				}
+				INamedTypeSymbol type = GetTypeSymbol( compilationAssemblies, compilation, qualifiedAssembly, typeName );
 
-				if( type == null || type.Kind == SymbolKind.ErrorType ) {
+				if( type == null ) {
 					continue;
 				}
 
@@ -103,12 +104,36 @@ internal static ImmutabilityContext Create( Compilation compilation, Annotations
 					type
 				);
 
-				builder.Add( type, info );
+				extraImmutableTypesBuilder.Add( type, info );
+			}
+
+			// Generate a set of methods that we have specifically determined
+			// have a return value which should be considered Immutable by the Analyzer.
+			var knownImmutableReturnsBuilder = ImmutableHashSet.CreateBuilder<IMethodSymbol>();
+			foreach( ( string typeName, string methodName, string qualifiedAssembly ) in KnownImmutableReturningMethods ) {
+				INamedTypeSymbol type = GetTypeSymbol( compilationAssemblies, compilation, qualifiedAssembly, typeName );
+
+				if( type == null ) {
+					continue;
+				}
+
+				IMethodSymbol[] methodSymbols = type
+					.GetMembers( methodName )
+					.OfType<IMethodSymbol>()
+					.Where( m => m.Parameters.Length == 0 )
+					.ToArray();
+
+				if( methodSymbols.Length != 1 ) {
+					continue;
+				}
+
+				knownImmutableReturnsBuilder.Add( methodSymbols[0] );
 			}
 
 			return new ImmutabilityContext(
 				annotationsContext: annotationsContext,
-				extraImmutableTypes: builder.ToImmutable(),
+				extraImmutableTypes: extraImmutableTypesBuilder.ToImmutable(),
+				knownImmutableReturns: knownImmutableReturnsBuilder.ToImmutable(),
 				conditionalTypeParamemters: ImmutableHashSet<ITypeParameterSymbol>.Empty
 			);
 		}
@@ -129,5 +154,30 @@ private static ImmutableDictionary<string, IAssemblySymbol> GetCompilationAssemb
 			return builder.ToImmutable();
 		}
 
+		private static INamedTypeSymbol GetTypeSymbol(
+			ImmutableDictionary<string, IAssemblySymbol> compilationAssemblies,
+			Compilation compilation,
+			string qualifiedAssembly,
+			string typeName
+		) {
+			INamedTypeSymbol type;
+
+			if( string.IsNullOrEmpty( qualifiedAssembly ) ) {
+				type = compilation.GetTypeByMetadataName( typeName );
+			} else {
+				if( !compilationAssemblies.TryGetValue( qualifiedAssembly, out IAssemblySymbol assembly ) ) {
+					return null;
+				}
+
+				type = assembly.GetTypeByMetadataName( typeName );
+			}
+
+			if( type == null || type.Kind == SymbolKind.ErrorType ) {
+				return null;
+			}
+
+			return type;
+		}
+
 	}
 }
diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.cs
@@ -19,6 +19,7 @@ internal sealed partial class ImmutabilityContext {
 
 		private readonly AnnotationsContext m_annotationsContext;
 		private readonly ImmutableDictionary<INamedTypeSymbol, ImmutableTypeInfo> m_extraImmutableTypes;
+		private readonly ImmutableHashSet<IMethodSymbol> m_knownImmutableReturns;
 		private readonly ImmutableHashSet<ITypeParameterSymbol> m_conditionalTypeParameters;
 
 		// Hard code this to avoid looking up the ITypeSymbol to include it in m_extraImmutableTypes
@@ -46,10 +47,12 @@ internal sealed partial class ImmutabilityContext {
 		private ImmutabilityContext(
 			AnnotationsContext annotationsContext,
 			ImmutableDictionary<INamedTypeSymbol, ImmutableTypeInfo> extraImmutableTypes,
+			ImmutableHashSet<IMethodSymbol> knownImmutableReturns,
 			ImmutableHashSet<ITypeParameterSymbol> conditionalTypeParamemters
 		) {
 			m_annotationsContext = annotationsContext;
 			m_extraImmutableTypes = extraImmutableTypes;
+			m_knownImmutableReturns = knownImmutableReturns;
 			m_conditionalTypeParameters = conditionalTypeParamemters;
 		}
 
@@ -72,6 +75,7 @@ INamedTypeSymbol type
 			return new ImmutabilityContext(
 				annotationsContext: m_annotationsContext,
 				extraImmutableTypes: m_extraImmutableTypes,
+				knownImmutableReturns: m_knownImmutableReturns,
 				conditionalTypeParamemters: m_conditionalTypeParameters.Union( conditionalTypeParameters )
 			);
 		}
@@ -210,6 +214,15 @@ out diagnostic
 			}
 		}
 
+		/// <summary>
+		/// Determines if the return value of a method is known to be immutable.
+		/// </summary>
+		/// <param name="methodSymbol">The method to check</param>
+		/// <returns>Is the return value immutable?</returns>
+		public bool IsReturnValueKnownToBeImmutable( IMethodSymbol methodSymbol ) {
+			return m_knownImmutableReturns.Contains( methodSymbol.OriginalDefinition );
+		}
+
 		public ImmutableTypeInfo GetImmutableTypeInfo(
 			INamedTypeSymbol type
 		) {

diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs
@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Collections.Immutable;
 using System.Linq;
 using D2L.CodeStyle.Analyzers.Extensions;
 using Microsoft.CodeAnalysis;
@@ -390,6 +391,7 @@ out Diagnostic diagnostic
 
 			query = default;
 			diagnostic = null;
+			SemanticModel semanticModel = null;
 
 			// Easy cases
 			switch( assignment.Expression.Kind() ) {
@@ -425,32 +427,51 @@ out Diagnostic diagnostic
 
 					return AssignmentQueryKind.Hopeless;
 
-				default:
-					var model = m_compilation.GetSemanticModel( assignment.Expression.SyntaxTree );
-					var typeInfo = model.GetTypeInfo( assignment.Expression );
-
-					// Type can be null in the case of an implicit conversion where the
-					// expression alone doesn't have a type. For example:
-					//   int[] foo = { 1, 2, 3 };
-					var typeToCheck = typeInfo.Type ?? typeInfo.ConvertedType;
-
-					if( assignment.Expression is BaseObjectCreationExpressionSyntax _ ) {
-						// When we have a new T() we don't need to worry about the value
-						// being anything other than an instance of T.
-						query = new ImmutabilityQuery(
-							ImmutableTypeKind.Instance,
-							typeToCheck
-						);
-					} else {
-						// In general we need to handle subtypes.
-						query = new ImmutabilityQuery(
-							ImmutableTypeKind.Total,
-							typeToCheck
-						);
+				case SyntaxKind.InvocationExpression:
+
+					// Some methods are known to have return values that are
+					// immutable (such as Enumerable.Empty()).
+					// These should be considered immutable by the Analyzer.
+					semanticModel ??= m_compilation.GetSemanticModel( assignment.Expression.SyntaxTree );
+					if( semanticModel
+						.GetSymbolInfo( assignment.Expression )
+						.Symbol is not IMethodSymbol methodSymbol
+					) {
+						break;
 					}
 
-					return AssignmentQueryKind.ImmutabilityQuery;
+					if( m_context.IsReturnValueKnownToBeImmutable( methodSymbol ) ) {
+						return AssignmentQueryKind.NothingToCheck;
+					}
+
+					break;
+			}
+
+			// If nothing above was caught, then fallback to querying.
+			semanticModel ??= m_compilation.GetSemanticModel( assignment.Expression.SyntaxTree );
+			var typeInfo = semanticModel.GetTypeInfo( assignment.Expression );
+
+			// Type can be null in the case of an implicit conversion where the
+			// expression alone doesn't have a type. For example:
+			//   int[] foo = { 1, 2, 3 };
+			var typeToCheck = typeInfo.Type ?? typeInfo.ConvertedType;
+
+			if( assignment.Expression is BaseObjectCreationExpressionSyntax _ ) {
+				// When we have a new T() we don't need to worry about the value
+				// being anything other than an instance of T.
+				query = new ImmutabilityQuery(
+					ImmutableTypeKind.Instance,
+					typeToCheck
+				);
+			} else {
+				// In general we need to handle subtypes.
+				query = new ImmutabilityQuery(
+					ImmutableTypeKind.Total,
+					typeToCheck
+				);
 			}
+
+			return AssignmentQueryKind.ImmutabilityQuery;
 		}
 
 		private static Location GetLocationOfMember( ISymbol s ) =>s

diff --git a/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs b/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs
@@ -1,8 +1,10 @@
 // analyzer: D2L.CodeStyle.Analyzers.Immutability.ImmutabilityAnalyzer
 
 using System;
+using System.Collections.Generic;
 using System.ComponentModel;
 using System.Dynamic;
+using System.Linq;
 using D2L.CodeStyle.Annotations;
 using static D2L.CodeStyle.Annotations.Objects;
 
@@ -689,6 +691,9 @@ object this[ int index ] {
 		readonly Types.SomeImmutableGenericInterfaceRestrictingTU</* NonImmutableTypeHeldByImmutable(class, object, ) */ object /**/, int>m_field176;
 		Types.SomeImmutableGenericInterfaceRestrictingTU</* NonImmutableTypeHeldByImmutable(class, object, ) */ object /**/, int> Property80 { get; }
 		Types.SomeImmutableGenericInterfaceRestrictingTU</* NonImmutableTypeHeldByImmutable(class, object, ) */ object /**/, int> Property81 { get { return default; } }
+
+		private readonly IEnumerable<int> m_enumerable = Enumerable.Empty<int>();
+		private readonly int[] m_array = Array.Empty<int>();
 	}
 
 	[Immutable]