Merge pull request #82 from AArnott/fix80

Avoid using == operator on structs that do not define them

Author: AArnott

src/ImmutableObjectGraph.Generation.Tests/ImmutableObjectGraph.Generation.Tests.csproj

@@ -66,6 +66,10 @@
       <Generator>MSBuild:GenerateCodeFromAttributes</Generator>
     </Compile>
     <Compile Include="TestSources\ImmutableDictionaryHelpers.Tests.cs" />
+    <Compile Include="TestSources\ImmutableWithComplexStructField.cs">
+      <Generator>MSBuild:GenerateCodeFromAttributes</Generator>
+    </Compile>
+    <Compile Include="TestSources\ImmutableWithComplexStructField.Tests.cs" />
     <Compile Include="TestSources\MSBuild.cs">
       <Generator>MSBuild:GenerateCodeFromAttributes</Generator>
     </Compile>

src/ImmutableObjectGraph.Generation.Tests/TestSources/ImmutableWithComplexStructField.Tests.cs

@@ -0,0 +1,64 @@
+namespace ImmutableObjectGraph.Generation.Tests.TestSources
+{
+    using System;
+    using System.Collections.Generic;
+    using System.Linq;
+    using System.Text;
+    using System.Threading.Tasks;
+    using Xunit;
+
+    public class ImmutableWithComplexStructFieldTests
+    {
+        [Fact]
+        public void StructWithoutOperatorsAlwaysRecreatesObjectWithChangedValue()
+        {
+            var originalStruct = new SomeStructWithMultipleFields(5, 0);
+            var structWithModifiedSecondField = new SomeStructWithMultipleFields(5, 1);
+            var v1 = ImmutableWithComplexStructField.Create(someStructField: originalStruct);
+            var v2 = v1.With(someStructField: structWithModifiedSecondField);
+            Assert.Equal(structWithModifiedSecondField.Field2, v2.SomeStructField.Field2);
+        }
+
+        [Fact]
+        public void StructWithoutOperatorsAlwaysRecreatesObjectWithSameValue()
+        {
+            var s1 = new SomeStructWithMultipleFields(1, 2);
+            var v1 = ImmutableWithComplexStructField.Create(someStructField: s1);
+            var v2 = v1.With(someStructField: s1);
+
+            // The object should have been recreated since equality between the two struct values
+            // cannot be determined without their operator defined.
+            Assert.NotSame(v1, v2);
+            Assert.Equal(s1.Field1, v2.SomeStructField.Field1);
+        }
+
+        [Fact(Skip = "Not yet passing (#81).")]
+        public void StructWithoutOperatorsAlwaysRecreatesObjectWithoutValue()
+        {
+            var s1 = new SomeStructWithMultipleFields(1, 2);
+            var v1 = ImmutableWithComplexStructField.Create(someStructField: s1);
+            var v2 = v1.With(); // omit the struct value altogether
+            Assert.Same(v1, v2);
+        }
+
+        [Fact]
+        public void StructWithOperatorsRecreatesObjectWithChangedValue()
+        {
+            var s12 = new SomeStructWithMultipleFieldsAndOperator(1, 2);
+            var s13 = new SomeStructWithMultipleFieldsAndOperator(1, 3);
+            var v1 = ImmutableWithComplexStructField.Create(someStructFieldWithOperator: s12);
+            var v2 = v1.With(someStructFieldWithOperator: s13);
+            Assert.NotSame(v1, v2);
+            Assert.Equal(s13.Field2, v2.SomeStructFieldWithOperator.Field2);
+        }
+
+        [Fact(Skip = "Not yet passing (#81).")]
+        public void StructWithOperatorsRecyclesObjectWithSameValue()
+        {
+            var s12 = new SomeStructWithMultipleFieldsAndOperator(1, 2);
+            var v1 = ImmutableWithComplexStructField.Create(someStructFieldWithOperator: s12);
+            var v2 = v1.With(someStructFieldWithOperator: s12);
+            Assert.Same(v1, v2);
+        }
+    }
+}

src/ImmutableObjectGraph.Generation.Tests/TestSources/ImmutableWithComplexStructField.cs

@@ -0,0 +1,58 @@
+namespace ImmutableObjectGraph.Generation.Tests.TestSources
+{
+    using System;
+
+    [GenerateImmutable]
+    partial class ImmutableWithComplexStructField
+    {
+        readonly SomeStructWithMultipleFields someStructField;
+
+        readonly SomeStructWithMultipleFieldsAndOperator someStructFieldWithOperator;
+    }
+
+    struct SomeStructWithMultipleFields
+    {
+        internal SomeStructWithMultipleFields(int field1, int field2)
+        {
+            this.Field1 = field1;
+            this.Field2 = field2;
+        }
+
+        internal int Field1 { get; }
+
+        internal int Field2 { get; }
+    }
+
+    struct SomeStructWithMultipleFieldsAndOperator
+    {
+        internal SomeStructWithMultipleFieldsAndOperator(int field1, int field2)
+        {
+            this.Field1 = field1;
+            this.Field2 = field2;
+        }
+
+        internal int Field1 { get; }
+
+        internal int Field2 { get; }
+
+        public static bool operator ==(SomeStructWithMultipleFieldsAndOperator one, SomeStructWithMultipleFieldsAndOperator two)
+        {
+            return one.Field1 == two.Field1 && one.Field2 == two.Field2;
+        }
+
+        public static bool operator !=(SomeStructWithMultipleFieldsAndOperator one, SomeStructWithMultipleFieldsAndOperator two)
+        {
+            return !(one == two);
+        }
+
+        public override bool Equals(object obj)
+        {
+            throw new NotImplementedException();
+        }
+
+        public override int GetHashCode()
+        {
+            throw new NotImplementedException();
+        }
+    }
+}

src/ImmutableObjectGraph.Generation/CodeGen.cs

@@ -210,6 +210,35 @@ private static IdentifierNameSyntax GetGenerationalMethodName(IdentifierNameSynt
             return SyntaxFactory.IdentifierName(baseName.Identifier.ValueText + generation.ToString(CultureInfo.InvariantCulture));
         }
 
+        /// <summary>
+        /// Checks whether a type defines equality operators for itself.
+        /// </summary>
+        /// <param name="symbol">The type to check.</param>
+        /// <returns><c>true</c> if the == and != operators are defined on the type.</returns>
+        private static bool HasEqualityOperators(ITypeSymbol symbol)
+        {
+            Requires.NotNull(symbol, nameof(symbol));
+
+            if (symbol.IsReferenceType)
+            {
+                // Reference types inherit their equality operators from System.Object.
+                return true;
+            }
+
+            if (symbol.SpecialType != SpecialType.None)
+            {
+                // C# knows how to run equality checks for special (built-in) types like int.
+                return true;
+            }
+
+            var equalityOperators = from method in symbol.GetMembers().OfType<IMethodSymbol>()
+                                    where method.MethodKind == MethodKind.BuiltinOperator || method.MethodKind == MethodKind.UserDefinedOperator
+                                    where method.Parameters.Length == 2 && method.Parameters.All(p => p.Type == symbol)
+                                    where method.Name == "op_Equality"
+                                    select method;
+            return equalityOperators.Any();
+        }
+
         private void ReportDiagnostic(string id, SyntaxNode blamedSyntax, params string[] formattingArgs)
         {
             Requires.NotNull(blamedSyntax, nameof(blamedSyntax));
@@ -533,18 +562,20 @@ private IEnumerable<MethodDeclarationSyntax> CreateWithCoreMethods()
         private MemberDeclarationSyntax CreateWithFactoryMethod()
         {
             // (field.IsDefined && field.Value != this.field)
-            Func<IdentifierNameSyntax, IdentifierNameSyntax, ExpressionSyntax> isChangedByNames = (propertyName, fieldName) =>
-                SyntaxFactory.ParenthesizedExpression(
-                    SyntaxFactory.BinaryExpression(
-                        SyntaxKind.LogicalAndExpression,
-                        Syntax.OptionalIsDefined(fieldName),
+            Func<IdentifierNameSyntax, IdentifierNameSyntax, ITypeSymbol, ExpressionSyntax> isChangedByNames = (propertyName, fieldName, fieldType) =>
+                fieldType == null || HasEqualityOperators(fieldType) ?
+                    (ExpressionSyntax)SyntaxFactory.ParenthesizedExpression(
                         SyntaxFactory.BinaryExpression(
-                            SyntaxKind.NotEqualsExpression,
-                            Syntax.OptionalValue(fieldName),
-                            Syntax.ThisDot(propertyName))));
-            Func<MetaField, ExpressionSyntax> isChanged = v => isChangedByNames(v.NameAsProperty, v.NameAsField);
+                            SyntaxKind.LogicalAndExpression,
+                            Syntax.OptionalIsDefined(fieldName),
+                            SyntaxFactory.BinaryExpression(
+                                SyntaxKind.NotEqualsExpression,
+                                Syntax.OptionalValue(fieldName),
+                                Syntax.ThisDot(propertyName)))) :
+                    Syntax.OptionalIsDefined(fieldName);
+            Func<MetaField, ExpressionSyntax> isChanged = v => isChangedByNames(v.NameAsProperty, v.NameAsField, v.Symbol.Type);
             var anyChangesExpression =
-                new ExpressionSyntax[] { isChangedByNames(IdentityPropertyName, IdentityParameterName) }.Concat(
+                new ExpressionSyntax[] { isChangedByNames(IdentityPropertyName, IdentityParameterName, null) }.Concat(
                     this.applyToMetaType.AllFields.Select(isChanged))
                     .ChainBinaryExpressions(SyntaxKind.LogicalOrExpression);
 

src/ImmutableObjectGraph/Optional.cs

@@ -1,11 +1,9 @@
-namespace ImmutableObjectGraph {
-	using System;
-	using System.Collections.Generic;
-	using System.Linq;
-	using System.Text;
-	using System.Threading.Tasks;
+namespace ImmutableObjectGraph
+{
+	using System.Diagnostics;
 
 	public static class Optional {
+		[DebuggerStepThrough]
 		public static Optional<T> For<T>(T value) {
 			return value;
 		}

src/ImmutableObjectGraph/Optional`1.cs

@@ -57,6 +57,7 @@ public static implicit operator Optional<T>(T value) {
 		/// </summary>
 		/// <param name="defaultValue">The default value to use if a value was not specified.</param>
 		/// <returns>The value.</returns>
+		[DebuggerStepThrough]
 		public T GetValueOrDefault(T defaultValue) {
 			return this.IsDefined ? this.value : defaultValue;
 		}