From f8251f3311de4673e3fddeb53fd753929087cc62 Mon Sep 17 00:00:00 2001 From: Timothy J Cowen <32042011+TimothyJCowen@users.noreply.github.com> Date: Fri, 12 Mar 2021 15:23:58 -0500 Subject: [PATCH] Added check to allow Array/Enumerable.Empty calls as immutable. (#732) Added supporting test cases. --- .../ImmutabilityContext.Factory.cs | 80 +++++++++++++++---- .../Immutability/ImmutabilityContext.cs | 13 +++ .../ImmutableDefinitionChecker.cs | 67 ++++++++++------ .../Specs/ImmutabilityAnalyzer.cs | 5 ++ 4 files changed, 127 insertions(+), 38 deletions(-) diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.Factory.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityContext.Factory.cs index 960f77bc..8931e9e3 100644 --- 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 compilationAssmeblies = GetCompilationAssemblies( compilation ); - var builder = ImmutableDictionary.CreateBuilder(); + ImmutableDictionary compilationAssemblies = GetCompilationAssemblies( compilation ); + // Generate a dictionary of types that we have specifically determined + // should be considered Immutable by the Analyzer. + var extraImmutableTypesBuilder = ImmutableDictionary.CreateBuilder(); 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(); + 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() + .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.Empty ); } @@ -129,5 +154,30 @@ private static ImmutableDictionary GetCompilationAssemb return builder.ToImmutable(); } + private static INamedTypeSymbol GetTypeSymbol( + ImmutableDictionary 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 index 46a6cafb..81e94087 100644 --- 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 m_extraImmutableTypes; + private readonly ImmutableHashSet m_knownImmutableReturns; private readonly ImmutableHashSet 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 extraImmutableTypes, + ImmutableHashSet knownImmutableReturns, ImmutableHashSet 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 } } + /// + /// Determines if the return value of a method is known to be immutable. + /// + /// The method to check + /// Is the return value immutable? + 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 index 0498b1d1..0449d906 100644 --- 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 index 24017320..0c4a2324 100644 --- 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.SomeImmutableGenericInterfaceRestrictingTUm_field176; Types.SomeImmutableGenericInterfaceRestrictingTU Property80 { get; } Types.SomeImmutableGenericInterfaceRestrictingTU Property81 { get { return default; } } + + private readonly IEnumerable m_enumerable = Enumerable.Empty(); + private readonly int[] m_array = Array.Empty(); } [Immutable]