immutability fixes (#958)

* require application of all [ConditionallyImmutable.OnlyIf] type paramters * a [ConditionallyImmutable] type should not be able to extend/implement an [Immutable] one Fixes: #956 Co-authored-by: Jacob Parker <[email protected]>
Brightspace · Aug 9, 2024 · bc72a4e · bc72a4e
1 parent 63fed57
commit bc72a4e
Show file tree

Hide file tree

Showing 5 changed files with 130 additions and 42 deletions.
diff --git a/src/D2L.CodeStyle.Analyzers/Diagnostics.cs b/src/D2L.CodeStyle.Analyzers/Diagnostics.cs
@@ -756,5 +756,14 @@ public static class Diagnostics {
 			defaultSeverity: DiagnosticSeverity.Error,
 			isEnabledByDefault: true
 		);
+
+		public static readonly DiagnosticDescriptor UnappliedConditionalImmutability = new DiagnosticDescriptor(
+			id: "D2L0103",
+			title: "A type parameter marked [ConditionallyImmutable.OnlyIf] was not applied to a [ConditionallyImmutable] implemented type",
+			messageFormat: "{0} should apply all [ConditionallyImmutable.OnlyIf] type parameters to the [ConditionallyImmutable] {1} {2}",
+			category: "Correctness",
+			defaultSeverity: DiagnosticSeverity.Error,
+			isEnabledByDefault: true
+		);
 	}
 }
diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs
@@ -31,7 +31,8 @@ public sealed partial class ImmutabilityAnalyzer : DiagnosticAnalyzer {
 			Diagnostics.UnknownImmutabilityAssignmentKind,
 
 			Diagnostics.MissingTransitiveImmutableAttribute,
-			Diagnostics.InconsistentMethodAttributeApplication
+			Diagnostics.InconsistentMethodAttributeApplication,
+			Diagnostics.UnappliedConditionalImmutability
 		);
 
 		private readonly ImmutableHashSet<string> m_additionalImmutableTypes;

diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs
@@ -86,52 +86,92 @@ private void CompareConsistencyToBaseType(
 			ImmutableTypeInfo baseTypeInfo,
 			CancellationToken cancellationToken
 		) {
-			switch( baseTypeInfo.Kind ) {
-				case ImmutableTypeKind.None:
+			switch( ((baseTypeInfo.Kind, baseTypeInfo.IsConditional), (typeInfo.Kind, typeInfo.IsConditional)) ) {
+				default:
+					throw new NotImplementedException();
+
+				// If the base type doesn't require us to be immutable then there is nothing to check
+				case ((ImmutableTypeKind.None, _), _):
+				case ((ImmutableTypeKind.Instance, _), _):
 					return;
 
-				case ImmutableTypeKind.Instance:
+				// When the base type is [Immutable] or [ConditionallyImmutable] we need to be as well
+				case ((ImmutableTypeKind.Total, _), (ImmutableTypeKind.None, _)):
+				case ((ImmutableTypeKind.Total, _), (ImmutableTypeKind.Instance, _)):
+					RaiseMissingAttribute();
 					return;
 
-				case ImmutableTypeKind.Total:
-					if( typeInfo.Kind == ImmutableTypeKind.Total ) {
-						break;
-					}
+				// If the implementing type is [Immutable] then the conditionality of the
+				// base type doesn't matter (for this diagnostic)
+				case ((ImmutableTypeKind.Total, _), (ImmutableTypeKind.Total, false)):
+					return;
 
-					// The docs say the only things that can have BaseType == null
-					// are interfaces, System.Object itself (won't come up in our
-					// analysis because (1) it !hasTheImmutableAttribute (2) you
-					// can't explicitly list it as a base class anyway) and
-					// pointer types (the base value type probably also doesn't
-					// have it.)
-					bool isInterface =
-						baseTypeInfo.Type.BaseType == null
-						&& baseTypeInfo.Type.SpecialType != SpecialType.System_Object
-						&& baseTypeInfo.Type.SpecialType != SpecialType.System_ValueType
-						&& baseTypeInfo.Type.Kind != SymbolKind.PointerType;
-
-					(TypeDeclarationSyntax syntax, _) = typeInfo.Type.ExpensiveGetSyntaxImplementingType(
-						baseTypeOrInterface: baseTypeInfo.Type,
-						compilation: m_compilation,
-						cancellationToken
-					);
-
-					m_diagnosticSink(
-						Diagnostic.Create(
-							Diagnostics.MissingTransitiveImmutableAttribute,
-							syntax.Identifier.GetLocation(),
-							properties: FixArgs,
-							typeInfo.Type.GetFullTypeName(),
-							baseTypeInfo.IsConditional ? " (or [ConditionallyImmutable])" : "",
-							isInterface ? "interface" : "base class",
-							baseTypeInfo.Type.GetFullTypeName()
-						)
-					);
-
-					break;
+				// If the base type is [Immutable] then the implementing type should be as well
+				case ((ImmutableTypeKind.Total, false), (ImmutableTypeKind.Total, true)):
+					// TODO: error message could be improved to directly explain that we need [Immutable]
+					RaiseMissingAttribute();
+					return;
 
-				default:
-					throw new NotImplementedException();
+				// When we and our base type are [ConditionallyImmutable] we need to consider the compatibility of those conditions
+				case ((ImmutableTypeKind.Total, true), (ImmutableTypeKind.Total, true)):
+					InspectConditionalParameterApplication();
+					return;
+			}
+
+			void RaiseMissingAttribute() {
+				(TypeDeclarationSyntax syntax, _) = typeInfo.Type.ExpensiveGetSyntaxImplementingType(
+					baseTypeOrInterface: baseTypeInfo.Type,
+					compilation: m_compilation,
+					cancellationToken
+				);
+
+				m_diagnosticSink(
+					Diagnostic.Create(
+						Diagnostics.MissingTransitiveImmutableAttribute,
+						syntax.Identifier.GetLocation(),
+						properties: FixArgs,
+						typeInfo.Type.GetFullTypeName(),
+						baseTypeInfo.IsConditional ? " (or [ConditionallyImmutable])" : "",
+						baseTypeInfo.Type.TypeKind == TypeKind.Interface ? "interface" : "base class",
+						baseTypeInfo.Type.GetFullTypeName()
+					)
+				);
+			}
+
+			void InspectConditionalParameterApplication() {
+				foreach( ITypeParameterSymbol typeParameter in typeInfo.ConditionalTypeParameters ) {
+					bool parameterUsed = false;
+					for( int i = 0; i < baseTypeInfo.Type.TypeArguments.Length && !parameterUsed; i++ ) {
+						ITypeSymbol typeArgument = baseTypeInfo.Type.TypeArguments[ i ];
+						if( !SymbolEqualityComparer.Default.Equals( typeParameter, typeArgument ) ) {
+							continue;
+						}
+
+						ITypeParameterSymbol baseTypeParameter = baseTypeInfo.Type.TypeParameters[ i ];
+						if( baseTypeInfo.ConditionalTypeParameters.Contains( baseTypeParameter, SymbolEqualityComparer.Default ) ) {
+							parameterUsed = true;
+							break;
+						}
+					}
+
+					if( !parameterUsed ) {
+						(TypeDeclarationSyntax syntax, _) = typeInfo.Type.ExpensiveGetSyntaxImplementingType(
+							baseTypeOrInterface: baseTypeInfo.Type,
+							compilation: m_compilation,
+							cancellationToken
+						);
+						m_diagnosticSink(
+							Diagnostic.Create(
+								Diagnostics.UnappliedConditionalImmutability,
+								syntax.Identifier.GetLocation(),
+								typeInfo.Type.GetFullTypeName(),
+								baseTypeInfo.Type.TypeKind == TypeKind.Interface ? "interface" : "base class",
+								baseTypeInfo.Type.GetFullTypeName()
+							)
+						);
+						return;
+					}
+				}
 			}
 		}
 

diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableTypeInfo.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableTypeInfo.cs
@@ -23,13 +23,18 @@ ImmutableArray<bool> conditionalTypeParameters
 			Kind = kind;
 			Type = type;
 			m_conditionalTypeParameters = conditionalTypeParameters;
+
+			IsConditional = m_conditionalTypeParameters.Any( isConditional => isConditional );
 		}
 
 		public ImmutableTypeKind Kind { get; }
 
 		public INamedTypeSymbol Type { get; }
 
-		public bool IsConditional => m_conditionalTypeParameters.Length > 0;
+		public bool IsConditional { get; }
+
+		public IEnumerable<ITypeParameterSymbol> ConditionalTypeParameters => Type.TypeParameters.Where( IsConditionalParameter );
+		private bool IsConditionalParameter( ITypeParameterSymbol _, int index ) => m_conditionalTypeParameters[ index ];
 
 		public bool IsImmutableDefinition(
 			ImmutabilityContext context,

diff --git a/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs b/tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs
@@ -198,6 +198,15 @@ public interface SomeImmutableGenericInterfaceGivenU<T, [ConditionallyImmutable.
 		[ConditionallyImmutable]
 		public interface SomeImmutableGenericInterfaceGivenTU<[ConditionallyImmutable.OnlyIf] T, [ConditionallyImmutable.OnlyIf] U> { }
 
+		[ConditionallyImmutable]
+		public abstract class SomeImmutableGenericBaseClassGivenT<[ConditionallyImmutable.OnlyIf] T, U> { }
+
+		[ConditionallyImmutable]
+		public abstract class SomeImmutableGenericBaseClassGivenU<T, [ConditionallyImmutable.OnlyIf] U> { }
+
+		[ConditionallyImmutable]
+		public abstract class SomeImmutableGenericBaseClassGivenTU<[ConditionallyImmutable.OnlyIf] T, [ConditionallyImmutable.OnlyIf] U> { }
+
 		[Immutable]
 		public interface SomeImmutableGenericInterfaceRestrictingT<[Immutable] T, U> { }
 
@@ -243,6 +252,24 @@ public sealed class NonImmutableClassWithNonImmutableTypeParameterImplementingIm
 	[Immutable]
 	public sealed class AnalyzedClassWithNonImmutableTypeParameterImplementingImmutableBaseClassParameter<T> : Types.SomeImmutableGenericClassRestrictingT</* TypeParameterIsNotKnownToBeImmutable(T) */ T /**/> { }
 
+	[Immutable] public sealed class ImmutableType_ConditionalInterface_TypeParameters_NotMet<T> : Types.SomeImmutableGenericInterfaceGivenT<T, T> { }
+	[Immutable] public sealed class ImmutableType_ConditionalInterface_TypeParameters_Met1<[Immutable] T> : Types.SomeImmutableGenericInterfaceGivenT<T, T> { }
+	[Immutable] public sealed class ImmutableType_ConditionalInterface_TypeParameters_Met2<[Immutable] T> : Types.SomeImmutableGenericInterfaceGivenU<T, T> { }
+	[Immutable] public sealed class ImmutableType_ConditionalInterface_TypeParameters_Met3<[Immutable] T> : Types.SomeImmutableGenericInterfaceGivenTU<T, T> { }
+	[Immutable] public sealed class ImmutableType_ConditionalInterface_KnownTypes_NotMet : Types.SomeImmutableGenericInterfaceGivenT<object, object> { }
+	[Immutable] public sealed class ImmutableType_ConditionalInterface_KnownTypes_Met1 : Types.SomeImmutableGenericInterfaceGivenT<int, int> { }
+	[Immutable] public sealed class ImmutableType_ConditionalInterface_KnownTypes_Met2 : Types.SomeImmutableGenericInterfaceGivenU<int, int> { }
+	[Immutable] public sealed class ImmutableType_ConditionalInterface_KnownTypes_Met3 : Types.SomeImmutableGenericInterfaceGivenTU<int, int> { }
+
+	[Immutable] public sealed class ImmutableType_ConditionalBaseClass_TypeParameters_NotMet<T> : /* TypeParameterIsNotKnownToBeImmutable(T) */ Types.SomeImmutableGenericBaseClassGivenT<T, T> /**/ { }
+	[Immutable] public sealed class ImmutableType_ConditionalBaseClass_TypeParameters_Met1<[Immutable] T> : Types.SomeImmutableGenericBaseClassGivenT<T, T> { }
+	[Immutable] public sealed class ImmutableType_ConditionalBaseClass_TypeParameters_Met2<[Immutable] T> : Types.SomeImmutableGenericBaseClassGivenU<T, T> { }
+	[Immutable] public sealed class ImmutableType_ConditionalBaseClass_TypeParameters_Met3<[Immutable] T> : Types.SomeImmutableGenericBaseClassGivenTU<T, T> { }
+	[Immutable] public sealed class ImmutableType_ConditionalBaseClass_KnownTypes_NotMet : /* NonImmutableTypeHeldByImmutable(class, object,) */ Types.SomeImmutableGenericBaseClassGivenT<object, object> /**/ { }
+	[Immutable] public sealed class ImmutableType_ConditionalBaseClass_KnownTypes_Met1 : Types.SomeImmutableGenericBaseClassGivenT<int, int> { }
+	[Immutable] public sealed class ImmutableType_ConditionalBaseClass_KnownTypes_Met2 : Types.SomeImmutableGenericBaseClassGivenU<int, int> { }
+	[Immutable] public sealed class ImmutableType_ConditionalBaseClass_KnownTypes_Met3 : Types.SomeImmutableGenericBaseClassGivenTU<int, int> { }
+
 	[ Immutable]
 	[ConditionallyImmutable]
 	public sealed class /* ConflictingImmutability(Immutable, ConditionallyImmutable, class) */ AnalyzedClassMarkedWithMultipleImmutabilities1 /**/ { }
@@ -1399,6 +1426,12 @@ public sealed class ImmutableClassImplementingConditionallyImmutable<[Immutable]
 	[ConditionallyImmutable]
 	public sealed class ConditionallyImmutableClassImplementingConditionallyImmutable<[ConditionallyImmutable.OnlyIf] T> : ISomethingConditionallyImmutable<T> { }
 
+	[ConditionallyImmutable]
+	public sealed class /* UnappliedConditionalImmutability(ConsistencyTests.ConditionallyImmutableImplementerWithUnappliedCondition, interface, ConsistencyTests.ISomethingConditionallyImmutable) */ ConditionallyImmutableImplementerWithUnappliedCondition /**/<[ConditionallyImmutable.OnlyIf] T, [ConditionallyImmutable.OnlyIf] U> : ISomethingConditionallyImmutable<T> { }
+
+	[ConditionallyImmutable]
+	public sealed class /* MissingTransitiveImmutableAttribute(ConsistencyTests.ConditionallyImmutableImplementingImmutable, , interface, ConsistencyTests.ISomethingImmutable) */ ConditionallyImmutableImplementingImmutable /**/<[ConditionallyImmutable.OnlyIf] T> : ISomethingImmutable { }
+
 	public sealed class /* MissingTransitiveImmutableAttribute(ConsistencyTests.SadImplementerOfConditionallyImmutable,  (or [ConditionallyImmutable]), interface, ConsistencyTests.ISomethingConditionallyImmutable) */ SadImplementerOfConditionallyImmutable /**/<T> : ISomethingConditionallyImmutable<T> { }
 
 	public partial class PartialClassNeedingImmutable { }