Confusing nullability of `IDatabase.ListRightPop()` (and possibly more?)

[bdach]

The interface signature of the method in question is:

StackExchange.Redis/src/StackExchange.Redis/Interfaces/IDatabase.cs

Lines 1082 to 1091 in cb8b20d

	/// <summary>
	/// Removes and returns count elements from the end the list stored at key.
	/// If the list contains less than count elements, removes and returns the number of elements in the list.
	/// </summary>
	/// <param name="key">The key of the list.</param>
	/// <param name="count">The number of elements to pop</param>
	/// <param name="flags">The flags to use for this operation.</param>
	/// <returns>Array of values that were popped, or nil if the key doesn't exist.</returns>
	/// <remarks><seealso href="https://redis.io/commands/rpop"/></remarks>
	RedisValue[] ListRightPop(RedisKey key, long count, CommandFlags flags = CommandFlags.None);

The <returns> tag says that the method can return nil, but the actual method signature says that it can't. (This came up because there was actually a null-check in our code, and it started displaying a warning with a package bump.)

Judging by the implementation of the method I found and looked at for 5 minutes:

StackExchange.Redis/src/StackExchange.Redis/RedisDatabase.cs

Lines 1270 to 1274 in cb8b20d

	public RedisValue[] ListRightPop(RedisKey key, long count, CommandFlags flags = CommandFlags.None)
	{
	var msg = Message.Create(Database, flags, RedisCommand.RPOP, key, count);
	return ExecuteSync(msg, ResultProcessor.RedisValueArray, defaultValue: Array.Empty<RedisValue>());
	}

it appears that the method signature is probably correct and the documented return value should be changed to "empty array" instead? That said because I took 5 minutes to look at this, I'm not gonna attempt to claim correctness of that without confirmation.

It would probably be a good idea to grep through all usages of nil in the interface xmldocs and double-check them too because I see some more suspect ones like

StackExchange.Redis/src/StackExchange.Redis/Interfaces/IDatabase.cs

Lines 1492 to 1499 in cb8b20d

	/// <summary>
	/// Removes and returns a random element from the set value stored at key.
	/// </summary>
	/// <param name="key">The key of the set.</param>
	/// <param name="flags">The flags to use for this operation.</param>
	/// <returns>The removed element, or nil when key does not exist.</returns>
	/// <remarks><seealso href="https://redis.io/commands/spop"/></remarks>
	RedisValue SetPop(RedisKey key, CommandFlags flags = CommandFlags.None);

RedisValue is a struct, so going off the type annotations alone this can at worst return a default(RedisValue), which I wouldn't generally interpret to be the same as nil.

[mgravell]

Short version: yes, I think the comments could be clearer

Longer version:

• for the single value version, there is an instance property that allows "null" (not C# null, but null nonetheless) to be conveyed
• for arrays, I suspect you're right and we always return a non-empty array

[NickCraver]

I remember when we designed here - we already returned a non-empty array to prevent breaks for a lot of scenarios and definitely didn't want to add any breaks. Agreed, it's likely we want to change the docs here.

[bdach]

Thanks for the confirmation. I can probably get a PR going for this if it helps?

[NickCraver]

@bdach That'd be very welcome :)