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.

Sign up for GitHub

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

Always try to register the EditorConfigDocumentOptionsProvider #40513

Merged
merged 10 commits into from
Mar 2, 2020
+25 −30
Merged

Always try to register the EditorConfigDocumentOptionsProvider #40513

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ec43652
Always try to register the EditorConfigDocumentOptionsProvider
JoeRobich Dec 19, 2019
55acf88
Remove import of VisualStudioWorksapce
JoeRobich Jan 8, 2020
e56bf8e
Remove unnecessary comment
JoeRobich Jan 8, 2020
007b21b
Make IErrorLoggerService not required
JoeRobich Jan 8, 2020
ccdc697
Add MockOptionService for test
JoeRobich Jan 8, 2020
25ea1db
Mark MockOptionService.OptionChanged event as unused
JoeRobich Jan 8, 2020
ec1ce4d
Merge remote-tracking branch 'origin/master' into workspace-registers…
JoeRobich Jan 8, 2020
505d448
Merge branch 'master' into workspace-registers-optionsprovider
JoeRobich Jan 23, 2020
f2d5c95
Ensure OptionPersisters are loaded before creating a workspace
JoeRobich Feb 21, 2020
48bf4d9
Avoid invoking GetOptions from Workspace constructor
mavasani Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/VisualStudio/Core/Def/Implementation/TaskList/CommentTaskTokenSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@ namespace Microsoft.VisualStudio.LanguageServices.Implementation.TaskList
[Export(typeof(IOptionPersister))]
internal class CommentTaskTokenSerializer : IOptionPersister
{
private readonly VisualStudioWorkspace _workspace;
private readonly ITaskList _taskList;
private readonly IGlobalOptionService _globalOptionService;

private string _lastCommentTokenCache = null;

[ImportingConstructor]
public CommentTaskTokenSerializer(
VisualStudioWorkspace workspace,
IGlobalOptionService globalOptionService,
[Import(typeof(SVsServiceProvider))] IServiceProvider serviceProvider)
{
_workspace = workspace;
_globalOptionService = globalOptionService;
mavasani marked this conversation as resolved.
Show resolved Hide resolved

// The SVsTaskList may not be available or doesn't actually implement ITaskList
// in the "devenv /build" scenario
Expand Down Expand Up @@ -68,8 +68,7 @@ private void OnPropertyChanged(object sender, PropertyChangedEventArgs e)

var commentString = GetTaskTokenList(_taskList);

var optionSet = _workspace.Options;
var optionValue = optionSet.GetOption(TodoCommentOptions.TokenList);
var optionValue = _globalOptionService.GetOption(TodoCommentOptions.TokenList);
if (optionValue == commentString)
{
return;
Expand All @@ -79,7 +78,7 @@ private void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
_lastCommentTokenCache = commentString;

// let people to know that comment string has changed
_workspace.SetOptions(optionSet.WithChangedOption(TodoCommentOptions.TokenList, _lastCommentTokenCache));
_globalOptionService.RefreshOption(TodoCommentOptions.TokenList, _lastCommentTokenCache);
}

private static string GetTaskTokenList(ITaskList taskList)
Expand Down
6 changes: 3 additions & 3 deletions src/VisualStudio/Core/Def/RoslynPackage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,12 @@ protected override async Task InitializeAsync(CancellationToken cancellationToke
var method = compilerFailFast.GetMethod(nameof(FailFast.OnFatalException), BindingFlags.Static | BindingFlags.NonPublic);
property.SetValue(null, Delegate.CreateDelegate(property.PropertyType, method));

_workspace = _componentModel.GetService<VisualStudioWorkspace>();
mavasani marked this conversation as resolved.
Show resolved Hide resolved
_workspace.Services.GetService<IExperimentationService>();

// Ensure the options persisters are loaded since we have to fetch options from the shell
_componentModel.GetExtensions<IOptionPersister>();

_workspace = _componentModel.GetService<VisualStudioWorkspace>();
_workspace.Services.GetService<IExperimentationService>();

RoslynTelemetrySetup.Initialize(this);

InitializeColors();
Expand Down
22 changes: 7 additions & 15 deletions ...rkspaces/Core/Portable/Options/EditorConfig/EditorConfigDocumentOptionsProviderFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,32 +6,24 @@

using System;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.ErrorLogger;

namespace Microsoft.CodeAnalysis.Options.EditorConfig
{
[Export(typeof(IDocumentOptionsProviderFactory)), Shared]
[ExportMetadata("Name", PredefinedDocumentOptionsProviderNames.EditorConfig)]
internal sealed class EditorConfigDocumentOptionsProviderFactory : IDocumentOptionsProviderFactory
internal static class EditorConfigDocumentOptionsProviderFactory
{
[ImportingConstructor]
public EditorConfigDocumentOptionsProviderFactory()
{
}

public IDocumentOptionsProvider? TryCreate(Workspace workspace)
public static IDocumentOptionsProvider? TryCreate(Workspace workspace)
mavasani marked this conversation as resolved.
Show resolved Hide resolved
{
if (!ShouldUseNativeEditorConfigSupport(workspace))
Copy link
Contributor

@mavasani mavasani Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
// Simply disable if the feature isn't on
return null;
}

return new EditorConfigDocumentOptionsProvider(workspace.Services.GetRequiredService<IErrorLoggerService>());
return new EditorConfigDocumentOptionsProvider(workspace.Services.GetService<IErrorLoggerService>());
}

private const string LocalRegistryPath = @"Roslyn\Internal\OnOff\Features\";
Expand All @@ -47,9 +39,9 @@ public static bool ShouldUseNativeEditorConfigSupport(Workspace workspace)

private class EditorConfigDocumentOptionsProvider : IDocumentOptionsProvider
{
private readonly IErrorLoggerService _errorLogger;
private readonly IErrorLoggerService? _errorLogger;

public EditorConfigDocumentOptionsProvider(IErrorLoggerService errorLogger)
public EditorConfigDocumentOptionsProvider(IErrorLoggerService? errorLogger)
{
_errorLogger = errorLogger;
}
Expand All @@ -64,9 +56,9 @@ public EditorConfigDocumentOptionsProvider(IErrorLoggerService errorLogger)
private class DocumentOptions : IDocumentOptions
{
private readonly ImmutableDictionary<string, string> _options;
private readonly IErrorLoggerService _errorLogger;
private readonly IErrorLoggerService? _errorLogger;

public DocumentOptions(ImmutableDictionary<string, string> options, IErrorLoggerService errorLogger)
public DocumentOptions(ImmutableDictionary<string, string> options, IErrorLoggerService? errorLogger)
{
_options = options;
_errorLogger = errorLogger;
Expand Down
15 changes: 15 additions & 0 deletions src/Workspaces/Core/Portable/Workspace/Workspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.ErrorLogger;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Internal.Log;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Options.EditorConfig;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Microsoft.CodeAnalysis.Text;
Expand Down Expand Up @@ -79,6 +81,19 @@ protected Workspace(HostServices host, string? workspaceKind)
var info = SolutionInfo.Create(SolutionId.CreateNewId(), VersionStamp.Create());
var emptyOptions = new SerializableOptionSet(languages: ImmutableHashSet<string>.Empty, _optionService, serializableOptions: ImmutableHashSet<IOption>.Empty, values: ImmutableDictionary<OptionKey, object?>.Empty);
_latestSolution = CreateSolution(info, emptyOptions);

TryRegisterDocumentOptionsProvider();
mavasani marked this conversation as resolved.
Show resolved Hide resolved
}

internal void TryRegisterDocumentOptionsProvider()
{
var documentOptionsProvider = EditorConfigDocumentOptionsProviderFactory.TryCreate(this);
if (documentOptionsProvider is null)
{
return;
}

_optionService.RegisterDocumentOptionsProvider(documentOptionsProvider);
}

internal void LogTestMessage(string message)
Expand Down