Make the FormatAsync methods in the Workspaces (portable) projects Formatter class public.

[Ross-Esmond]

The Formatter class has a few FormatAsync methods for formatting individual nodes. Each of these methods are marked internal but have a public synchronous method exposing them. This creates some problems though because the .WaitAndGetResult call used throws an exception if called from a background thread. Which prevents unit tests that need to format individual nodes from accessing the api.

[Pilchie]

Can you take a look and see if we should just expose the async versions?

[Ross-Esmond]

That seems like the best option to me. There's specifically four methods that are being exposed by synchronous methods. The only issue I found is that making them public triggers warning RS0026, but that seems to be related to issue dotnet/roslyn-analyzers#6757. I've gone over all the methods and there's no room for ambiguity or backwards compatibility problems. I wound up just having to suppress it.

[mattwar]

These particular methods are not supposed to be asynchronous at all. They seem to be async now based on work that I assume HeeJae did to make portions of the format engine work in parallel.

[Ross-Esmond]

The actaul issue I ran into is that WaitAndGetResult breaks unit tests. So refactoring the Format methods code to be natively synchronous would also fix my problem.

[mattwar]

I was implying that they are incorrectly being implemented as async methods, when they should not be. I don't think exposing the incorrect signatures that should be undone would be wise.

[Ross-Esmond]

I understand. Making it public can't be reversed, so if the async method is undesirable then it's not an option. It sounds like my issue report is just inherently flawed. The specific problem that does exists though is a mismatch between what WaitAndGetResult expects and how the Format methods work. In WaitAndGetResult there is an error that unit tests will always trigger with a comment saying

"If you hit this when running tests then your code is in error. WaitAndGetResult should only be called from a foreground thread."

But with the public api as it exists now there is no way to do that while formatting nodes from unit tests. I did some digging and it appears that there use to be a check that would exclude unit tests but it was removed in this commit.

Microsoft.CodeAnalysis.Workspace.PrimaryWorkspace != null
// only care if we are in a UI situation.. this keeps normal unit tests from failing

If you can think of a better solution I can open up an entirely separate issue to replace this one, but either way it sounds like this issue is won't-fix.

[Pilchie]

Tagging @CyrusNajmabadi. IIRC - there is a "WaitAndGetResult_AllowFromBackgroundThread" (or similar). should we just be using that instead in this case?

[CyrusNajmabadi]

@Pilchie As this is part of a library probably. We can't go willy nill asserting things like that for public APIs. It's a shame though as that assert is good for catching badly behaving Rolsyn code.

We should also just make the async API public.

The issue here is that we believed, earlier, that APIs like this could just synchronous, and that all asynchrony would be at the Document level. However, as time has gone on, certain paths have been come async (sometimes for parallelization reasons, sometimes just because an underlying API can't be sync anymore), and that bleeds out and causes these mismatches.

I think we should like just expose the async public api and deprecate the sync version (as it's harmful when it does blocking waits).

[CyrusNajmabadi]

We can't go willy nill asserting things like that for public APIs. It's a shame though as that assert is good for catching badly behaving Rolsyn code.

Actually, scratch that. This assert only happens in debug. @Ross-Esmond Are you referencing debug versions of Roslyn?

[Ross-Esmond]

@CyrusNajmabadi Yes. I'm using the source code and I have several edits so I never thought to change it. I guess I could just build for release. That would definitely fix the error.

[CyrusNajmabadi]

Ok. So, here's what i think i'd like moving forward:

1. We should expose the async versions. After all, just exposing sync methods that block on async can be very bad for the threadpool.
2. We should deprecate the sync versions. People should really move to the async ones. If people need sync functionality they can always choose to do the blocking themselves instead of it being out of their control.
3. We should keep the debug-asserts. They're incredibly valuable for Roslyn itself to detect when we're doing something inappropriate. And they won't hurt anyone who is just referencing our published dlls.

[Ross-Esmond]

Sounds fine to me. As long as you saw @mattwar's first comment.

[sharwell]

This was resolved via @mattwar's approach in #31413 (the asynchronous operations were removed from the internal API following removal of parallel execution in #31276, so there is nothing left to expose).