Merge pull request #674 from SteveDunn/nullability-mismatch-on-overri…

…dden-member

Nullability mismatch on overridden member

docs/site/Writerside/topics/reference/Differences-when-using-records.md

@@ -1,14 +1,9 @@
 # Differences with records
 
-TL;DR: there are significant differences, and it is best to stick to a `class` or `struct` rather than records as the benefits of records don't apply to Vogen where a single primitive value is being wrapped and protected.
+There are significant differences, and it is best to stick to a `class` or `struct` rather than records as the benefits of records don't apply to Vogen where a single primitive value is being wrapped and protected.
 
-For classes and structs, Vogen generates a lot of boilerplate code. But for records, some of this boilerplate code is 
-already generated. This page lists the differences between records (classes and structs) and non-record classes and structs.
+For classes and structs, Vogen generates a lot of boilerplate code. But for records, some of this boilerplate code is already generated. This page lists the differences between records (classes and structs) and non-record classes and structs.
 
-* the generated code for records have an `init` accessibility on the `Value` property to support `with`, 
-e.g. `var vo2 = vo1 with { Value = 42 }` - but initializing via this doesn't set the object as being initialized as this
-would promote using a public constructor (even though the analyzer will still cause a compilation error)
+* the generated code for records have an `init` accessibility on the `Value` property to support `with`, e.g. `var vo2 = vo1 with { Value = 42 }` - but initializing via this doesn't set the object as being initialized as this would promote using a public constructor (even though the analyzer will still cause a compilation error)
 * the generated code for records still overrides `ToString` as the default enumerates fields, which we don't want
 
-Something not yet implemented is primary constructor analysis for classes in C# 12, and how they will fit in with Vogen.
-This is covered in [this issue](https://github.com/SteveDunn/Vogen/issues/563).

src/Vogen/DiscoverUserProvidedOverloads.cs

@@ -45,7 +45,7 @@ private static UserProvidedTryParseMethods DiscoverTryParseMethods(INamedTypeSym
 
     private static UserProvidedToString HasToStringOverload(ITypeSymbol typeSymbol)
     {
-        var method = MethodDiscovery.TryGetToStringOverload(typeSymbol);
+        IMethodSymbol? method = MethodDiscovery.TryGetToStringOverride(typeSymbol);
         return method is null
             ? UserProvidedToString.NotProvided
             : new UserProvidedToString(

src/Vogen/MethodDiscovery.cs

@@ -9,13 +9,14 @@ namespace Vogen;
 internal static class MethodDiscovery
 {
     /// <summary>
-    /// Tries to get the ToString overload on the Value Object or base types.
-    /// It only includes overrides or virtuals.
-    /// It ignores the implicitly declared ToString on records.
+    /// Tries to get the ToString override on the Value Object or base types.
+    /// It only includes overrides or virtual methods.
+    /// We ignore the implicitly generated ToStrings when a VO *is* a record or *derives from* a record, as we still want to
+    /// generate one ourselves as we don't want the default behaviour.
     /// </summary>
     /// <param name="typeSymbol"></param>
-    /// <returns></returns>
-    public static IMethodSymbol? TryGetToStringOverload(ITypeSymbol typeSymbol)
+    /// <returns>null if no overloads</returns>
+    public static IMethodSymbol? TryGetToStringOverride(ITypeSymbol typeSymbol)
     {
         var toStringMethods = typeSymbol.GetMembers("ToString").OfType<IMethodSymbol>();
 
@@ -63,7 +64,7 @@ internal static class MethodDiscovery
             return null;
         }
 
-        return TryGetToStringOverload(baseType);
+        return TryGetToStringOverride(baseType);
     }
 
     public static ITypeSymbol? TryGetHashCodeOverload(ITypeSymbol vo)

src/Vogen/Util.cs

@@ -301,14 +301,20 @@ public static string GenerateDebuggerProxyForClasses(TypeDeclarationSyntax tds,
 
     private static string GenerateToString(VoWorkItem item, bool isReadOnly)
     {
+        if (item.UserProvidedOverloads.ToStringInfo.WasSupplied)
+        {
+            return string.Empty;
+        }
+
         string ro = isReadOnly ? " readonly" : string.Empty;
-
-        return item.UserProvidedOverloads.ToStringInfo.WasSupplied
-            ? string.Empty
-            : $"""
-               /// <summary>Returns the string representation of the underlying <see cref="{EscapeTypeNameForTripleSlashComment(item.UnderlyingType)}" />.</summary>
-               public{ro} override global::System.String{item.Nullable.QuestionMarkForOtherReferences} ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
-               """;
+
+        // we don't consider nullability here as *our* ToString never returns null, and we are free to 'narrow'
+        // the signature (going from nullable to non-nullable): https://github.com/dotnet/coreclr/pull/23466#discussion_r269822099
+
+        return $"""
+                /// <summary>Returns the string representation of the underlying <see cref="{EscapeTypeNameForTripleSlashComment(item.UnderlyingType)}" />.</summary>
+                public{ro} override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
+                """;
     }
 
     public static string GenerateGuidFactoryMethodIfNeeded(VoWorkItem item)

tests/ConsumerTests/ToStringTests/BasicFunctionality.cs

@@ -4,6 +4,27 @@ namespace ConsumerTests.ToStringTests;
 
 public class BasicFunctionality
 {
+    [Fact]
+    public void Defaults_to_empty_string_if_primitive_returns_null()
+    {
+        VoWrappingNaughtyPrimitive v = VoWrappingNaughtyPrimitive.From(new NaughtyPrimitive());
+        v.ToString().Should().Be(string.Empty);
+    }
+
+    [Fact]
+    public void Does_not_use_ToString_from_record()
+    {
+        MyRecordVo record = MyRecordVo.From(123);
+        record.ToString().Should().Be("123");
+    }
+
+    [Fact]
+    public void Generates_correct_nullability_for_when_vo_record_derives_from_a_record()
+    {
+        MyDerivedRecordVo record = MyDerivedRecordVo.From(123);
+        record.ToString().Should().Be("123");
+    }
+
     [SkippableIfBuiltWithNoValidationFlagFact]
     public void ToString_does_not_throw_for_something_uninitialized()
     {
@@ -36,4 +57,22 @@ public void ToString_uses_generated_method()
         Name.From("barney").ToString().Should().Be("barney");
         Name.From("wilma").ToString().Should().Be("wilma");
     }
-}
+}
+
+[ValueObject<NaughtyPrimitive>]
+public partial class VoWrappingNaughtyPrimitive
+{
+}
+
+public class NaughtyPrimitive
+{
+    public override string? ToString() => null;
+}
+
+[ValueObject]
+public partial record MyRecordVo;
+
+public record BaseRecord;
+
+[ValueObject]
+public partial record MyDerivedRecordVo : BaseRecord;

tests/SnapshotTests/BsonSerializationGeneration/snapshots/snap-v8.0/BsonSerializationGenerationTests.Writes_bson_serializers.verified.txt

@@ -661,7 +661,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
         }
 
         /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-        public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+        public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
         [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
         private readonly void EnsureInitialized()
         {
@@ -973,7 +973,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
         }
 
         /// <summary>Returns the string representation of the underlying <see cref = "System.String"/>.</summary>
-        public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+        public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
         [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
         private readonly void EnsureInitialized()
         {

tests/SnapshotTests/BugFixes/Bug673_nullability_override.cs

@@ -0,0 +1,31 @@
+using System.Threading.Tasks;
+using Shared;
+using Vogen;
+
+namespace SnapshotTests.BugFixes;
+
+// See https://github.com/SteveDunn/Vogen/issues/673
+public class Bug673_nullability_override
+{
+    [Fact]
+    public async Task When_deriving_from_a_record_it_matches_the_override_for_the_records_ToString()
+    {
+        var source = """
+                     #nullable enable
+                     using System;
+                     using Vogen;
+
+                     namespace Foo;
+
+                     internal abstract record B;
+
+                     [ValueObject]
+                     internal partial record D : B;
+                     """;
+
+        await new SnapshotRunner<ValueObjectGenerator>()
+                .WithSource(source)
+                .IgnoreInitialCompilationErrors()
+                .RunOn(TargetFramework.Net8_0);
+    }
+}

tests/SnapshotTests/BugFixes/snapshots/snap-v3.1/Bug575_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -379,7 +379,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v3.1/Bug581_StjDeserializer_throws_correct_exception.Test.verified.txt

@@ -296,7 +296,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Guid"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v3.1/Bug595_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -378,7 +378,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v4.6.1/Bug575_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -322,7 +322,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v4.6.1/Bug581_StjDeserializer_throws_correct_exception.Test.verified.txt

@@ -261,7 +261,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Guid"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v4.6.1/Bug595_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -321,7 +321,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v4.6.1/Bug597_STJ_being_emitted_even_with_no_conversions_specified.Omitted_if_stj_package_not_referenced.verified.txt

@@ -278,7 +278,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v4.8/Bug575_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -322,7 +322,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v4.8/Bug581_StjDeserializer_throws_correct_exception.Test.verified.txt

@@ -261,7 +261,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Guid"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v4.8/Bug595_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -321,7 +321,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v5.0/Bug575_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -379,7 +379,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v5.0/Bug581_StjDeserializer_throws_correct_exception.Test.verified.txt

@@ -296,7 +296,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Guid"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v5.0/Bug595_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -378,7 +378,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v6.0/Bug575_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -379,7 +379,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v6.0/Bug581_StjDeserializer_throws_correct_exception.Test.verified.txt

@@ -296,7 +296,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Guid"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v6.0/Bug595_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -378,7 +378,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {

tests/SnapshotTests/BugFixes/snapshots/snap-v7.0/Bug575_AttributesWithArraysBreaksGenerator.Test.verified.txt

@@ -436,7 +436,7 @@ private readonly global::System.Diagnostics.StackTrace _stackTrace = null!;
     }
 
     /// <summary>Returns the string representation of the underlying <see cref = "System.Int32"/>.</summary>
-    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() : "[UNINITIALIZED]";
+    public readonly override global::System.String ToString() => IsInitialized() ? Value.ToString() ?? "" : "[UNINITIALIZED]";
     [global::System.Runtime.CompilerServices.MethodImpl(global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
     private readonly void EnsureInitialized()
     {