CA2016: Forward cancellation token codefix is incorrect

[Youssef1313]

See dotnet/efcore#22667

cc: @carlossanlop

given a type that contains overloads like this:

public class C{
  public ValueTask<object?> FindAsync(params object?[]? keyValues) {}
  public ValueTask<object?> FindAsync(object?[]? keyValues, CancellationToken cancellationToken)
}

and is called like this:

async Task M(string[] args, CancellationToken token)
{
    var c = new C();
    var result = await c.FindAsync(5);
}

Actual

today CA2016 suggests this as a codefix:

async Task M(string[] args, CancellationToken token)
{
    var c = new C();
    var result = await c.FindAsync(5, cancellationToken: token); // Error CS1503 Argument 1: cannot convert from 'int' to 'object?[]?'
} 

Expected

if items at the previous call site was part of a params array then the codefix should wrap those elements inside a new object[] expression

async Task M(string[] args, CancellationToken token)
{
    var c = new C();
    var result = await c.FindAsync(new object?[] { 5 }, token);
}

an alternative could be

async Task M(string[] args, CancellationToken token)
{
    var c = new C();
    var result = await c.FindAsync(new [] { (object?)5 }, token);
}

[jmarolf]

We may want to change the analyzer behavior to only fire in situations where we are calling a method with a cancellation token as an optional parameter with a special list of known framework apis where the two method signatures are considered semantically equivalent.

[sharwell]

I believe the analyzer here is correct. The code fix should be updated to account for the case where the original parameter list is:

..., params T[] values

and the new parameter list is:

..., T[] values, CancellationToken cancellationToken

[jmarolf]

I see, we just want to make sure that the codefix does the right thing. I mis-read the issue

[Youssef1313]

I believe the analyzer here is correct. The code fix should be updated to account for the case where the original parameter list is:

..., params T[] values

and the new parameter list is:

..., T[] values, CancellationToken cancellationToken

While this is interesting case to test and make sure it's working, the case meant for EF Core is different.

The old signature is

..., params T[] values

and the new is:

..., CancellationToken cancellationToken, params T[] values

[arminatwork]

@Youssef1313

I believe the analyzer here is correct. The code fix should be updated to account for the case where the original parameter list is:

..., params T[] values

and the new parameter list is:

..., T[] values, CancellationToken cancellationToken

While this is interesting case to test and make sure it's working, the case meant for EF Core is different.

The old signature is

..., params T[] values

and the new is:

..., CancellationToken cancellationToken, params T[] values

this not working

got me error say:
AggregateException: One or more errors occurred. (The key value at position 0 of the call to 'DbSet.Find' was of type 'CancellationToken', which does not match the property type of 'int'.)

[carlossanlop]

I think the extra params object[] keyword may be causing the problem. The analyzer is smart enough to only fire when the CancellationToken instance is not in the last position.

I'll take a look.

Update: The cancellationToken parameter can only be in the last position:

See:

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ForwardCancellationTokenToInvocationsTests.cs

Lines 211 to 227 in a0522af

	[Fact]
	public Task CS_NoDiagnostic_OverloadTokenNotLastParameter()
	{
	return VerifyCS.VerifyAnalyzerAsync(@"
	using System.Threading;
	using System.Threading.Tasks;
	class C
	{
	async void M(CancellationToken ct)
	{
	await MethodAsync();
	}
	Task MethodAsync() => Task.CompletedTask;
	Task MethodAsync(int x, CancellationToken ct, string s) => Task.CompletedTask;
	}
	");
	}

but we need to consider the params object[], which allows passing anything without warning.

[arminatwork]

@carlossanlop
hi man

Well, it's going to make a mistake to me.
I put CancellationToken in the last parameter but he's going to miss it again.

so if have a params object, should CancellationToken put in the first parameter of method
it's going to make a error to me again

what's wrong?

[carlossanlop]

So looking into this more deeply, I don't think this is an issue with CA2016. This is an issue with allowing value types to get automatically boxed because of the params object[] parameter.

I cannot seem to repro the case where you are getting an error. Can you please share a code snippet?

This code works fine and shows no errors:

using System;
using System.Threading;
using System.Threading.Tasks;

namespace N
{
    class C
    {
        static async Task MyAsync(string id, CancellationToken ct)
        {
            await MethodAsync(CancellationToken.None, id); // This is using the second overload correctly (which takes a ct as first parameter)
            await MethodAsync(id, CancellationToken.None); // This is not showing any errors because we are boxing the struct, causing the first overload to be used
            await MethodAsync(o: id, ct: ct); // This is using the second overload correctly (named parameters force using ct as the first parameter)
        }
        static Task<int> MethodAsync(params object[] o) { Console.WriteLine(o); return Task.FromResult(1); }
        static Task<int> MethodAsync(CancellationToken ct, params object[] o) { Console.WriteLine($"{ct} {o}"); return Task.FromResult(2); }
    }
}

@mavasani what can we do about this, since it's unrelated to CA2016?

[arminatwork]

@carlossanlop Thanks for feedback

well I do passing 2 parameter to params object[] with CancellationToken
this code:
for example User.FindAsync(new object[] { userId, roleId}, cancellationToken);

got me error say :entity type UserRole is defined with a 2 key property, but 3 values were passed to the DbSet.Find method

[jmarolf]

@arminatwork

User.FindAsync(new object[] { userId, roleId}, cancellationToken);

What is the definition of FindAsync? does it compile? CS0231 should be triggered for a method that has a params array anywhere but in the final position.

[arminatwork]

@jmarolf
well I said error in the last comment:\

in this issues is my problem FindAsync rough edges when passing CancellationToken

[mavasani]

@arminatwork @jmarolf Are we good to close this issue?

[arminatwork]

@arminatwork @jmarolf Are we good to close this issue?

Well no
I don't think the problem is solved!

[jmarolf]

I'll update the bug with a description of the issue so its clear what the issue is.

[jmarolf]

updated with a repo @arianmotamedi is my description of the problem correct?

[arianmotamedi]

updated with a repo @arianmotamedi is my description of the problem correct?

I think you tagged the wrong person :)

[jmarolf]

:( woops @arminatwork is my description of the problem correct?

[arminatwork]

I'll update the bug with a description of the issue so its clear what the issue is.

Ok👍

[arminatwork]

updated with a repo @arianmotamedi is my description of the problem correct?

What do you mean?

[jmarolf]

well I am updating a bug on others behalf I wanted to make sure the expected/actual was correct just to be safe.

[arminatwork]

@jmarolf
well
Has the problem been solved?

[jmarolf]

the snark :)