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.

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

Switch SG mode to 'balanced' by default #73618

Merged
merged 13 commits into from
May 22, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using Microsoft.CodeAnalysis.Editor.UnitTests;
using Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces;
using Microsoft.CodeAnalysis.Formatting;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Indentation;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.Extensions;
Expand Down Expand Up @@ -468,10 +469,15 @@ End Class
var classC = classD.BaseType;
}

[Fact]
public async Task TestGetCompilationOnCrossLanguageDependentProjectChanged()
[Theory, CombinatorialData]
internal async Task TestGetCompilationOnCrossLanguageDependentProjectChanged(
SourceGeneratorExecutionPreference preference)
{
using var workspace = CreateWorkspace();
using var workspace = CreateWorkspace(composition: EditorTestCompositions.EditorFeatures.AddParts(typeof(TestWorkspaceConfigurationService)));

var configService = workspace.ExportProvider.GetExportedValue<TestWorkspaceConfigurationService>();
configService.Options = new WorkspaceConfigurationOptions(SourceGeneratorExecution: preference);

var solutionX = workspace.CurrentSolution;

var document1 = new EditorTestHostDocument(@"public class C { }");
Expand Down Expand Up @@ -514,7 +520,12 @@ End Class
var classDz = compilation2Z.SourceModule.GlobalNamespace.GetTypeMembers("D").Single();
var classCz = classDz.BaseType;

Assert.Equal(TypeKind.Error, classCz.TypeKind);
// In balanced mode the skeleton won't be regenerated. So the downstream project won't see the change to
// remove the class.
if (preference is SourceGeneratorExecutionPreference.Automatic)
Assert.Equal(TypeKind.Error, classCz.TypeKind);
else
Assert.Equal(TypeKind.Class, classCz.TypeKind);
}

[WpfFact]
Expand Down Expand Up @@ -575,12 +586,19 @@ End Class
}
}

[WpfFact]
public async Task TestGetCompilationOnCrossLanguageDependentProjectChangedInProgress()
[WpfTheory, CombinatorialData]
internal async Task TestGetCompilationOnCrossLanguageDependentProjectChangedInProgress(
SourceGeneratorExecutionPreference preference)
{
var composition = EditorTestCompositions.EditorFeatures.AddParts(typeof(TestDocumentTrackingService));
var composition = EditorTestCompositions.EditorFeatures.AddParts(
typeof(TestDocumentTrackingService),
typeof(TestWorkspaceConfigurationService));

using var workspace = CreateWorkspace(disablePartialSolutions: false, composition: composition);

var configService = workspace.ExportProvider.GetExportedValue<TestWorkspaceConfigurationService>();
configService.Options = new WorkspaceConfigurationOptions(SourceGeneratorExecution: preference);

var trackingService = (TestDocumentTrackingService)workspace.Services.GetRequiredService<IDocumentTrackingService>();
var solutionX = workspace.CurrentSolution;

Expand Down Expand Up @@ -662,8 +680,12 @@ End Class
}
}

// Should find now that we're going a normal compilation.
Assert.True(foundTheError, "Did not find error");
// In balanced mode the skeleton won't be regenerated. So the downstream project won't see the change to
// remove the class. So it will not find the error symbol.
if (preference is SourceGeneratorExecutionPreference.Automatic)
Assert.True(foundTheError);
else
Assert.False(foundTheError);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ public partial class GeneratedClass : IInterface { }
</Document>
</Project>
</Workspace>, host:=host, renameTo:="A", sourceGenerator:=New GeneratorThatImplementsInterfaceMethod())

End Using
End Sub

Expand Down
12 changes: 8 additions & 4 deletions src/EditorFeatures/Test2/Rename/RenameEngineResult.vb
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@ Imports System.Collections.Immutable
Imports System.Threading
Imports Microsoft.CodeAnalysis
Imports Microsoft.CodeAnalysis.CodeActions
Imports Microsoft.CodeAnalysis.CodeCleanup
Imports Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces
Imports Microsoft.CodeAnalysis.Host
Imports Microsoft.CodeAnalysis.Options
Imports Microsoft.CodeAnalysis.Remote.Testing
Imports Microsoft.CodeAnalysis.Rename
Imports Microsoft.CodeAnalysis.Rename.ConflictEngine
Expand Down Expand Up @@ -57,18 +55,24 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename
host As RenameTestHost,
Optional renameOptions As SymbolRenameOptions = Nothing,
Optional expectFailure As Boolean = False,
Optional sourceGenerator As ISourceGenerator = Nothing) As RenameEngineResult
Optional sourceGenerator As ISourceGenerator = Nothing,
Optional executionPreference As SourceGeneratorExecutionPreference = SourceGeneratorExecutionPreference.Automatic) As RenameEngineResult

Dim composition = EditorTestCompositions.EditorFeatures.AddParts(
GetType(NoCompilationContentTypeLanguageService),
GetType(NoCompilationContentTypeDefinitions),
GetType(WorkspaceTestLogger))
GetType(WorkspaceTestLogger),
GetType(TestWorkspaceConfigurationService))

If host = RenameTestHost.OutOfProcess_SingleCall OrElse host = RenameTestHost.OutOfProcess_SplitCall Then
composition = composition.WithTestHostParts(TestHost.OutOfProcess)
End If

Dim workspace = TestWorkspace.CreateWorkspace(workspaceXml, composition:=composition)

Dim configService = workspace.ExportProvider.GetExportedValue(Of TestWorkspaceConfigurationService)
configService.Options = New WorkspaceConfigurationOptions(SourceGeneratorExecution:=executionPreference)

workspace.Services.SolutionServices.SetWorkspaceTestOutput(helper)

If sourceGenerator IsNot Nothing Then
Expand Down
6 changes: 2 additions & 4 deletions src/EditorFeatures/Test2/Rename/RenameTestHelpers.vb
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,8 @@ Imports Microsoft.CodeAnalysis.Editor.Implementation.RenameTracking
Imports Microsoft.CodeAnalysis.Editor.Shared.Utilities
Imports Microsoft.CodeAnalysis.Editor.UnitTests.RenameTracking
Imports Microsoft.CodeAnalysis.Editor.UnitTests.Utilities.GoToHelpers
Imports Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces
Imports Microsoft.CodeAnalysis.Options
Imports Microsoft.CodeAnalysis.Shared.TestHooks
Imports Microsoft.CodeAnalysis.Rename
Imports Microsoft.CodeAnalysis.Text
Imports Microsoft.CodeAnalysis.Text.Shared.Extensions
Imports Microsoft.VisualStudio.Text
Expand All @@ -22,10 +20,10 @@ Imports Microsoft.VisualStudio.Text.Tagging

Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename
Friend Module RenameTestHelpers

Private ReadOnly s_composition As TestComposition = EditorTestCompositions.EditorFeaturesWpf.AddParts(
GetType(MockDocumentNavigationServiceFactory),
GetType(MockPreviewDialogService))
GetType(MockPreviewDialogService),
GetType(TestWorkspaceConfigurationService))

Private Function GetSessionInfo(workspace As EditorTestWorkspace) As (document As Document, textSpan As TextSpan)
Dim hostdoc = workspace.DocumentWithCursor
Expand Down
8 changes: 6 additions & 2 deletions src/EditorFeatures/Test2/Rename/RenameViewModelTests.vb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Imports System.Threading
Imports Microsoft.CodeAnalysis.Editor.Implementation.InlineRename
Imports Microsoft.CodeAnalysis.Editor.InlineRename
Imports Microsoft.CodeAnalysis.Editor.[Shared].Utilities
Imports Microsoft.CodeAnalysis.Host
Imports Microsoft.CodeAnalysis.InlineRename
Imports Microsoft.CodeAnalysis.Options
Imports Microsoft.CodeAnalysis.[Shared].TestHooks
Expand Down Expand Up @@ -547,8 +548,8 @@ class D : B
Optional renameFile As Boolean = False,
Optional resolvableConflictText As String = Nothing,
Optional unresolvableConflictText As String = Nothing,
Optional severity As RenameDashboardSeverity = RenameDashboardSeverity.None
) As Tasks.Task
Optional severity As RenameDashboardSeverity = RenameDashboardSeverity.None,
Optional executionPreference As SourceGeneratorExecutionPreference = SourceGeneratorExecutionPreference.Automatic) As Task
Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit odd to me that rename tests are concerning themselves with this -- it seems like this is just something the underlying system should be handling. But maybe I'm missing something here since I'm ramping back up on this space.


Using workspace = CreateWorkspaceWithWaiter(test, host)
Dim globalOptions = workspace.GetService(Of IGlobalOptionService)()
Expand All @@ -557,6 +558,9 @@ class D : B
globalOptions.SetGlobalOption(InlineRenameSessionOptionsStorage.RenameInComments, renameInComments)
globalOptions.SetGlobalOption(InlineRenameSessionOptionsStorage.RenameFile, renameFile)

Dim configService = workspace.ExportProvider.GetExportedValue(Of TestWorkspaceConfigurationService)
configService.Options = New WorkspaceConfigurationOptions(SourceGeneratorExecution:=executionPreference)

Dim cursorDocument = workspace.Documents.Single(Function(d) d.CursorPosition.HasValue)
Dim cursorPosition = cursorDocument.CursorPosition.Value

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@
using System.Runtime.Loader;
using System.Text.Json;
using Microsoft.CodeAnalysis.Contracts.Telemetry;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.LanguageServer;
using Microsoft.CodeAnalysis.LanguageServer.BrokeredServices;
using Microsoft.CodeAnalysis.LanguageServer.HostWorkspace;
using Microsoft.CodeAnalysis.LanguageServer.LanguageServer;
using Microsoft.CodeAnalysis.LanguageServer.Logging;
using Microsoft.CodeAnalysis.LanguageServer.Services;
using Microsoft.CodeAnalysis.LanguageServer.StarredSuggestions;
using Microsoft.CodeAnalysis.Options;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Console;
using Roslyn.Utilities;
Expand Down Expand Up @@ -82,6 +84,11 @@ static async Task RunAsync(ServerConfiguration serverConfiguration, Cancellation

using var exportProvider = await ExportProviderBuilder.CreateExportProviderAsync(extensionManager, serverConfiguration.DevKitDependencyPath, loggerFactory);

// LSP server doesn't have the pieces yet to support 'balanced' mode for source-generators. Hardcode us to
// 'automatic' for now.
var globalOptionService = exportProvider.GetExportedValue<IGlobalOptionService>();
globalOptionService.SetGlobalOption(WorkspaceConfigurationOptionsStorage.SourceGeneratorExecution, SourceGeneratorExecutionPreference.Automatic);

// The log file directory passed to us by VSCode might not exist yet, though its parent directory is guaranteed to exist.
Directory.CreateDirectory(serverConfiguration.ExtensionLogDirectory);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,5 @@ public static WorkspaceConfigurationOptions GetWorkspaceConfigurationOptions(thi
SourceGeneratorExecutionPreferenceUtilities.GetEditorConfigString));

public static readonly Option2<bool> SourceGeneratorExecutionBalancedFeatureFlag = new(
"dotnet_source_generator_execution_balanced_feature_flag", false);
"dotnet_source_generator_execution_balanced_feature_flag", true);
Copy link
Member Author

Choose a reason for hiding this comment

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

note: we still have the feature flag. So we still have control-tower support to flip this back to 'false' if we run into any problems with this in the wild.

Copy link
Member

Choose a reason for hiding this comment

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

Think we might need to have a modification here to leave it false by default for VSCode - I haven't yet had a chance to implement balanced mode there yet. Easiest way if you want to change the default is have a line of code in the language server project to explicitly set this to false - https://github.com/dotnet/roslyn/blob/main/src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs#L90

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
Imports Microsoft.CodeAnalysis
Imports Microsoft.CodeAnalysis.Diagnostics
Imports Microsoft.CodeAnalysis.Editor.Shared.Utilities
Imports Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces
Imports Microsoft.CodeAnalysis.Editor.UnitTests
Imports Microsoft.CodeAnalysis.Host
Imports Microsoft.CodeAnalysis.Shared.TestHooks
Imports Microsoft.CodeAnalysis.Test.Utilities
Imports Microsoft.CodeAnalysis.Text
Expand Down Expand Up @@ -122,16 +123,23 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.SolutionExplorer
End Using
End Function

<WpfFact>
Public Async Function ChangeToRemoveAllGeneratedDocumentsUpdatesListCorrectly() As Task
<WpfTheory, CombinatorialData>
Friend Async Function ChangeToRemoveAllGeneratedDocumentsUpdatesListCorrectly(
preference As SourceGeneratorExecutionPreference) As Task
Dim workspaceXml =
<Workspace>
<Project Language="C#" CommonReferences="true" LanguageVersion="Preview">
<AdditionalDocument FilePath="Test1.txt"></AdditionalDocument>
</Project>
</Workspace>

Using workspace = EditorTestWorkspace.Create(workspaceXml)
Using workspace = EditorTestWorkspace.Create(
workspaceXml,
composition:=EditorTestCompositions.EditorFeatures.AddParts(GetType(TestWorkspaceConfigurationService)))

Dim configService = workspace.ExportProvider.GetExportedValue(Of TestWorkspaceConfigurationService)
configService.Options = New WorkspaceConfigurationOptions(SourceGeneratorExecution:=preference)

Dim projectId = workspace.Projects.Single().Id
Dim source = CreateItemSourceForAnalyzerReference(workspace, projectId)
Dim generatorItem = Assert.IsAssignableFrom(Of SourceGeneratorItem)(Assert.Single(source.Items))
Expand All @@ -147,19 +155,31 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.SolutionExplorer

Await WaitForGeneratorsAndItemSourcesAsync(workspace)

Assert.IsType(Of NoSourceGeneratedFilesPlaceholderItem)(Assert.Single(generatorFilesItemSource.Items))
' In balanced-mode the SG file won't go away until a save/build happens.
If preference = SourceGeneratorExecutionPreference.Automatic Then
Assert.IsType(Of NoSourceGeneratedFilesPlaceholderItem)(Assert.Single(generatorFilesItemSource.Items))
Else
Assert.IsType(Of SourceGeneratedFileItem)(Assert.Single(generatorFilesItemSource.Items))
End If
End Using
End Function

<WpfFact>
Public Async Function AddingAGeneratedDocumentUpdatesListCorrectly() As Task
<WpfTheory, CombinatorialData>
Friend Async Function AddingAGeneratedDocumentUpdatesListCorrectly(
preference As SourceGeneratorExecutionPreference) As Task
Dim workspaceXml =
<Workspace>
<Project Language="C#" CommonReferences="true" LanguageVersion="Preview">
</Project>
</Workspace>

Using workspace = EditorTestWorkspace.Create(workspaceXml)
Using workspace = EditorTestWorkspace.Create(
workspaceXml,
composition:=EditorTestCompositions.EditorFeatures.AddParts(GetType(TestWorkspaceConfigurationService)))

Dim configService = workspace.ExportProvider.GetExportedValue(Of TestWorkspaceConfigurationService)
configService.Options = New WorkspaceConfigurationOptions(SourceGeneratorExecution:=preference)

Dim projectId = workspace.Projects.Single().Id
Dim source = CreateItemSourceForAnalyzerReference(workspace, projectId)
Dim generatorItem = Assert.IsAssignableFrom(Of SourceGeneratorItem)(Assert.Single(source.Items))
Expand All @@ -179,7 +199,12 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.SolutionExplorer

Await WaitForGeneratorsAndItemSourcesAsync(workspace)

Assert.IsType(Of SourceGeneratedFileItem)(Assert.Single(generatorFilesItemSource.Items))
' In balanced-mode the SG file won't be created until a save/build happens.
If preference = SourceGeneratorExecutionPreference.Automatic Then
Assert.IsType(Of SourceGeneratedFileItem)(Assert.Single(generatorFilesItemSource.Items))
Else
Assert.IsType(Of NoSourceGeneratedFilesPlaceholderItem)(Assert.Single(generatorFilesItemSource.Items))
End If

' Add a second item and see if it updates correctly again
workspace.OnAdditionalDocumentAdded(
Expand All @@ -188,7 +213,12 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.SolutionExplorer
"Test2.txt"))

Await WaitForGeneratorsAndItemSourcesAsync(workspace)
Assert.Equal(2, generatorFilesItemSource.Items.Cast(Of SourceGeneratedFileItem)().Count())

If preference = SourceGeneratorExecutionPreference.Automatic Then
Assert.Equal(2, generatorFilesItemSource.Items.Cast(Of SourceGeneratedFileItem)().Count())
Else
Assert.Equal(1, generatorFilesItemSource.Items.Cast(Of NoSourceGeneratedFilesPlaceholderItem)().Count())
End If
End Using
End Function

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,8 @@ private async Task OnBatchScopeDisposedMaybeAsync(bool useAsync)
var additionalDocumentsToOpen = new List<(DocumentId documentId, SourceTextContainer textContainer)>();
var analyzerConfigDocumentsToOpen = new List<(DocumentId documentId, SourceTextContainer textContainer)>();

var hasAnalyzerChanges = _analyzersAddedInBatch.Count > 0 || _analyzersRemovedInBatch.Count > 0;

await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsync, solutionChanges =>
{
_sourceFiles.UpdateSolutionForBatch(
Expand Down Expand Up @@ -667,25 +669,21 @@ await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsyn
}).ConfigureAwait(false);

foreach (var (documentId, textContainer) in documentsToOpen)
{
await _projectSystemProjectFactory.ApplyChangeToWorkspaceMaybeAsync(useAsync, w => w.OnDocumentOpened(documentId, textContainer)).ConfigureAwait(false);
}

foreach (var (documentId, textContainer) in additionalDocumentsToOpen)
{
await _projectSystemProjectFactory.ApplyChangeToWorkspaceMaybeAsync(useAsync, w => w.OnAdditionalDocumentOpened(documentId, textContainer)).ConfigureAwait(false);
}

foreach (var (documentId, textContainer) in analyzerConfigDocumentsToOpen)
{
await _projectSystemProjectFactory.ApplyChangeToWorkspaceMaybeAsync(useAsync, w => w.OnAnalyzerConfigDocumentOpened(documentId, textContainer)).ConfigureAwait(false);
}

// Give the host the opportunity to check if those files are open
if (documentFileNamesAdded.Count > 0)
{
await _projectSystemProjectFactory.RaiseOnDocumentsAddedMaybeAsync(useAsync, documentFileNamesAdded.ToImmutable()).ConfigureAwait(false);
}

// If we added or removed analyzers, then re-run all generators to bring them up to date.
if (hasAnalyzerChanges)
_projectSystemProjectFactory.Workspace.EnqueueUpdateSourceGeneratorVersion(projectId: null, forceRegeneration: true);
Copy link
Member

Choose a reason for hiding this comment

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

Why is projectId null here?

Copy link
Member Author

Choose a reason for hiding this comment

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

that means "redo all generators".

Copy link
Member

Choose a reason for hiding this comment

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

@CyrusNajmabadi For all projects though?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,16 @@

using System;
using System.Composition;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Host.Mef;

namespace Roslyn.Test.Utilities
{
[Export]
[ExportWorkspaceService(typeof(IWorkspaceConfigurationService), ServiceLayer.Test)]
[Shared]
[PartNotDiscoverable]
internal sealed class TestWorkspaceConfigurationService : IWorkspaceConfigurationService
{
public WorkspaceConfigurationOptions Options { get; set; }
namespace Roslyn.Test.Utilities;

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public TestWorkspaceConfigurationService()
{
}
}
[Export, Shared, PartNotDiscoverable]
[ExportWorkspaceService(typeof(IWorkspaceConfigurationService), ServiceLayer.Test)]
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class TestWorkspaceConfigurationService() : IWorkspaceConfigurationService
{
public WorkspaceConfigurationOptions Options { get; set; }
}
Loading