NullReferenceException from Token.operator !=

[KalleOlaviNiemitalo]

If operator ==(Token left, Token right) or operator !=(Token left, Token right) is called such that left is null, then it throws NullReferenceException because it tries to call left.Equals(right).

command-line-api/src/System.CommandLine/Parsing/Token.cs

Lines 63 to 77 in 209b724

	/// <summary>
	/// Checks if two specified <see cref="Token"/> instances have the same value.
	/// </summary>
	/// <param name="left">The first <see cref="Token"/>.</param>
	/// <param name="right">The second <see cref="Token"/>.</param>
	/// <returns><see langword="true" /> if the objects are equal.</returns>
	public static bool operator ==(Token left, Token right) => left.Equals(right);
	/// <summary>
	/// Checks if two specified <see cref="Token"/> instances have different values.
	/// </summary>
	/// <param name="left">The first <see cref="Token"/>.</param>
	/// <param name="right">The second <see cref="Token"/>.</param>
	/// <returns><see langword="true" /> if the objects are not equal.</returns>
	public static bool operator !=(Token left, Token right) => !left.Equals(right);

That happened in my app, which compared optionResult.Token != null as part of the #1795 (comment) implementation. I'll be able to work around the bug by using !object.ReferenceEquals(optionResult.Token, null) instead.

No other types in System.CommandLine define comparison operators that could have similar bugs.

[KalleOlaviNiemitalo]

It was broken by #1654 commit 59b1941; before that, Token was a value type, and the arguments of the comparison operators could not have been null.

[KalleOlaviNiemitalo]

Can the comparison operators of Token be just deleted? And perhaps Equals, GetHashCode, and IEquatable<> as well.

[KalleOlaviNiemitalo]

Could then seal the class, too. If it has been a value type until recently, there cannot be too many application-specific types derived from it.

[KalleOlaviNiemitalo]

Removing only operator == and operator != from Token would cause only one test to fail:

• System.CommandLine.ApiCompatibility.Tests.ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed

AFAIK, breaking API changes are still permitted until 2.0 RTM.

Removing Equals, GetHashCode, and IEquatable<Token> from Token would additionally cause the following tests to fail:

• System.CommandLine.Tests.ParserTests.Option_argument_arity_can_be_a_fixed_value_greater_than_1
• System.CommandLine.Tests.ParserTests.Option_argument_arity_can_be_a_range_with_a_lower_bound_greater_than_1

These failures are caused by the tests using BeEquivalentTo. When Token itself no longer provides equality comparisons, this ends up using StructuralEqualityEquivalencyStep and comparing all non-private properties and fields, including Token.IsImplicit and Token.Position, which the current Token.Equals implementations would ignore. This could perhaps be fixed by defining an IEqualityComparer<Token> just for tests, but the Token.Symbol property would then have to be made public in order to let the test project access it. (The public constructor of Token has a parameter for initializing Token.Symbol, but the property was made internal in #1560; I don't know why.)

I recommend removing operator == and operator !=. Defining those made sense when Token was a value type, but it is somewhat unexpected in a reference type.