Suggestion: Make all structs immutable

[Joki]

Related to this topic I want to suggest to make all structs immutable.

- With mutable structs it is extremely easy to loss data, when e.g. users accidently modify a copy of a struct instead of the structs
- Immutable structs are automatically thread safe by nature
- Users will no longer forget to initialize some of the fields
- There would be almost no performance penalty, as structs (unlike as in Java) are extremely light-weight objects

For reference:
Microsoft Advice on Struct Design

Burning monk on structs

Eric Lippert (Designer of the C# language and the CLR) about mutable structs

Eric Lippert - value types may be created on the stack (extremely light-weight to create)

[Nexus]

Is that a good idea for all structs? E.g. Transform or FloatRect, which are modified in-place. Their methods would need to be adjusted to return copies, and maybe additional ones have to be provided to keep usage still convenient.

[Laurent]

To me this would be very inconvenient. Not to mention that we are very constrained by the fact that these structures must directly map to the C API ones.

So, I'd like to see this suggestion in action if you have some time

[zsbzsb]

No, no, and no again.

This will break lots of existing code and cause more issues than the two we already have.

Issue #1

// default constructor
RenderStates states = new RenderStates();

Issue #2 (not really an issue as any C# dev should understand this)

// compiler error, trying to modify temporary object
vertexarray[0].Color = Color.Red;

With mutable structs it is extremely easy to loss data, when e.g. users accidently modify a copy of a struct instead of the structs

This isn't the case anywhere with the SFML.NET bindings, not to mention any decent C# dev will know that modifying value types that have been copied into methods won't change it outside of the method. Or a quick google search will bring this up.

Immutable structs are automatically thread safe by nature

Doesn't apply to SFML as we aren't trying to be thread safe with the public API.

Users will no longer forget to initialize some of the fields

How so? It doesn't force them to use the non-default constructor and all non-default constructors initialize everything with default values. Making structs immutable will require us to now provide every single possible constructor since the user now can't change values after constructing an object. Just think of vectors, to change a vector's value you will now need to create a new vector every single time you want to modify it instead of just changing the existing value.

turn the fields into properties, and have the setter return a new instance of the modified struct.

What fields into properties?

I'm at work now, but if this isn't enough to convince you that this isn't right for SFML.NET I will gladly pull out more specific code examples that will break if we make stuff immutable in the binding.

[Joki]

This will break lots of existing code and cause more issues than the two we already have.

To me, this is the only valid objection, as this is - of course - a breaking change.

not to mention any decent C# dev will know that modifying value types that have been copied into methods won't change it outside of the method. Or a quick google search will bring this up.

This logic perplexes me. So, if any "decent dev knows" / "it is easy to google for" how 'abc' works, so why not keep forcing the user to use 'def' instead, which is tons more error-prone.

How so? It doesn't force them to use the non-default constructor and all non-default constructors initialize everything with default values.

Look for example as the link why got me to create this post at the very top. It _is_ easy to overlook.

Edit: removed links to constructor chaining and intialization syntax, since zsbzsb was talking about time of creation of the type. My bad.

Making structs immutable will require us to now provide every single possible constructor
Just think of vectors, to change a vector's value you will now need to create a new vector every single time you want to modify it instead of just changing the existing value.

Thats were Properties in C# come in.

The majority of books on this subject suggest to implement _all_ public members (with exception for public static or read-only value types or ReadOnlyCollections) as Properties.
Not only will they allow total control of access on the underlaying value, also modifying the logic when accessing its value will never be a breaking change for existing code.

And you don't need to worry for performance either. ValueTypes are extremely light-weight, and if used locally they are created on the stack even. Now will replacing the fields by properties be a performance drawback in Release builds, since the properties will be inlined.

[zsbzsb]

To me, this is the only valid objection, as this is - of course - a breaking change.

Why break anything if we aren't fixing anything? I already pointed out the only 2 issues we currently have the your proposed changes don't fix anything related to them.

This logic perplexes me. So, if any "decent dev knows" / "it is easy to google for" how 'abc' works, so why not keep forcing the user to use 'def' instead, which is tons more error-prone.

If you have to pass a value type into a method and then expect to get the changes back, you probably have a design issue and there is probably a much better way to do what you are trying to do. Not to mention that adding properties / marking immutable doesn't fix this in any way whatsoever.

Thats were Properties in C# come in.

The majority of books on this subject suggest to implement _all_ public members (with exception for public static or read-only value types or ReadOnlyCollections) as Properties.
Not only will they allow total control of access on the underlaying value, also modifying the logic when accessing its value will never be a breaking change for existing code.

Properties do not behave like you think when it comes to structs. You still have the problem of modifying a copy of a returned struct and default constructors. Look at the following code to see what I mean.

(click to show/hide)

struct MyStruct
    {
        private int _value;
        public MyStruct(int Value)
        {
            _value = Value;
        }
        public int Value
        {
            get
            {
                return _value;
            }
            set
            {
                _value = value;
            }
        }
    }

    class Foo
    {
        private MyStruct _struct;
        public MyStruct Struct
        {
            get
            {
                return _struct;
            }
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            var m = new Foo();
            // THIS STILL WON'T WORK
            m.Struct.Value = 5;
        }
       
    }

So please, if we mark all the fields in the structs as readonly and provide properties, what exactly (give me a code example) does marking structs immutable fix?

[Laurent]

I feel like this discussion could last forever, and personnally since I'm not a C# programmer, your arguments are not really convincing.

So what I suggest is: implement your idea for a simple type (Vertex for example), and show (with code) how it solves the initial problems, and how it doesn't create more.

[Joki]

/// <summary>
/// Define a point with color and texture coordinates
/// </summary>
[StructLayout(LayoutKind.Sequential)]
public struct Vertex
{
        /// <summary>
        /// Construct the vertex from its position
        /// The vertex color is white and texture coordinates are (0, 0).
        /// </summary>
        /// <param name="position">Vertex position</param>
        public Vertex(Vector2f position) :
                this(position, Color.White, new Vector2f(0, 0))
        {
        }

        /// <summary>
        /// Construct the vertex from its position and color
        /// The texture coordinates are (0, 0).
        /// </summary>
        /// <param name="position">Vertex position</param>
        /// <param name="color">Vertex color</param>
        public Vertex(Vector2f position, Color color) :
                this(position, color, new Vector2f(0, 0))
        {
        }

        /// <summary>
        /// Construct the vertex from its position and texture coordinates
        /// The vertex color is white.
        /// </summary>
        /// <param name="position">Vertex position</param>
        /// <param name="texCoords">Vertex texture coordinates</param>
        public Vertex(Vector2f position, Vector2f texCoords) :
                this(position, Color.White, texCoords)
        {
        }

        /// <summary>
        /// Construct the vertex from its position, color and texture coordinates
        /// </summary>
        /// <param name="position">Vertex position</param>
        /// <param name="color">Vertex color</param>
        /// <param name="texCoords">Vertex texture coordinates</param>
        public Vertex(Vector2f position, Color color, Vector2f texCoords)
        {
                this.position = position;
                this.color = color;
                this.texCoords = texCoords;
        }

        /// <summary>
        /// Provide a string describing the object
        /// </summary>
        /// <returns>String description of the object</returns>
        public override string ToString()
        {
                return "[Vertex]" +
                                " Position(" + Position + ")" +
                                " Color(" + Color + ")" +
                                " TexCoords(" + TexCoords + ")";
        }

        /// <summary>2D position of the vertex</summary>
        public Vector2f Position { get { return Position; } }
        private readonly Vector2f position;

        /// <summary>Color of the vertex</summary>
        public Color Color { get { return color; } }
        private readonly Color color;

        /// <summary>Coordinates of the texture's pixel to map to the vertex</summary>
        public Vector2f TexCoords { get { return texCoords; } }
        private readonly Vector2f texCoords;
}



class Program
{
        static void Main(string[] args)
        {
                // oops, accidently called default ctor on immutable struct
                Vertex badVertex = new Vertex();
                // notice at compile time
                Console.WriteLine(badVertex);
                badVertex.Color = Color.Red;
                Console.WriteLine(badVertex);

                // hey, thats fine.
                Vertex vertex = new Vertex(new Vector2f(50, 50), Color.Black);
        }
}
 

The advantages were mentioned, as suggested by Eric Lippert, Microsoft, and every serious book on topic. A value type should just be used as a container to represent values, that are not changable in any way.

I am not willing to get into a flame war with you, zsbzsb.

It was a suggestion.

If you don't like it, don't do it.
If you like it, do it.
If you are unsure, use that well-known search engine what the developers of the C# language and CLR's take is on this issue.

[krzat]

XNA has mutable struct for a reason (despite being Microsoft's product).

These guidelines may make sense for "enterprisey" programming, where people barely use value types and may be bitten by their behaviour. IN SFML's context, they are used as simple data holders and should stay this way. I can't imagine what a hell manipulating FloatRect would be.

btw. you should benchmark your Vertex (after fixing stack overflow) to see if it really is that lightweight. Probably not.

[Laurent]

I still believe that it would be very inconvenient in the context of SFML. The Vertex class has 3 members and there are already a lot of constructors to create, imagine the RnederStates class with its 4 members (and possibly more in the future), it's just impossible to create a constructor for every combination of members.

And what do properties add, compared to making the members public directly?

[Joki]

@Laurent: The thing that properties add compared to fields are mainly better encapsulation and changes to Vertex would never be breaking code on the consumer side, since you can just modify what the property does.

Well, i did some performance testing. Obviously the immutable Vertex is slower by longitudes.

On Ideone (scroll to the very bottom for result times) it ran about 3 times slower, however on my machine (SFML 2.1, VS 2010 Ultimate, X86 Release Build) it ran slower by a factor of 35.

Now i think i am no longer willing to sacrifice THAT much performance for having immutable value types.
Maybe some good practices and business rules don't apply to graphic libs.

[Laurent]

@Laurent: The thing that properties add compared to fields are mainly better encapsulation and changes to Vertex would never be breaking code on the consumer side, since you can just modify what the property does.

This is valid as a general consideration. But here it's a direct binding to a C structure, there's no point encapsulating things more than they are in the underlying library. And we can still add properties later without breaking anything, if we really need to.

You didn't answer the "huge number of constructors" argument, so I guess you also agree

Maybe some good practices and business rules don't apply to graphic libs.

Things can be very different, indeed, because:
- your code can be executed as often as 60 times per second, which is unlikely to happen in other types of applications
- your design and data structures must not differ too much from what's expected by the underlying drawing API (OpenGL), otherwise you'll have too many adaptations and conversions which can also ruin performances

[Joki]

You didn't answer the "huge number of constructors" argument, so I guess you also agree 

Code: [Select]

RenderStates r = new RenderStates { BlendMode = BlendMode.Add, Texture = new Texture(@"blah.png") };

You don't need to add any new ctor overload, if you use initalization syntax.


Well, I guess, keep it as it is.

[Laurent]

I didn't know it was possible in C#, thanks.

[zsbzsb]

Code: [Select]

RenderStates r = new RenderStates { BlendMode = BlendMode.Add, Texture = new Texture(@"blah.png") };

You don't need to add any new ctor overload, if you use initalization syntax.

This doesn't solve the issue of needing a large number of constructors. Because for you to use this syntax there is 2 requirements: what you are assigning is a property (not a plain old variable), and that the property can be assigned (and it won't if the struct is immutable as the property can't have a setter).