immutability fixes #958

omsmith · 2024-08-09T14:42:11Z

require application of all [ConditionallyImmutable.OnlyIf] type paramters
a [ConditionallyImmutable] type should not be able to extend/implement an [Immutable] one

Fixes: #956

* require application of all [ConditionallyImmutable.OnlyIf] type paramters * a [ConditionallyImmutable] type should not be able to extend/implement an [Immutable] one Fixes: #956

j3parker · 2024-08-09T14:52:59Z

src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs

+								for( int i = 0; i < baseTypeInfo.Type.TypeArguments.Length && !parameterUsed; i++ ) {
+									ITypeSymbol typeArgument = baseTypeInfo.Type.TypeArguments[ i ];
+									if( !SymbolEqualityComparer.Default.Equals( typeParameter, typeArgument ) ) {
+										continue;


So this would emit a diagnostic for IMyReadOnlyDictionary right?

[ConditionallyImmutable] class MyKeyValuePair< [ConditionallyImmutable.OnlyIf]TKey, [ConditionallyImmutable.OnlyIf]TValue > {} [ConditionallyImmutable] interface IMyReadOnlyCollection< [ConditionallyImmutable.OnlyIf]T > {} [ConditionallyImmutable] interface IMyReadOnlyDictionary< [ConditionallyImmutable.OnlyIf]TKey, [ConditionallyImmutable.OnlyIf]TValue > : IMyReadOnlyCollection<KeyValuePair<TKey, TValue>> {}

because TKey "isn't used" in the type arguments to IMyReadOnlyCollection, right?

Yeah, it would

You thinking a recursive search?

Ok -- you agree that the above is safe/we would like it to be accepted in an ideal world, right?

Not saying its a problem with this PR, just wrapping my head around the issue

Ok -- you agree that the above is safe/we would like it to be accepted right?

Yep

You thinking a recursive search?

Something like that but I think it's a maybe a little fancy.

I think what we want to know is, are we (IMyReadOnlyDictionary) "at least" as immutable as all of the interfaces (base class) we implement (derive from)? "At least" meaning that any time the broader type gets judged as immutable we also do...

I think the reason its safe here is IMyReadOnlyCollection<T> is only immutable when T is, ok so recursively we need to care about when KeyValuePair<TKey, TValue> is... again recursively we look at that and see we have conditionals on TKey and TValue....

If we accumulate all those conditionals then I think we want to say OK, are the set of conditionals from my equal to (or a superset of?) the conditionals from my type parameters?

I'm not quite sure though, still thinking about it.

omsmith · 2024-08-09T14:59:22Z

Hmm, something is broken

j3parker · 2024-08-09T19:15:11Z

src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs

+						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;
+							}
+						}


We talked on Zoom about how this chunk could be a helper and that helper would change if we implemented the more powerful logic here (recursive/whatever)

j3parker

We talked on zoom about reorganizing this a bit (to cut down on state/nesting) but didn't get anywhere fantastic
EDIT: we're looking at a big tuple switch and that looks promising; if it pans out this approval still stands

src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs

Co-authored-by: Jacob Parker <[email protected]>

…sistencyChecker.cs Co-authored-by: Jacob Parker <[email protected]>

src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs

j3parker · 2024-08-09T19:37:42Z

src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs

+								baseTypeInfo.Type.GetFullTypeName()
+							)
+						);
+						return;


We talked about how we could drop this return if the diagnostic was on typeParameter instead, like we could emit multiple diagnostics instead. I dno, consider it since this is a new diagnostic anyway

src/D2L.CodeStyle.Analyzers/Immutability/ImmutableAttributeConsistencyChecker.cs

…sistencyChecker.cs Co-authored-by: Jacob Parker <[email protected]>

immutability fixes

80275b8

* require application of all [ConditionallyImmutable.OnlyIf] type paramters * a [ConditionallyImmutable] type should not be able to extend/implement an [Immutable] one Fixes: #956

omsmith requested a review from j3parker as a code owner August 9, 2024 14:42