Records should not have members with mutable types

[MelbourneDeveloper]

Describe the problem you are trying to solve

C# 9 introduces records. This is good, but it is easy to make a mistake and add members that have types that are mutable.

Check this code out. It runs on .NET Core 3.1 with LangVersion 9.0. For programmers who want actual immutability, this is a problem.

using System;

namespace ConsoleApp
{
    class Program
    {
        static void Main(string[] args)
        {
            var record = new Record("Bob", new Mutable { Name = "Jim" });
            record.Mutable.Name = "Oops";
            Console.WriteLine($"Oh no! The property was changed to {record.Mutable.Name}");
        }
    }

    public record Record
    (
        string Name,
        Mutable Mutable
    );

    public class Mutable
    {
        public string Name { get; set; }
    }
}

namespace System.Runtime.CompilerServices { public class IsExternalInit { } }

Describe suggestions on how to achieve the rule

Recursively check all types of members to make sure that the types are also immutable.

Additional context

This is about immutability and the functional programming paradigm.

[Evangelink]

I like the idea but I am wondering how far we want to go with this new analyzer in term of detecting mutation. In your example that's pretty easy there is a public setter but for example do you consider any of these to be mutable:

public class C1
{
    private int _counter;

    public void M()
	{
		// ...
		_counter++;
		// ...
	}
}

public class C2
{
	private readonly List<int> _list = new List<int>();

	public void M()
	{
		// ...
		var result = GetSomething();
		_list.Add(result);
		// ...
	}
}

[MelbourneDeveloper]

@Evangelink I think both would be considered mutable.

It might be worth adding two levels.

One level would allow potentially mutable types - i.e. Types that don't have public setters to change properties

The stronger level would enforce that all other types must be record types. You would probably need to check that each field is read-only for that.

[Youssef1313]

When using records, you should never assume it's immutable. I think it's okay to use them even if they are mutable.

If this rule is going to be implemented, I think it should be disabled by default.

[MelbourneDeveloper]

@Youssef1313 the whole point of records is immutability.

I'm not sure what you mean by enabling/disabling by default though...

[Youssef1313]

@Youssef1313 the whole point of records is immutability.

It's not really the whole point. The whole point of records is value equality semantics.

[MelbourneDeveloper]

@Youssef1313 which is dependent on immutability

[Youssef1313]

I don't think so. There is another language proposal for immutability. See dotnet/csharplang#3885.

However, I won't be very opposed if this is implemented. But should be disabled by default IMO (i.e. user should manually enable it through editorconfig)

[Evangelink]

Quoting the record types documentation page:

Record types make it easy to create immutable reference types in .NET. Historically, .NET types are largely classified as reference types (including classes and anonymous types) and value types (including structs and tuples). While immutable value types are recommended, mutable value types don’t often introduce errors. Value type variables hold the values so changes are made to a copy of the original data when value types are passed to methods.

You are somehow both rights. I mean this is meant to be an easy way to implement immutable types with semantic equality BUT you are not forced to use it purely as immutable object.

If this rule is going to be implemented, I think it should be disabled by default.

Yes, of course. @MelbourneDeveloper "disabled by default" means that when using the analyzer package, this specific rule is not active (ON/enabled) by default and that you manually have to opt-in for it. This is what is usually done for personal preference rules.

[Youssef1313]

@Evangelink There is an existing issue in dotnet/docs to update the paragraph you quoted. While records may make it easier to implement immutable types, it's not its main goal. And again, I'm not opposed to the rule proposal, but just noting a common misunderstanding about records.

[MelbourneDeveloper]

@Evangelink there must be something I'm missing. I've used FxCop with . ruleset files and recently I've switched to editor config files. Both are opt in. I don't understand how this would be any different.

[MelbourneDeveloper]

@Youssef1313 @Evangelink

Just incidentally, it's unfortunate that C# records are not compatible with F# records. If you use dnSpy to convert the IL back to C# record types are different between C# and F#.

For my two cents, I think having strong rules that you can opt in to are important for record types. If we want to get serious about functional programming with C#, the analyzers need to pick up the work that the language itself doesn't doesn't do.

[MelbourneDeveloper]

@Evangelink where is this configured?

this specific rule is not active (ON/enabled) by default and that you manually have to opt-in for it

I use the editor config these days and I have to opt in to the rules I want. What have I missed?

[Evangelink]

Some rules (e.g. CA1831) are enabled by default which means that when you install the nuget package, without any manual modification (ruleset or editorconfig) the rule will analyze your code. For others, you will have to manually enable the rule (ruleset or editorconfig). Hope this is clearer.