Brightspace / D2L.CodeStyle

Annotations and analysis tools for D2L C# code style
Apache License 2.0
10 stars 22 forks source link

[ConditionallyImmutable] is unsound #956

Closed j3parker closed 2 months ago

j3parker commented 2 months ago
[ConditionallyImmutable]
interface IOne<[ConditionallyImmutable.OnlyIf]A> {}

[ConditionallyImmutable]
interface ITwo<[ConditionallyImmutable.OnlyIf]A, [ConditionallyImmutable.OnlyIf]B> : IOne<A>{}

record Impl(int X, List<int> Y) : ITwo<int, List<int>>;

An IOne<A> is immutable when its type argument A is immutable (otherwise it is not). An ITwo<A, B> likewise is immutable when both of its type arguments are immutable.

However, ITwo<A, B> also extends IOne<A> which will cause a problem:

Impl implements ITwo<int, List<int>>. It is never judged (itself) to be immutable, because it's Y property is a mutable List<int>.

If you refer to it as an ITwo<int, List<int>> it will not be considered immutable due to the OnlyIf on ITwo's B type parameter.

However, every ITwo<int, List<int>> is an IOne<int>, which is always considered to be immutable, which is problematic.

We could detect cases from one immutable type to another to cover many of the consequences of this, but I don't think that would be sufficient. The smuggling could happen outside of code we are able to analyze (e.g. in the .NET standard library), e.g. via covariance on wrapper classes (ISomeInnocentThing<out T> being able to turn an ISomeInnocentThing<ITwo<int, List<int>>> into an ISomeInnocentThing<IOne<int>>) or even worse a method (e.g. TReturn SomeInnocentThing<TInput, TReturn>( TInput input ) internally having casting).

Prior to ConditionallyImmutable we have accepted that we can't scan all code (e.g. the standard library) but we still want reasonable guarantees -- so I worry that hunting for cases like casts takes us far away from that.

One idea is to ban ITwo<[OnlyIf]A, [OnlyIf]B> from implementing IOne<[OnlyIf]A> on the rationale that it is meaningfully "less immutable". It's immutability is qualified on things that IOne<A>'s isn't, and so it makes sense to complain. The exact rules may need some thinking. To make a more concrete and slightly more complicated case:

[ConditionallyImmutable]
interface IReadOnlyCollection<[ConditionallyImmutable.OnlyIf] T> {}

[ConditionallyImmutable]
class KeyValuePair<[ConditionallyImmutable.OnlyIf] TKey, [ConditionallyImmutable.OnlyIf] TValue> {}

[ConditionallyImmutable]
interface IReadOnlyDictionary<[ConditionallyImmutable.OnlyIf] TKey, [ConditionallyImmutable.OnlyIf] TValue>
  : IReadOnlyCollection<KeyValuePair<TKey, TValue>> {}

This one ought to work because every e.g. every IReadOnlyDictionary<int, int> is immutable and so is IReadOnlyCollection<KeyValuePair<int, int>>... but IReadOnlyDictionary<int, object> looks not-immutable, but so does IReadOnlyCollection<KeyValuePair<int, object>> because KeyValuePair<int, object> is not immutable...