Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 2, 2024
Merged

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

merged 1 commit into from
Jan 2, 2024

Conversation

AthenaAzuraeaX
Copy link

@AthenaAzuraeaX AthenaAzuraeaX commented Nov 22, 2023

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.

…id issues if wrapped type implements equality operators
@SteveDunn
Copy link
Owner

Many thanks for the PR! Apologies for the delay.

Copy link
Owner

@SteveDunn SteveDunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks again for the PR. And sorry again for the slow response!

@SteveDunn SteveDunn merged commit ad76425 into SteveDunn:main Jan 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants