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

Overload with cancellation ignored when using params #2

Open
mrphil2105 opened this issue Feb 14, 2020 · 3 comments
Open

Overload with cancellation ignored when using params #2

mrphil2105 opened this issue Feb 14, 2020 · 3 comments

Comments

@mrphil2105
Copy link

mrphil2105 commented Feb 14, 2020

Version

1.0.1

Description

When you have a CancellationToken in scope and two methods, one that takes a params array and another that takes a regular array and a CancellationToken, for example the two following methods defined in DbSet<TEntity> in Entity Framework Core:

public virtual ValueTask<TEntity> FindAsync(object[] keyValues, CancellationToken cancellationToken);
public virtual ValueTask<TEntity> FindAsync(params object[] keyValues);

...it favors the params method as expected if you pass an object, but the analyzer doesn't give a warning that I should use the other method with manual array construction, and pass the CancellationToken

Observed and Expected Behavior

As described above, it doesn't warn when using a method that takes a params array when there's another method that takes a regular array and a CancellationToken:

// No warning here.
var server = await dbContext.Servers.FindAsync(command.Id);
// What it should tell me to do.
var server = await dbContext.Servers.FindAsync(new object[] { command.Id }, cancellationToken);

The analyzer should warn me that I should construct the array directly and use the 2nd overload and pass along the CancellationToken, as shown in the example above.

@JakenVeina
Copy link
Owner

So, this isn't a bug, it's definitely working as intended. But it's definitely a common case that should be supported.

The issue is that the proper fix is more than just adding CancellationToken to the arguments list. You have also have to wrap all the other arguments. The analyzer sees these calls as "incompatible" on account of the presence of the params keyword on only one of them.

I think it's worth creating a separate diagnostic code for this case, since the codefix is a little more involved, and requires a transform of the other arguments. Might even just not provide a codefix for this, since it would basically involve re-inventing argument binding, to figure out which arguments need transformed into the params array, and which don't.

@mrphil2105
Copy link
Author

I thought as much. It's simply something the analyzer doesn't cover at the moment, as it's a separate thing. So I guess my issue is a feature request then 😄 Is it possible to have a codefix that if it sees another overload like in my example, it will construct the array directly and pass the CancellationToken?

@JakenVeina
Copy link
Owner

Hey, anything's possible.

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

No branches or pull requests

2 participants