Always try to register the EditorConfigDocumentOptionsProvider #40513
Conversation
| @@ -74,6 +76,19 @@ protected Workspace(HostServices host, string? workspaceKind) | |||
|
|
|||
| // initialize with empty solution | |||
| _latestSolution = CreateSolution(SolutionId.CreateNewId()); | |||
|
|
|||
| _taskQueue.ScheduleTask(() => TryRegisterDocumentOptionsProvider(), nameof(TryRegisterDocumentOptionsProvider)); | |||
There was a problem hiding this comment.
- why on a task? are there race conditions here?
- what general value is this change trying to provide? What's wrong with MEF here?
There was a problem hiding this comment.
Talked to @jasonmalinowski offline and he helped me get to the root of the issue. Will update in a bit. Thanks for taking a look!
|
@JoeRobich let me know when this is ready for review |
|
@sharwell I'm ready for feedback. Please take a look. |
src/EditorFeatures/Test/Workspaces/ProjectCacheHostServiceFactoryTests.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/TaskList/CommentTaskTokenSerializer.cs
Show resolved
Hide resolved
|
Tagging @sharwell - I believe this fixes the issue we discussed offline. @JoeRobich this change is required to enable .editorconfig based testing for CodeStyle layer. |
Extracted out of dotnet#41363 so it only contains the following changes: 1. Port analyzer/fixer/test files for ConvertSwitchStatementToExpression to shared layer 2. Options related namespace changes (see comment dotnet#41363 (comment) for details) 3. MEF based language service discovery in CodeStyle layer. dotnet#41462 tracks replacing this with a non-MEF based approach. 4. Minimal resx/xlf file changes - few were needed as the resources used by analyzer/fixer were moved to shared resx file. 5. Enable .editorconfig based unit test support for CodeStyle layer. NOTE: First commit dotnet@b006324 just ports changes from dotnet#40513 which are required to enable this support. This commit should become redundant once that PR is merged in.
|
@JoeRobich The published artifacts have devenv.dmp and servicehub.dmp, which indicate following assert is being hit during cleanup: http://sourceroslyn.io/#Microsoft.CodeAnalysis.Workspaces/Shared/TestHooks/AsynchronousOperationListener.cs,118 |
|
PR #41477 would remove the use of |
| } | ||
|
|
||
| public IDocumentOptionsProvider? TryCreate(Workspace workspace) | ||
| public static IDocumentOptionsProvider? TryCreate(Workspace workspace) | ||
| { | ||
| if (!ShouldUseNativeEditorConfigSupport(workspace)) |
There was a problem hiding this comment.
@JoeRobich I was able to verify the integration test failures from this PR locally, and the root cause seems to be this code path (from workspace constructor) can be invoked from a background thread, but fetching options can force loading of option persisters, which seem to require UI thread on creation. I moved this specific check ShouldUseNativeEditorConfigSupport from here down into GetOptionsForDocumentAsync below and then the tests pass locally. I think the correct long term solution here would be to remove the UI thread requirement from option persisters during their construction OR add functionality in GlobalOptionsService to be able to force UI thread before loading persisters. For now, this change should unblock us.
There was a problem hiding this comment.
By the way, do we still need to support legacy editorconfig provider? I would be fine with us deleting it completely as well. Tagging @jasonmalinowski
src/Workspaces/Core/Portable/Options/EditorConfig/EditorConfigDocumentOptionsProviderFactory.cs
Outdated
Show resolved
Hide resolved
Updates the Workspace to try to register the EditorConfigDocumentOptionsProvider.