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

Ignore conditional block when condition is CancellationToken.IsCancellationRequested #61

Closed
skarllot opened this issue Mar 16, 2024 · 4 comments

Comments

@skarllot
Copy link

If the asynchronous method has a conditional block testing if CancellationToken is canceled that block should be ignored.

public ValueTask Test(CancellationToken cancellationToken)
{
    if (cancellationToken.IsCancellationRequested)
    {
        // ...
        return ValueTask.FromCanceled(cancellationToken);
    }
    // ...
}
@lsoft
Copy link

lsoft commented Mar 17, 2024

@skarllot not sure that ISG should care about this issue. Imagine more complex if statements like if (cancellationToken.IsCancellationRequested && ...). removing conditional subtree is not an option here, ISG will have to rewrite source code in a complex way like replacing cancellationToken.IsCancellationRequested with false. Not sure ISG should perform such complex rewriting, let's wait for @virzak resolution.

I guess this case is the case for #if SYNC_ONLY.

@skarllot
Copy link
Author

On second thought I believe you are right, I prefer using compiler conditional.

@virzak
Copy link
Contributor

virzak commented Mar 17, 2024

Great issue.

The source generator has already picked an aggressive/advanced way of dealing with such issues. The goal is to make is as close to the code one would write by hand.

For example if it sees IProgress<int> used like this:

if (true)
{
    progress.Report(1);
}

the entire if block will be dropped.

This approach did land us in a bit of a hot water, for example in #45 (comment)

I think that the example above should be dropped without forcing the user to invoke SYNC_ONLY, just because of how common that scenario can be.

When combined with && it should be dropped as well.
When negated (!) the body should get unrolled.

If there's while instead of if, the block should be dropped for ct.IsCancellationRequested and be made while (true) for !ct.IsCancellationRequested

Looking at code occurrences under dotnet org, the most common uses are:

  • while (!ct.IsCancellationRequested)
  • if (ct.IsCancellationRequested)

@virzak virzak closed this as completed in fa72a97 Mar 19, 2024
@virzak
Copy link
Contributor

virzak commented Mar 19, 2024

This fix is available in 1.3.35. Let me know how this works out.

At the end not unrolling the if block because you could have a duplicate variable:

if (true)
{
    var myVar = 1;
}

if (true)
{
    var myVar = 1;
}

Taking either myVar out of scope would cause a compilation issue.

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

3 participants