Runtime: Inconsistent behavior when comparing Version instances where one is null.

[stukselbax]

If you have two object of type System.Version, and you want to compare them using operators <, <=, >, >=, (_and you forgot to check for null by yourself_ ) there is inconsistent behavior when some value is null. Consider the code sample:

            Version ver1 = Version.Parse("1.2.3.4");
            Version ver2 = null;

            Console.WriteLine("ver1 < ver2 ? {0}", ver1 < ver2);
            try
            {
                Console.WriteLine("ver1 > ver2 ? {0}", ver1 > ver2);
            }
            catch (ArgumentNullException e)
            {
                Console.WriteLine(e.Message);
            }

            Console.WriteLine("ver1 <= ver2 ? {0}", ver1 <= ver2);
            try
            {
                Console.WriteLine("ver1 >= ver2 ? {0}", ver1 >= ver2);
            }
            catch (ArgumentNullException e)
            {
                Console.WriteLine(e.Message);
            }

Output will be as follow:

ver1 < ver2 ? False
Value cannot be null.
Parameter name: v1
ver1 <= ver2 ? False
Value cannot be null.
Parameter name: v1

It is expected that in all cases an exception must be thrown.

[karelz]

Here's the code:
https://github.com/dotnet/corefx/blob/aa73b08881ccc2a438947c19d60a6665008bcc08/src/Common/src/CoreLib/System/Version.cs#L423-L445

For consistency we should probably follow the rule that null is lower than anything else and not throw ArgumentNullException:
https://github.com/dotnet/corefx/blob/aa73b08881ccc2a438947c19d60a6665008bcc08/src/Common/src/CoreLib/System/Version.cs#L157

It will create subtle platform difference - @stephentoub do you have opinion if it is worth a change, or just keep current status (for platform consistency)?

[stephentoub]

It is expected that in all cases an exception must be thrown.

That would be a breaking change, introducing exceptions where they didn't previously exist, and I expect there are actually cases where it would break existing code.

@stephentoub do you have opinion if it is worth a change, or just keep current status (for platform consistency)?

@karelz's suggestion of "For consistency we should probably follow the rule that null is lower than anything else and not throw ArgumentNullException" sounds fine to me.

From a quick search across coreclr and corefx, it looks like Version is the only class we have that implements these <, <=, >, and >= operators; every other type that implements them is a struct and thus isn't concerned about nulls. But there are some other classes that implement == and !=, e.g. String and Uri, and they allow either or both arguments to be null rather than throwing if the first argument is, so it seems reasonable that the ordering operators wouldn't throw if the first argument is null either, and removing argument exceptions is generally not considered a breaking change.

@terrajobst, @KrzysztofCwalina, do we have API guidelines for this case anywhere? It's pretty rare.

[jnm2]

I've always pointed people to the IComparable<>.CompareTo docs:

By definition, any object compares greater than null, and two null references compare equal to each other.

(It seems to be making a generalization that goes beyond the capabilities of the CompareTo itself, since it refers to comparing null to null. Perhaps describing how CompareTo should be used.)

[stukselbax]

@jnm2 interesting definition.
Thus @karelz solution is the most appropriate in this case?

[karelz]

@stukselbax let's wait for @terrajobst @KrzysztofCwalina to chime in, but it is likely my recommendation (endorsed by @stephentoub) is the right one.

[stukselbax]

Any news? Will you accept PR where @karelz suggestion will be implemented (do not throw exception)?

[karelz]

@terrajobst @bartonjs @ericstj what's your opinion?

[bartonjs]

So long as it's consistent (a < b means b > a) I'm fine with deciding that null is the lowest, or highest, value.

[ericstj]

I'm OK with it. We have precedent for accepting the removal of an exception case as an acceptable breaking change. We make sure to think about reverse compat (eg: missing a bug because you only test where its fixed), but given there's no pathological case here I'm willing to accept the reverse compat issue. I believe it only makes sense if null is the considered the lowest and I believe that's consistent with the CompareTo guidelines.

I think we should draft an addition to the framework design guidelines to make that consistent: operators should not throw, comparison operators on classes should have defined behavior for null. Arithmetic operators are a different discussion but I could imagine going either way. I don't believe we need to address that for this issue.

[karelz]

@stukselbax ok, you have green light on the PR - thank you!

[SingleAccretion]

@jeffhandley This bug has been fixed in 0414ad171efd758ed1c527a07d2b8c77138d0dea - I think the issue can now be closed.