Update project configuration from Roslyn info #11092

davidwengier · 2024-10-25T04:58:40Z

Fixes #10736 and brings in IDE support for the new Roslyn tokenizer, which makes this work:

…uration

davidwengier · 2024-10-25T05:07:22Z

A better fix here would be to get the SuppressAddComponentParameter flag off of RazorConfiguration and put it on ProjectWorkspaceState, then I wouldn't have needed to hack things into the ProjectWorkspaceStateGenerator as I have. That would require compiler changes though, so I didn't bother since the issue doesn't seem to be fixed either.

The other option is just to rename ProjectWorkspaceStateGenerator to RoslynProjectProcessor or something, since that is what it really does anyway :)

DustinCampbell

A better fix here would be to get the SuppressAddComponentParameter flag off of RazorConfiguration and put it on ProjectWorkspaceState.

Unfortunately, I think that might be a hackier fix. I took a quick look, even though this is in draft, and I'm not comfortable elevating ProjectWorkspaceState to hold more options. IMO, data needed to configure the compiler should go on RazorConfiguration.

...azor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/ProjectSystem/ProjectWorkspaceState.cs

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/RazorProjectInfoFactory.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs

DustinCampbell · 2024-10-25T16:58:41Z

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/ProjectSystem/RemoteProjectSnapshot.cs

@@ -19,6 +19,7 @@
 using Microsoft.CodeAnalysis.Razor;
 using Microsoft.CodeAnalysis.Razor.Compiler.CSharp;
 using Microsoft.CodeAnalysis.Razor.ProjectSystem;
+using Microsoft.NET.Sdk.Razor.SourceGenerators;


This feels wrong from a layering perspective. Note that there are (sadly) multiple "features" for configuring the Razor compiler's parse options. This is using the source generator's implementation of IConfigureRazorParserOptionsFeature rather than the newer one that is actually used in most cases: IRazorParserOptionsFactoryProjectFeature.

.../Microsoft.AspNetCore.Razor.ProjectEngineHost.Test/Serialization/SerializerValidationTest.cs

davidwengier · 2024-10-26T02:41:24Z

it isn't clear to me why CSharpLanguageVersion exists on ProjectWorkspaceState. Seems like this makes better sense on RazorConfiguration?

Noting that I am being descriptive, not prescriptive, but in VS at least, ProjectWorkspaceState is "things that come from Roslyn", and RazorConfiguration is "things that come from the project system". This is ultimately why the SuppressAddComponentParameter change didn't work in VS - that info simply isn't present in the data we receive from the project system, so the value in the configuration is always default.

IMO, data needed to configure the compiler should go on RazorConfiguration.

No argument from me, and with this PR allowing the ProjectWorkspaceStateGenerator to update the configuration as well, that is definitely something we can do. I'm not necessarily a fan of our compiler configuration being dependant on where each piece of data comes from, however the changes in this PR around UseRoslynTokenizer prioritised expediency over everything. Also, to do this properly (ie, not just in a way that uses the configuration properties in the configure lambda) this would require changes to the compiler and how it gets the UseRoslynTokenizer configuration, which was definitely not in scope for the other PR. In general though, I do not like the configure lambda and would happily work to move away from it entirely. Too much potential for bugs when specific calls are missing, and using a strongly typed configuration object is much preferred to me.

its purpose is just to configure RazorCodeGenerationOptions.
This is using the source generator's implementation of IConfigureRazorParserOptionsFeature rather than the newer one that is actually used in most cases: IRazorParserOptionsFactoryProjectFeature.

I am unfamiliar with all 3 of these types, so thank you for the pointers. The current code is just me copying code from the compiler because, as I mentioned above, expediency was the goal.

… overload of the configuration object

…e it comes from

davidwengier · 2024-10-28T03:51:07Z

Okay, pushed up an update to move UseRoslynTokenizer and CSharpLanguageVersion to RazorConfiguration, since they configure the project engine. I would argue RootNamespace should move there too, and in fact it has special handling in some specific cases that could be removed if we did so. I decided not to do that now, and I haven't changed the compiler parts of the wire-up, because I think this is still desired for 17.13 Preview 1, and that work does not seem trivial. It also should probably have some input from the compiler team. I certainly don't feel qualified :) Will happily log issues and we can follow up, unless people have an issue with that approach.

I also took the liberty of renaming some classes because "WorkspaceProject" and "ProjectWorkspace" seem to both mean "Roslyn", so I just called them "Roslyn".

Also taking this out of draft since I found and fixed the issue with SuppressAddComponentParameter.

DustinCampbell · 2024-10-28T15:06:19Z

it isn't clear to me why CSharpLanguageVersion exists on ProjectWorkspaceState. Seems like this makes better sense on RazorConfiguration?

Noting that I am being descriptive, not prescriptive, but in VS at least, ProjectWorkspaceState is "things that come from Roslyn", and RazorConfiguration is "things that come from the project system". This is ultimately why the SuppressAddComponentParameter change didn't work in VS - that info simply isn't present in the data we receive from the project system, so the value in the configuration is always default.

Not related to your pull request at all, but ProjectWorkspaceState is probably something that should just go away. It has an especially bad design smell because it's exposed on IProjectSnapshot. So, there's a IProjectSnapshot.GetTagHelpersAsync(...) method and there's an IProjectSnapshot.ProjectWorkspaceState.TagHelpers property. I've got private changes to address that at some point.

DustinCampbell

This is definitely a big improvement! Thanks for your hard work cleaning much of this up.

...oft.AspNetCore.Razor.Microbenchmarks/Resources/Telerik/Kendo.Mvc.Examples.project.razor.json

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

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs

src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/IRoslynProjectChangeProcessor.cs

DustinCampbell · 2024-10-28T15:27:54Z

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/RazorProjectServiceTest.cs

@@ -66,7 +65,7 @@ await _projectManager.UpdateAsync(updater =>
            updater.ProjectAdded(hostProject);
        });

-        var projectWorkspaceState = ProjectWorkspaceState.Create(LanguageVersion.LatestMajor);
+        var projectWorkspaceState = ProjectWorkspaceState.Default;


Does this still represent a project change as the test requires?

...ft.VisualStudio.LanguageServices.Razor.Test/ProjectSystem/RoslynProjectChangeDetectorTest.cs

.../test/Microsoft.VisualStudio.LanguageServices.Razor.Test/RoslynProjectChangeProcessorTest.cs

...isualStudio.LanguageServices.Razor.Test/VsSolutionUpdatesProjectSnapshotChangeTriggerTest.cs

chsienki

Compiler changes LGTM

davidwengier · 2024-10-28T19:16:29Z

ProjectWorkspaceState is probably something that should just go away

Agreed. Now it is just tag helpers, and the name makes little sense.

DustinCampbell · 2024-10-28T21:54:36Z

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/OpenDocumentGeneratorTest.cs

+            var changed = _hostProject1 with
+            {
+                Configuration = _hostProject1.Configuration with
+                {
+                    CSharpLanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8
+                }
+            };


I'm pretty sure you can use dotted names in with. Doesn't block this PR, but good for a follow up.

Suggested change

var changed = _hostProject1 with

{

Configuration = _hostProject1.Configuration with

{

CSharpLanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8

}

};

var changed = _hostProject1 with

{

Configuration.CSharpLanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8

};

Actually, it looks like you can't do this, but I was sure we added it. Sigh...

DustinCampbell · 2024-10-28T21:56:46Z

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/OpenDocumentGeneratorTest.cs

+            var changed = _hostProject1 with
+            {
+                Configuration = _hostProject1.Configuration with
+                {
+                    CSharpLanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8
+                }
+            };


Non-blocking!

Suggested change

var changed = _hostProject1 with

{

Configuration = _hostProject1.Configuration with

{

CSharpLanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8

}

};

var changed = _hostProject1 with

{

Configuration.CSharpLanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8

};

This reverts commit a37ee12, reversing changes made to ad74439.

This reverts commit a37ee12, reversing changes made to ad74439. We'll have to do another dual insertion into VS Code too, I guess

davidwengier added 4 commits October 25, 2024 15:06

Add UseRoslynTokenizer onto ProjectWorkspaceState and plumb through

d81dd60

Update tests

1e66056

Invalidate project engine when UseRoslynTokenizer changes

6702e25

Allow the workspace state generator to also update the project config…

51ba92b

…uration

DustinCampbell reviewed Oct 25, 2024

View reviewed changes

DustinCampbell mentioned this pull request Oct 25, 2024

Wire up UseRoslynTokenizer in Visual Studio #11091

Closed

davidwengier added 8 commits October 28, 2024 12:35

Fix updating of SuppressAddComponentParameter

349b158

Misc PR feedback

66b94d6

Stop piecemeal checking individual properties, and rely on the equals…

12476a8

… overload of the configuration object

Move UseRoslynTokenizer to RazorConfiguration

90c0db6

Just specify defaults once

f6e3f1c

Move CSharpLanguageVersion to RazorConfiguration

04375f6

Remove CSharpLanguageVersion from project classes, so it's clear wher…

1e8e1f1

…e it comes from

Rename classes for clarity

0ee391d

davidwengier marked this pull request as ready for review October 28, 2024 03:51

davidwengier requested review from a team as code owners October 28, 2024 03:51

DustinCampbell approved these changes Oct 28, 2024

View reviewed changes

chsienki reviewed Oct 28, 2024

View reviewed changes

PR Feedback

39dccd9

DustinCampbell reviewed Oct 28, 2024

View reviewed changes

DustinCampbell approved these changes Oct 28, 2024

View reviewed changes

Missed one test

efd0d38

davidwengier enabled auto-merge October 28, 2024 22:07

davidwengier merged commit a37ee12 into dotnet:main Oct 28, 2024
12 checks passed

davidwengier deleted the SuppressAddComponentPlumbing branch October 28, 2024 22:34

dotnet-policy-service bot added this to the Next milestone Oct 28, 2024

This was referenced Oct 29, 2024

Bump Razor to 9.0.0-preview.24528.3 dotnet/roslyn#75659

Merged

Move RootNamespace from HostProject to RazorConfiguration #11109

Open

Preprocessor directives don't flow from the IDE to the compiler #11120

Open

phil-allen-msft modified the milestones: Next, 17.13 P1 Oct 31, 2024

jjonescz modified the milestones: 17.13 P1, Next Nov 5, 2024

jjonescz added a commit to jjonescz/razor that referenced this pull request Nov 5, 2024

Revert "Update project configuration from Roslyn info (dotnet#11092)"

0ef2236

This reverts commit a37ee12, reversing changes made to ad74439.

jjonescz added a commit to jjonescz/razor that referenced this pull request Nov 5, 2024

Revert "Update project configuration from Roslyn info (dotnet#11092)"

ac63331

This reverts commit a37ee12, reversing changes made to ad74439.

davidwengier added a commit to davidwengier/razor that referenced this pull request Nov 5, 2024

Revert "Update project configuration from Roslyn info (dotnet#11092)"

c939163

This reverts commit a37ee12, reversing changes made to ad74439.

davidwengier added a commit that referenced this pull request Nov 5, 2024

Revert "Update project configuration from Roslyn info (#11092)" (#11165)

9b2adb4

This reverts commit a37ee12, reversing changes made to ad74439. We'll have to do another dual insertion into VS Code too, I guess

This was referenced Nov 7, 2024

[Automated] PRs inserted in VS build main-35506.54 #11174

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35507.203 #11186

Closed

dotnet-bot mentioned this pull request Nov 19, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35518.109 #11227

Closed

jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update project configuration from Roslyn info #11092

Update project configuration from Roslyn info #11092

davidwengier commented Oct 25, 2024 •

edited

Loading

davidwengier commented Oct 25, 2024

DustinCampbell left a comment

DustinCampbell Oct 25, 2024

davidwengier commented Oct 26, 2024

davidwengier commented Oct 28, 2024 •

edited

Loading

DustinCampbell commented Oct 28, 2024

DustinCampbell left a comment

DustinCampbell Oct 28, 2024

chsienki left a comment

davidwengier commented Oct 28, 2024

DustinCampbell Oct 28, 2024

DustinCampbell Oct 28, 2024

DustinCampbell Oct 28, 2024

Update project configuration from Roslyn info #11092

Update project configuration from Roslyn info #11092

Conversation

davidwengier commented Oct 25, 2024 • edited Loading

davidwengier commented Oct 25, 2024

DustinCampbell left a comment

Choose a reason for hiding this comment

DustinCampbell Oct 25, 2024

Choose a reason for hiding this comment

davidwengier commented Oct 26, 2024

davidwengier commented Oct 28, 2024 • edited Loading

DustinCampbell commented Oct 28, 2024

DustinCampbell left a comment

Choose a reason for hiding this comment

DustinCampbell Oct 28, 2024

Choose a reason for hiding this comment

chsienki left a comment

Choose a reason for hiding this comment

davidwengier commented Oct 28, 2024

DustinCampbell Oct 28, 2024

Choose a reason for hiding this comment

DustinCampbell Oct 28, 2024

Choose a reason for hiding this comment

DustinCampbell Oct 28, 2024

Choose a reason for hiding this comment

davidwengier commented Oct 25, 2024 •

edited

Loading

davidwengier commented Oct 28, 2024 •

edited

Loading