From bc72a4ecd672fc94deb0d5aed157d6062fe1f575 Mon Sep 17 00:00:00 2001 From: Owen Smith Date: Fri, 9 Aug 2024 15:48:57 -0400 Subject: [PATCH] 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: https://github.com/Brightspace/D2L.CodeStyle/issues/956 Co-authored-by: Jacob Parker --- src/D2L.CodeStyle.Analyzers/Diagnostics.cs | 9 ++ .../Immutability/ImmutabilityAnalyzer.cs | 3 +- .../ImmutableAttributeConsistencyChecker.cs | 120 ++++++++++++------ .../Immutability/ImmutableTypeInfo.cs | 7 +- .../Specs/ImmutabilityAnalyzer.cs | 33 +++++ 5 files changed, 130 insertions(+), 42 deletions(-) diff --git a/src/D2L.CodeStyle.Analyzers/Diagnostics.cs b/src/D2L.CodeStyle.Analyzers/Diagnostics.cs index bd1d14f3..1c4053e6 100644 --- 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 index fd535028..9c10fd29 100644 --- 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 m_additionalImmutableTypes; diff --git a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs index 0e410bf3..b7d92bda 100644 --- 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 index 813fee6b..82865ae6 100644 --- a/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableTypeInfo.cs +++ b/src/D2L.CodeStyle.Analyzers/Immutability/ImmutableTypeInfo.cs @@ -23,13 +23,18 @@ ImmutableArray 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 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 index 2646bd9b..84f12fd0 100644 --- 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 { } + [ConditionallyImmutable] + public abstract class SomeImmutableGenericBaseClassGivenT<[ConditionallyImmutable.OnlyIf] T, U> { } + + [ConditionallyImmutable] + public abstract class SomeImmutableGenericBaseClassGivenU { } + + [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 : Types.SomeImmutableGenericClassRestrictingT { } + [Immutable] public sealed class ImmutableType_ConditionalInterface_TypeParameters_NotMet : Types.SomeImmutableGenericInterfaceGivenT { } + [Immutable] public sealed class ImmutableType_ConditionalInterface_TypeParameters_Met1<[Immutable] T> : Types.SomeImmutableGenericInterfaceGivenT { } + [Immutable] public sealed class ImmutableType_ConditionalInterface_TypeParameters_Met2<[Immutable] T> : Types.SomeImmutableGenericInterfaceGivenU { } + [Immutable] public sealed class ImmutableType_ConditionalInterface_TypeParameters_Met3<[Immutable] T> : Types.SomeImmutableGenericInterfaceGivenTU { } + [Immutable] public sealed class ImmutableType_ConditionalInterface_KnownTypes_NotMet : Types.SomeImmutableGenericInterfaceGivenT { } + [Immutable] public sealed class ImmutableType_ConditionalInterface_KnownTypes_Met1 : Types.SomeImmutableGenericInterfaceGivenT { } + [Immutable] public sealed class ImmutableType_ConditionalInterface_KnownTypes_Met2 : Types.SomeImmutableGenericInterfaceGivenU { } + [Immutable] public sealed class ImmutableType_ConditionalInterface_KnownTypes_Met3 : Types.SomeImmutableGenericInterfaceGivenTU { } + + [Immutable] public sealed class ImmutableType_ConditionalBaseClass_TypeParameters_NotMet : /* TypeParameterIsNotKnownToBeImmutable(T) */ Types.SomeImmutableGenericBaseClassGivenT /**/ { } + [Immutable] public sealed class ImmutableType_ConditionalBaseClass_TypeParameters_Met1<[Immutable] T> : Types.SomeImmutableGenericBaseClassGivenT { } + [Immutable] public sealed class ImmutableType_ConditionalBaseClass_TypeParameters_Met2<[Immutable] T> : Types.SomeImmutableGenericBaseClassGivenU { } + [Immutable] public sealed class ImmutableType_ConditionalBaseClass_TypeParameters_Met3<[Immutable] T> : Types.SomeImmutableGenericBaseClassGivenTU { } + [Immutable] public sealed class ImmutableType_ConditionalBaseClass_KnownTypes_NotMet : /* NonImmutableTypeHeldByImmutable(class, object,) */ Types.SomeImmutableGenericBaseClassGivenT /**/ { } + [Immutable] public sealed class ImmutableType_ConditionalBaseClass_KnownTypes_Met1 : Types.SomeImmutableGenericBaseClassGivenT { } + [Immutable] public sealed class ImmutableType_ConditionalBaseClass_KnownTypes_Met2 : Types.SomeImmutableGenericBaseClassGivenU { } + [Immutable] public sealed class ImmutableType_ConditionalBaseClass_KnownTypes_Met3 : Types.SomeImmutableGenericBaseClassGivenTU { } + [ 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 { } + [ConditionallyImmutable] + public sealed class /* UnappliedConditionalImmutability(ConsistencyTests.ConditionallyImmutableImplementerWithUnappliedCondition, interface, ConsistencyTests.ISomethingConditionallyImmutable) */ ConditionallyImmutableImplementerWithUnappliedCondition /**/<[ConditionallyImmutable.OnlyIf] T, [ConditionallyImmutable.OnlyIf] U> : ISomethingConditionallyImmutable { } + + [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 /**/ : ISomethingConditionallyImmutable { } public partial class PartialClassNeedingImmutable { }