From d00a59f8d4fc90d1c02ba0adc7a46b149d2556f8 Mon Sep 17 00:00:00 2001 From: Owen Smith Date: Fri, 9 Apr 2021 11:45:29 -0400 Subject: [PATCH 1/4] refactor AssignmentInfo linq expression to support extra logic before construction --- .../ImmutableDefinitionChecker.cs | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs index 46ad2793..d4d4d1d1 100644 --- a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs +++ b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs @@ -357,13 +357,9 @@ ExpressionSyntax initializer .Select( sr => sr.GetSyntax() ) .SelectMany( constructorSyntax => constructorSyntax.DescendantNodes() ) .OfType() - .Where( assignmentSyntax => IsAnAssignmentTo( assignmentSyntax, memberSymbol ) ) - .Select( assignmentSyntax => assignmentSyntax.Right ) - .Select( expr => AssignmentInfo.Create( - model: m_compilation.GetSemanticModel( expr.SyntaxTree ), - isInitializer: false, - expression: expr - ) ); + .Select( assignmentSyntax => GetAssignmentInfoIfToSymbolOrNull( assignmentSyntax, memberSymbol ) ) + .Where( info => info.HasValue ) + .Select( info => info.Value ); if ( initializer != null ) { assignmentExpressions = assignmentExpressions.Append( @@ -378,7 +374,7 @@ ExpressionSyntax initializer return assignmentExpressions; } - private bool IsAnAssignmentTo( + private AssignmentInfo? GetAssignmentInfoIfToSymbolOrNull( AssignmentExpressionSyntax assignmentSyntax, ISymbol memberSymbol ) { @@ -387,9 +383,17 @@ ISymbol memberSymbol var leftSideSymbol = semanticModel.GetSymbolInfo( assignmentSyntax.Left ) .Symbol; - return SymbolEqualityComparer.Default.Equals( + if( !SymbolEqualityComparer.Default.Equals( memberSymbol, leftSideSymbol + ) ) { + return null; + } + + return AssignmentInfo.Create( + model: semanticModel, + isInitializer: false, + expression: assignmentSyntax.Right ); } From d78e2ba3e9e2df9b6ebe3ca3d4394d5f6a76d3bb Mon Sep 17 00:00:00 2001 From: Owen Smith Date: Fri, 9 Apr 2021 13:07:25 -0400 Subject: [PATCH 2/4] handle deconstructed assignments Fixes: https://github.com/Brightspace/D2L.CodeStyle/issues/738 --- .../ImmutableDefinitionChecker.cs | 106 ++++++++++++++++-- .../Specs/ImmutabilityAnalyzer.cs | 61 +++++++++- 2 files changed, 153 insertions(+), 14 deletions(-) diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs index d4d4d1d1..ed5ba0b6 100644 --- a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs +++ b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs @@ -333,6 +333,18 @@ ExpressionSyntax expression assignedType: assignedType ); } + + public static AssignmentInfo Create( + bool isInitializer, + ExpressionSyntax expression, + ITypeSymbol assignedType + ) { + return new AssignmentInfo( + isInitializer: isInitializer, + expression: expression, + assignedType: assignedType + ); + } } @@ -380,21 +392,80 @@ ISymbol memberSymbol ) { var semanticModel = m_compilation.GetSemanticModel( assignmentSyntax.SyntaxTree ); - var leftSideSymbol = semanticModel.GetSymbolInfo( assignmentSyntax.Left ) - .Symbol; + var lhsExpressions = assignmentSyntax.Left switch { + TupleExpressionSyntax tuple => tuple.Arguments.Select( arg => arg.Expression ).ToImmutableArray(), + _ => ImmutableArray.Create( assignmentSyntax.Left ) + }; - if( !SymbolEqualityComparer.Default.Equals( - memberSymbol, - leftSideSymbol - ) ) { - return null; + for( var i = 0; i < lhsExpressions.Length; ++i ) { + var lhs = lhsExpressions[ i ]; + + var leftSideSymbol = semanticModel.GetSymbolInfo( lhs ) + .Symbol; + + if( !SymbolEqualityComparer.Default.Equals( + memberSymbol, + leftSideSymbol + ) ) { + continue; + } + + if( lhsExpressions.Length == 1 ) { + return AssignmentInfo.Create( + model: semanticModel, + isInitializer: false, + expression: assignmentSyntax.Right + ); + } + + if( semanticModel.GetTypeInfo( assignmentSyntax.Right ).Type is INamedTypeSymbol assignedType + && assignedType.IsTupleType + ) { + if( assignedType.TypeArguments.Length != lhsExpressions.Length ) { + return AssignmentInfo.Create( + isInitializer: false, + expression: assignmentSyntax.Right, + assignedType: null + ); + } + + ExpressionSyntax rhs = assignmentSyntax.Right switch { + TupleExpressionSyntax tuple => tuple.Arguments[ i ].Expression, + _ => assignmentSyntax.Right + }; + + return AssignmentInfo.Create( + isInitializer: false, + expression: rhs, + assignedType: assignedType.TypeArguments[ i ] + ); + } + + DeconstructionInfo deconstruction = semanticModel.GetDeconstructionInfo( assignmentSyntax ); + if( deconstruction.Method == null ) { + return AssignmentInfo.Create( + isInitializer: false, + expression: assignmentSyntax.Right, + assignedType: null + ); + } + + if( deconstruction.Method.Parameters.Length != lhsExpressions.Length ) { + return AssignmentInfo.Create( + isInitializer: false, + expression: assignmentSyntax.Right, + assignedType: null + ); + } + + return AssignmentInfo.Create( + isInitializer: false, + expression: assignmentSyntax.Right, + assignedType: deconstruction.Method.Parameters[ i ].Type + ); } - return AssignmentInfo.Create( - model: semanticModel, - isInitializer: false, - expression: assignmentSyntax.Right - ); + return null; } private enum AssignmentQueryKind { @@ -478,6 +549,17 @@ .Symbol is not IMethodSymbol methodSymbol break; } + if( assignment.AssignedType == null + || assignment.AssignedType.TypeKind == TypeKind.Error + ) { + diagnostic = Diagnostic.Create( + Diagnostics.NonImmutableTypeHeldByImmutable, + assignment.Expression.GetLocation(), + "blarg", "blarg", "blarg" + ); + return AssignmentQueryKind.Hopeless; + } + // If nothing above was caught, then fallback to querying. if( assignment.Expression is BaseObjectCreationExpressionSyntax _ ) { diff --git a/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs b/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs index 9398d58a..e0fc97b5 100644 --- a/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs +++ b/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs @@ -122,6 +122,63 @@ public SomeClassWithConstructor8() { (m_interface) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, (or [ImmutableBaseClass])) */ new Bad() /**/; } } + + [Immutable] + public sealed class SomeClassWithConstructor9 { + public readonly RegularInterface m_interface = new Good(); + public readonly RegularInterface m_interface2 = new Good(); + + public SomeClassWithConstructor9() { + (m_interface, m_interface2) = ( + /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, (or [ImmutableBaseClass])) */ new Bad() /**/, + new Good(); + ); + } + } + + [Immutable] + public sealed class SomeClassWithConstructor10 { + public readonly RegularInterface m_interface = new Good(); + public readonly RegularInterface m_interface2 = new Good(); + + public SomeClassWithConstructor10() { + (m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, ) */ GetValues() /**/; + } + + public static (Bad, Good) GetValues() => default; + } + + [Immutable] + public sealed class SomeClassWithConstructor11 { + public readonly RegularInterface m_interface = new Good(); + public readonly RegularInterface m_interface2 = new Good(); + + public SomeClassWithConstructor11() { + (m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, ) */ GetValues() /**/; + } + + public static DeconstructingClass GetValues() => default; + + private sealed class DeconstructingClass { + public void Deconstruct( out Bad bad, out Good good ) => (bad, good) = (default, default); + } + } + + [Immutable] + public sealed class SomeClassWithConstructor12 { + public readonly RegularInterface m_interface = new Good(); + public readonly RegularInterface m_interface2 = new Good(); + + public SomeClassWithConstructor12() { + (m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(blarg, blarg, blarg) | NonImmutableTypeHeldByImmutable(blarg, blarg, blarg) */ GetValues() /**/; + } + + public static DeconstructingClass GetValues() => default; + + private sealed class DeconstructingClass { + public void Deconstruct( out Good A, out Good B, out Good C ) => (A, B, C) = (default, default, default); + } + } #endregion @@ -1199,10 +1256,10 @@ public record SomeRecord { int /* MemberIsNotReadOnly(Property, Y, SomeRecord) */ Y /**/ { get; set; } - /* NonImmutableTypeHeldByImmutable(class, object, ) */ object /**/ Z { get; } + object Z { get; } public SomeRecord( SomeRecord v, int w, Types.SomeImmutableClass x, int y, object z ) - => (V, W, X, Y, Z) = (v, w, x, y, z); + => (V, W, X, Y, Z) = (v, w, x, y, /* NonImmutableTypeHeldByImmutable(class, object, ) */ z /**/); } record NonImmutableBaseRecord(object x); From 1bfe273432aac7893e75c03897386bde1ef6124e Mon Sep 17 00:00:00 2001 From: Owen Smith Date: Mon, 12 Apr 2021 11:22:15 -0400 Subject: [PATCH 3/4] commentary --- .../ImmutableDefinitionChecker.cs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs index ed5ba0b6..9ea6d452 100644 --- a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs +++ b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs @@ -410,6 +410,7 @@ ISymbol memberSymbol continue; } + // LHS is just a single variable, no deconstruction happening if( lhsExpressions.Length == 1 ) { return AssignmentInfo.Create( model: semanticModel, @@ -418,6 +419,8 @@ ISymbol memberSymbol ); } + // Value tuples are a common RHS assignment for deconstruction, but it's built-in and there's + // no deconstruction method. Handle value tuples specifically if it is one. if( semanticModel.GetTypeInfo( assignmentSyntax.Right ).Type is INamedTypeSymbol assignedType && assignedType.IsTupleType ) { @@ -429,6 +432,9 @@ ISymbol memberSymbol ); } + // If this is a value tuple literal then we can narrow in to the exact expression + // responsible for the tuple item. This allows for additional specificity prior to conversions + // or things like "known immutable method returns" (such as Array.Empty()) ExpressionSyntax rhs = assignmentSyntax.Right switch { TupleExpressionSyntax tuple => tuple.Arguments[ i ].Expression, _ => assignmentSyntax.Right @@ -441,7 +447,18 @@ ISymbol memberSymbol ); } + // Other types can participate in deconstruction by specifying a deconstruction method + // public void Deconstruct( out T1 a, out T2 b ... ) + // These methods must have a unique number of arguments. e.g. the following are ambiguous + // public void Deconstruct( out string a, out string b ) + // public void Deconstruct( out int a, out int b ) + // The overall deconstruction can be "nested" if a type's deconstruction includes a type which + // is itself deconstructed as well DeconstructionInfo deconstruction = semanticModel.GetDeconstructionInfo( assignmentSyntax ); + + // If there was no method, then: + // * This might've been something like a value tuple, where the deconstruction is implicit + // * This isn't a valid assignment (and thus should be compiler error) if( deconstruction.Method == null ) { return AssignmentInfo.Create( isInitializer: false, @@ -450,6 +467,12 @@ ISymbol memberSymbol ); } + // DeconstructionInfo is a tree. Each non-terminal node has a method which produces the values + // Some child nodes may themselves have methods as well, producing more values (referred to as + // "nested" deconstruction). + // + // Only handling the simple case of non-nested deconstruction, meaning the tree has a depth of 2 + // with a single method. In this case there should be a parameter for every variable being assigned to if( deconstruction.Method.Parameters.Length != lhsExpressions.Length ) { return AssignmentInfo.Create( isInitializer: false, From 330a3a6e5c08a8b8ad6515ac42989893ea7f9459 Mon Sep 17 00:00:00 2001 From: Owen Smith Date: Mon, 12 Apr 2021 11:47:07 -0400 Subject: [PATCH 4/4] fixup tests some. test some nesting --- .../Immutability/ImmutabilityAnalyzer.cs | 2 +- .../ImmutableDefinitionChecker.cs | 5 +- .../Specs/ImmutabilityAnalyzer.cs | 67 +++++++++++++++---- 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs index 9913f919..c10a8121 100644 --- a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs +++ b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs @@ -31,7 +31,7 @@ public sealed class ImmutabilityAnalyzer : DiagnosticAnalyzer { ); public override void Initialize( AnalysisContext context ) { - context.EnableConcurrentExecution(); + //context.EnableConcurrentExecution(); context.ConfigureGeneratedCodeAnalysis( GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics ); context.RegisterCompilationStartAction( CompilationStart ); } diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs index 9ea6d452..9fb990e5 100644 --- a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs +++ b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs @@ -477,7 +477,10 @@ ISymbol memberSymbol return AssignmentInfo.Create( isInitializer: false, expression: assignmentSyntax.Right, - assignedType: null + + // While we weren't able to determine the pre-conversion type being assigned (with this simple logic) + // just consider the variable's type so this isn't always a complete error + assignedType: semanticModel.GetTypeInfo( lhsExpressions[ i ] ).Type ); } diff --git a/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs b/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs index e0fc97b5..111a6e0c 100644 --- a/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs +++ b/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs @@ -116,17 +116,17 @@ public SomeClassWithConstructor7( U you ) { [Immutable] public sealed class SomeClassWithConstructor8 { - public readonly RegularInterface m_interface = new Good(); + public readonly RegularInterface m_interface; public SomeClassWithConstructor8() { - (m_interface) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, (or [ImmutableBaseClass])) */ new Bad() /**/; + ( m_interface ) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, (or [ImmutableBaseClass])) */ new Bad() /**/; } } [Immutable] public sealed class SomeClassWithConstructor9 { - public readonly RegularInterface m_interface = new Good(); - public readonly RegularInterface m_interface2 = new Good(); + public readonly RegularInterface m_interface; + public readonly RegularInterface m_interface2; public SomeClassWithConstructor9() { (m_interface, m_interface2) = ( @@ -138,8 +138,8 @@ public SomeClassWithConstructor9() { [Immutable] public sealed class SomeClassWithConstructor10 { - public readonly RegularInterface m_interface = new Good(); - public readonly RegularInterface m_interface2 = new Good(); + public readonly RegularInterface m_interface; + public readonly RegularInterface m_interface2; public SomeClassWithConstructor10() { (m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, ) */ GetValues() /**/; @@ -150,8 +150,8 @@ public SomeClassWithConstructor10() { [Immutable] public sealed class SomeClassWithConstructor11 { - public readonly RegularInterface m_interface = new Good(); - public readonly RegularInterface m_interface2 = new Good(); + public readonly RegularInterface m_interface; + public readonly RegularInterface m_interface2; public SomeClassWithConstructor11() { (m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, ) */ GetValues() /**/; @@ -159,15 +159,15 @@ public SomeClassWithConstructor11() { public static DeconstructingClass GetValues() => default; - private sealed class DeconstructingClass { + public sealed class DeconstructingClass { public void Deconstruct( out Bad bad, out Good good ) => (bad, good) = (default, default); } } [Immutable] public sealed class SomeClassWithConstructor12 { - public readonly RegularInterface m_interface = new Good(); - public readonly RegularInterface m_interface2 = new Good(); + public readonly RegularInterface m_interface; + public readonly RegularInterface m_interface2; public SomeClassWithConstructor12() { (m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(blarg, blarg, blarg) | NonImmutableTypeHeldByImmutable(blarg, blarg, blarg) */ GetValues() /**/; @@ -175,10 +175,53 @@ public SomeClassWithConstructor12() { public static DeconstructingClass GetValues() => default; - private sealed class DeconstructingClass { + public sealed class DeconstructingClass { public void Deconstruct( out Good A, out Good B, out Good C ) => (A, B, C) = (default, default, default); } } + + [Immutable] + public sealed class SomeClassWithConstructor13 { + public readonly RegularInterface m_interface; + public readonly RegularInterface m_interface2; + public readonly RegularInterface m_interface3; + + public SomeClassWithConstructor13() { + (m_interface, (m_interface2, m_interface3)) = + /*NonImmutableTypeHeldByImmutable(interface, SpecTests.Types.RegularInterface, ) | NonImmutableTypeHeldByImmutable(interface, SpecTests.Types.RegularInterface, ) | NonImmutableTypeHeldByImmutable(interface, SpecTests.Types.RegularInterface, ) */ GetValues() /**/; + } + + public static DeconstructingClass GetValues() => default; + + public sealed class DeconstructingClass { + public void Deconstruct( out Good A, out DeconstructingClass2 DC ) => (A, DC) = (default, default); + } + + private sealed class DeconstructingClass2 { + public void Deconstruct( out Good B, out Good C ) => (B, C) = (default, default); + } + } + + [Immutable] + public sealed class SomeClassWithConstructor14 { + public readonly Good m_interface; + public readonly Good m_interface2; + public readonly Good m_interface3; + + public SomeClassWithConstructor14() { + (m_interface, (m_interface2, m_interface3)) = GetValues(); + } + + public static DeconstructingClass GetValues() => default; + + public sealed class DeconstructingClass { + public void Deconstruct( out Good A, out DeconstructingClass2 DC ) => (A, DC) = (default, default); + } + + public sealed class DeconstructingClass2 { + public void Deconstruct( out Good B, out Good C ) => (B, C) = (default, default); + } + } #endregion