SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
782 stars 46 forks source link

Use 'is not null' instead of '!= null' in Newtonsoft converter #537

Closed AthenaAzuraeaX closed 8 months ago

AthenaAzuraeaX commented 9 months ago

Vogen does not follow best practices for null checks. Instead of using the "is" operator, it is using equality operators. This sometimes causes issues when a class has overloaded the equality operators.

Consider this code:

[ValueObject(typeof(String))]
public partial class LinuxAbsoluteFilename: IAbsoluteFilename
{
    public static Boolean operator ==(LinuxAbsoluteFilename? Left, IAbsoluteFilename? Right);
    public static Boolean operator !=(LinuxAbsoluteFilename? Left, IAbsoluteFilename? Right);
}

[ValueObject(typeof(LinuxAbsoluteFilename), conversions: Conversions.NewtonsoftJson)]
public sealed partial class AbsoluteContainerFilename
{
}

When using the Newtonsoft converter, the generated code by Vogen looks like:

var result = serializer.Deserialize<LinuxAbsoluteFilename>(reader);
return result != null ? AbsoluteContainerFilename.Deserialize(result) : null;

The problem with this generated code is that Vogen uses != instead of not null, and so the compiler is unable to determine which operator to call. If we look at the metadata for the class LinuxAbsoluteFilename, since Vogen also adds its own operators, we see:

[DebuggerDisplay("Underlying type: System.String, Value = { _value }")] [DebuggerTypeProxy(typeof(LinuxAbsoluteFilenameDebugView))] [ExcludeFromCodeCoverage] [GeneratedCode("Vogen", "3.0.0.0")] [JsonConverter(typeof(LinuxAbsoluteFilenameSystemTextJsonConverter))] [TypeConverter(typeof(LinuxAbsoluteFilenameTypeConverter))] [ValueObject(typeof(String), Conversions.Default, null, Customizations.None, DeserializationStrictness.AllowValidAndKnownInstances, DebuggerAttributeGeneration.Default)] public class LinuxAbsoluteFilename: IAbsoluteFilename, IFilename, IEquatable<IAbsoluteFilename>, IEquatable<LinuxAbsoluteFilename>, IEquatable<String>, IComparable<LinuxAbsoluteFilename>, IComparable { public static Boolean operator ==(LinuxAbsoluteFilename? Left, IAbsoluteFilename? Right); public static Boolean operator ==(LinuxAbsoluteFilename left, String right); public static Boolean operator ==(String left, LinuxAbsoluteFilename right); public static Boolean operator ==(LinuxAbsoluteFilename left, LinuxAbsoluteFilename right); public static Boolean operator !=(String left, LinuxAbsoluteFilename right); public static Boolean operator !=(LinuxAbsoluteFilename left, String right); public static Boolean operator !=(LinuxAbsoluteFilename left, LinuxAbsoluteFilename right); public static Boolean operator !=(LinuxAbsoluteFilename? Left, IAbsoluteFilename? Right); }

So the compiler has three overloads to choose from:

public static Boolean operator !=(LinuxAbsoluteFilename left, String right); public static Boolean operator !=(LinuxAbsoluteFilename left, LinuxAbsoluteFilename right); public static Boolean operator !=(LinuxAbsoluteFilename? Left, IAbsoluteFilename? Right);

Since null is not strongly typed, the call is ambiguous. This can be prevented by using "is not null" instead. But for now, this is causing a compile error for all such value objects.

This PR solves at least this issue for a specific case of Newtonsoft. I am not familiar enough with the code to know if there are similar issues elsewhere, so I have only fixed this specific place.

SteveDunn commented 9 months ago

Many thanks for the PR! Apologies for the delay.