Skip to content

Add code fix provider for VSTHRD109#397

Merged
AArnott merged 1 commit into
microsoft:v15.8from
AArnott:fix316
Oct 9, 2018
Merged

Add code fix provider for VSTHRD109#397
AArnott merged 1 commit into
microsoft:v15.8from
AArnott:fix316

Conversation

@AArnott
Copy link
Copy Markdown
Member

@AArnott AArnott commented Oct 8, 2018

This required some of the same code fixer functionality already found in the VSTHRD010 code fixer, so a few chunks of code got moved from that fixer into the Utils class.

There are several tested scenarios, but the crux of the fix is that it changes this:

    async Task FooAsync(CancellationToken ct) {
        ThreadHelper.ThrowIfNotOnUIThread(); // VSTHRD109 diagnostic here
        await Task.Yield();
    }

to this:

    async Task FooAsync(CancellationToken ct) {
        await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(ct);
        await Task.Yield();
    }

Note that the code fix automatically finds JTF instance(s) to use (the user picks the one from the fix list), and automatically finds any local CancellationToken to propagate into the switch method call.

Closes #316

@AArnott AArnott added this to the v15.8 milestone Oct 8, 2018
@AArnott AArnott self-assigned this Oct 8, 2018
@AArnott AArnott requested a review from sharwell October 8, 2018 03:04
await Verify.VerifyCodeFixAsync(test, expected, fix);
}

[Fact(Skip = "Fails because the semantic model claims the anonymous func returns Task instead of Task<int>. We should probably skip that check and always preserve return statements.")]
Copy link
Copy Markdown
Contributor

@sharwell sharwell Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 This may work in newer versions of Roslyn with later language versions (bestest betterness). If the underlying issue is the inability to distinguish between Task.Run(Func<Task>) and Task.Run<TResult>(Func<Task<TResult>>), you can force a disambiguation in the test by calling Task.Run<int> instead of just Task.Run.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, using Task.Run<int> does resolve the issue (see the test directly above this one, on which I applied that workaround).

Interestingly though, although this compiles:

Task<int> t = Task.Run(() => Task.FromResult(1));

Even in that case, which compiles, Roslyn still claims that the delegate returns Task. Yet if it really did, the result couldn't be assigned to Task<int> as I did. So what's up with that? Would 'bestest betterness' actually solve this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even in that case, which compiles,

Are you compiling with the same version of Roslyn as you are using to run the tests?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I compile the analyzers against Roslyn 1.2, and the tests run with 2.3. Evidently the dependency on the new Roslyn test framework bumped up the version of Roslyn I'm testing with.
How does that affect this investigation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evidently the dependency on the new Roslyn test framework bumped up the version of Roslyn I'm testing with.

This shouldn't be the case. The test framework is compiled against Roslyn 1.0.1. The version of Roslyn used for testing is controlled by the following line:

https://github.com/Microsoft/vs-threading/blob/9df4cbf729555494c4a3b2ef9902fbb7dfa81100/src/Microsoft.VisualStudio.Threading.Analyzers.Tests/Microsoft.VisualStudio.Threading.Analyzers.Tests.csproj#L23

Git blame indicates this was updated to Roslyn 2.3.2 in #190.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I can certainly try to match the versions up (presumably by rolling back the version in the tests). What do you predict will happen? Will it resolve the issue? If so, why?

@AArnott AArnott merged commit dce573f into microsoft:v15.8 Oct 9, 2018
@AArnott AArnott deleted the fix316 branch October 9, 2018 05:45
AArnott pushed a commit that referenced this pull request Sep 22, 2025
…fad15 (#397)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants