razor icon indicating copy to clipboard operation
razor copied to clipboard

Update project configuration from Roslyn info

Open davidwengier opened this issue 1 year ago • 2 comments

Fixes https://github.com/dotnet/razor/issues/10736

Sadly this doesn't actually work, though I've confirmed that the SuppressAddComponentParameter flag is correctly flowing through. Going to need more investigation, but putting this up as a head start.

Based on the UseRoslynTokenizer PR so ignore the first three commits :)

davidwengier avatar Oct 25 '24 04:10 davidwengier

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 :)

davidwengier avatar Oct 25 '24 05:10 davidwengier

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.

davidwengier avatar Oct 26 '24 02:10 davidwengier

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.

davidwengier avatar Oct 28 '24 03:10 davidwengier

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 avatar Oct 28 '24 15:10 DustinCampbell

ProjectWorkspaceState is probably something that should just go away

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

davidwengier avatar Oct 28 '24 19:10 davidwengier