Initial work to move indentation services down to codestyle layer (step 9/N)#60452
Initial work to move indentation services down to codestyle layer (step 9/N)#60452CyrusNajmabadi merged 21 commits intodotnet:mainfrom
Conversation
|
@CyrusNajmabadi I can take a look at this PR once the other approved PR is merged and you have pushed a merge commit on this PR. Thanks! |
|
@mavasani done. |
|
though feel free to do this one piece at a time. |
| } | ||
|
|
||
| #pragma warning disable IDE0060 // Remove unused parameter | ||
| public static bool IsPreProcessorKeywordContext(this SyntaxTree syntaxTree, int position, SyntaxToken preProcessorTokenOnLeftOfPosition) |
There was a problem hiding this comment.
I understand this a move, but seems this can be an extension method on the token instead of the unused syntaxTree parameter.
There was a problem hiding this comment.
yes. but that would have a downstream impact on all the existing callers. i didn't want to do that. :)
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Indentation/AbstractIndentation.cs
Outdated
Show resolved
Hide resolved
...edUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb
Show resolved
Hide resolved
| #if CODE_STYLE | ||
| var sourceText = document.GetTextAsync(cancellationToken).WaitAndGetResult_CanCallOnBackground(cancellationToken); | ||
| #else | ||
| var sourceText = document.GetTextSynchronously(cancellationToken); |
There was a problem hiding this comment.
Ah, I hate that the synchronous API is non-public and we end up writing sync helper methods in Workspaces layer that should really be async, and now that we want to move it down you are either left with cascading async changes flowing up the call chain OR this ugly code with preprocessor directives and WaitAndGetResult_CanCallOnBackground calls and such. If possible, try to convert GetPreferredIndentation to an async method to avoid this and just use GetTextAsync here (if it doesn't lead to too big an explosion of changes).
There was a problem hiding this comment.
so the main problem here is that for VS we do want the actual indentation-feature (e.g. when you hit enter) to be synchronous. so we want to actually avoid async+blocking. We then also expose this functionality out for other features to use (like code-fixes). Codefixes could be async without issue, but we still need the hardened sync path for the mainline feature, and that mainline feature should not block on asynchrony as that could lead to stalls on UI thread. :(
(this is not pretty).
mavasani
left a comment
There was a problem hiding this comment.
Overall looks good. Seems there are some WIP pieces and an API that can possible be made async to make it cleaner.
Followup to #60451.
This PR moves the 'Convert Namespace' fixer down to the CodeFixes layer now that the indentation service is available there.
I'm doing these PRs in multiple parts to make things simpler to review. Every time i try to tackle the general problem, it explodes into a ton of changes.
The reason for this change overall is that we have fixers taht need to know indentation information. For example, the 'convert to file-scoped-namespace (and reverse)' fixes have to know indentation information so it can make the right fixes. Not having indentation info be available at thsi level means that the fixes are constrained to the VS host.