Fix vscode fuse feature flag #10169

ryzngard · 2024-03-26T20:53:26Z

Prior to this change, the feature flag was being serialized and deserialized as part of the RazorConfiguration. This works fine for VS, since the razor extension is the one serializing. However, if we want to store that information in the serialization on vs code, we'd have to have a hook into Roslyn to do that.

Instead, this adds a section of the configuration specifically labeled for feature flags. This should not be serialized, and should be filled in based on the LanguageServerConfiguration. This allows us to pass the flag into rzls and retrieve for the correct implementation when running the ProjectEngine

...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationFileChangeEventArgs.cs

davidwengier

This is a surprisingly tricky bit of plumbing. My comments are mostly just me thinking out loud, I'm curious what you think

...icrosoft.AspNetCore.Razor.ProjectEngineHost/Serialization/MessagePack/SerializationFormat.cs

src/Shared/Microsoft.AspNetCore.Razor.Serialization.Json/SerializationFormat.cs

davidwengier · 2024-03-26T21:25:58Z

....Razor.ProjectEngineHost/Serialization/MessagePack/Formatters/RazorConfigurationFormatter.cs

+        var forceRuntimeCodeGeneration = false;
+
+        if (reader.NextMessagePackType is MessagePackType.Boolean)
+        {
+            forceRuntimeCodeGeneration = reader.ReadBoolean();
+            count -= 1;
+        }


Why is this back again?

Because I'm so afraid to break things!

I think it's probably safe to remove at this point.

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorConfiguration.cs

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorLanguageFeatureFlags.cs

...icrosoft.VisualStudio.LanguageServices.Razor/ProjectSystem/DefaultWindowsRazorProjectHost.cs

davidwengier

Looks good, but a couple of places to update before merging.

...icrosoft.VisualStudio.LanguageServices.Razor/ProjectSystem/DefaultWindowsRazorProjectHost.cs

...icrosoft.AspNetCore.Razor.ProjectEngineHost/Serialization/MessagePack/SerializationFormat.cs

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorLanguageFeatureFlags.cs

DustinCampbell · 2024-03-27T17:40:36Z

....Razor.ProjectEngineHost/Serialization/MessagePack/Formatters/RazorConfigurationFormatter.cs

@@ -47,7 +39,7 @@ public override RazorConfiguration Deserialize(ref MessagePackReader reader, Ser
            ? version
            : RazorLanguageVersion.Version_2_1;

-        return new(languageVersion, configurationName, extensions, ForceRuntimeCodeGeneration: forceRuntimeCodeGeneration);
+        return new(languageVersion, configurationName, extensions, RazorLanguageFeatureFlags.Default);


Should we be serializing the language feature flags?

DustinCampbell · 2024-03-27T17:44:23Z

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/ProjectSystem/RazorProjectInfo.cs

@@ -12,20 +12,20 @@

 namespace Microsoft.AspNetCore.Razor.ProjectSystem;

-internal sealed class RazorProjectInfo
+internal sealed record class RazorProjectInfo


Does making this a record help in some way? Note that the equality of this record won't be correct because it will need to use SequenceEquals for the Documents property.

It made it cleaner to add a with for modifying readonly properties than having to write out new(...)

DustinCampbell · 2024-03-27T17:45:34Z

....Razor.ProjectEngineHost/Serialization/MessagePack/Formatters/RazorConfigurationFormatter.cs

@@ -47,7 +47,7 @@ public override RazorConfiguration Deserialize(ref MessagePackReader reader, Ser
            ? version
            : RazorLanguageVersion.Version_2_1;

-        return new(languageVersion, configurationName, extensions, ForceRuntimeCodeGeneration: forceRuntimeCodeGeneration);
+        return new(languageVersion, configurationName, extensions);


Why are we bothering to read forceRuntimeCodeGeneration above if it's not used any longer? It looks like the read was removed from the JSON serialization.

There is potential overlap where we have it written to a file right?

It's super small though. Is it worth protecting between 17.10p2 and 17.10p3? (Was this even in 17.10p2?)

Only p3, which we could port to remove

oh actually, it's shipped in vs code already.

We need a flow chart!

...icrosoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationFileChangeEventArgsTest.cs

src/Shared/Microsoft.AspNetCore.Razor.Serialization.Json/SerializationFormat.cs

DustinCampbell · 2024-03-27T17:52:47Z

....Razor.ProjectEngineHost/Serialization/MessagePack/Formatters/RazorConfigurationFormatter.cs

+        var forceRuntimeCodeGeneration = false;
+
+        if (reader.NextMessagePackType is MessagePackType.Boolean)
+        {
+            forceRuntimeCodeGeneration = reader.ReadBoolean();
+            count -= 1;
+        }


I think it's probably safe to remove at this point.

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/LanguageServerFlags.cs

ryzngard · 2024-03-27T22:48:19Z

.../Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs

@@ -63,14 +64,14 @@ public async Task ProjectConfigurationFileChanged_Removed_NonNormalizedPaths()
            .Setup(x => x.AddProject(
                projectInfo.FilePath,
                intermediateOutputPath,
-                projectInfo.Configuration,
+                It.IsAny<RazorConfiguration>(),


For the curious or inquisitive person who finds this, I had to change this to IsAny because we're mocking our own code and the path to add/update a project enforces that the configuration is up to date with the feature flags. That causes reference equality to fail and thus moq to fail.

DustinCampbell · 2024-03-28T17:35:41Z

src/Razor/src/Microsoft.VisualStudio.Editor.Razor/ProjectSystem/FallbackProjectManager.cs

@@ -107,11 +107,13 @@ private async Task AddFallbackProjectAsync(ProjectId projectId, string filePath,

        var rootNamespace = project.DefaultNamespace;

+        var configuration = FallbackRazorConfiguration.Latest with { LanguageServerFlags = _languageServerFeatureOptions.ToLanguageServerFlags() };


Pretty change! 😄

chsienki

Compiler changes LGTM

VS Code portion of dotnet/razor#10169

ryzngard added 2 commits March 26, 2024 11:48

Move feature flags to their own portion of RazorConfiguration

8042183

Flow feature flag through vscode

58cb774

ryzngard commented Mar 26, 2024

View reviewed changes

...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationFileChangeEventArgs.cs Show resolved Hide resolved

ryzngard changed the title ~~[DRAFT] Fix vscode fuse feature flag~~ Fix vscode fuse feature flag Mar 26, 2024

ryzngard marked this pull request as ready for review March 26, 2024 21:00

ryzngard requested review from a team as code owners March 26, 2024 21:00

davidwengier reviewed Mar 26, 2024

View reviewed changes

ryzngard added 4 commits March 26, 2024 16:45

PR Feedback

5ac1765

Add a comment

ddad5fd

Missed one

d315b68

Missed a null removal

d16f04e

davidwengier approved these changes Mar 27, 2024

View reviewed changes

...icrosoft.VisualStudio.LanguageServices.Razor/ProjectSystem/DefaultWindowsRazorProjectHost.cs Outdated Show resolved Hide resolved

DustinCampbell reviewed Mar 27, 2024

View reviewed changes

ryzngard added 4 commits March 27, 2024 14:45

Make serialization 3 again

42fcd73

Use TestLanguageServerFeatureOptions.Instance

1ad61fa

Formatting, and add ToLanguageServerFlags

9f82088

Fix tests

35fe441

ryzngard commented Mar 27, 2024

View reviewed changes

DustinCampbell reviewed Mar 28, 2024

View reviewed changes

DustinCampbell approved these changes Mar 28, 2024

View reviewed changes

Fix test

919b6ff

chsienki approved these changes Mar 28, 2024

View reviewed changes

No really use the right number in serialization

331659e

ryzngard enabled auto-merge (squash) March 28, 2024 21:39

fix version in test data

5d44c7d

ryzngard merged commit a4be655 into dotnet:main Mar 29, 2024
12 checks passed

dotnet-policy-service bot added this to the Next milestone Mar 29, 2024

ryzngard deleted the vscode_featureflag branch March 29, 2024 00:21

ryzngard mentioned this pull request Apr 2, 2024

Add flag for ForceRuntimeCodeGeneration dotnet/vscode-csharp#7017

Merged

ryzngard added a commit to dotnet/vscode-csharp that referenced this pull request Apr 3, 2024

Add flag for ForceRuntimeCodeGeneration (#7017)

40ca2e3

VS Code portion of dotnet/razor#10169

This was referenced Apr 4, 2024

[Automated] PRs inserted in VS build feature.debugger.main-34803.197 #10218

Closed

[Automated] PRs inserted in VS build main-34803.156 #10219

Closed

[Automated] PRs inserted in VS build feature.dotnetVS-34808.241 #10241

Closed

Cosifne modified the milestones: Next, 17.11 P2 May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix vscode fuse feature flag #10169

Fix vscode fuse feature flag #10169

ryzngard commented Mar 26, 2024

davidwengier left a comment

davidwengier Mar 26, 2024

ryzngard Mar 26, 2024

DustinCampbell Mar 27, 2024

davidwengier left a comment

DustinCampbell Mar 27, 2024

DustinCampbell Mar 27, 2024

ryzngard Mar 27, 2024

DustinCampbell Mar 27, 2024

ryzngard Mar 27, 2024

DustinCampbell Mar 28, 2024

ryzngard Mar 28, 2024

ryzngard Mar 28, 2024

davidwengier Mar 28, 2024

DustinCampbell Mar 27, 2024

ryzngard Mar 27, 2024

DustinCampbell Mar 28, 2024

DustinCampbell Mar 28, 2024

chsienki left a comment

		@@ -107,11 +107,13 @@ private async Task AddFallbackProjectAsync(ProjectId projectId, string filePath,

		var rootNamespace = project.DefaultNamespace;

		var configuration = FallbackRazorConfiguration.Latest with { LanguageServerFlags = _languageServerFeatureOptions.ToLanguageServerFlags() };

Fix vscode fuse feature flag #10169

Fix vscode fuse feature flag #10169

Conversation

ryzngard commented Mar 26, 2024

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki left a comment

Choose a reason for hiding this comment