Wire up UseRoslynTokenizer in Visual Studio #11091
Conversation
| @@ -209,6 +211,7 @@ RazorProjectEngine CreateProjectEngine() | |||
| builder.SetRootNamespace(HostProject.RootNamespace); | |||
| builder.SetCSharpLanguageVersion(CSharpLanguageVersion); | |||
| builder.SetSupportLocalizedComponentNames(); | |||
| builder.Features.Add(new ConfigureRazorParserOptions(useRoslynTokenizer: ProjectWorkspaceState.UseRoslynTokenizer, CSharpParseOptions.Default)); | |||
There was a problem hiding this comment.
The CSharpParseOptions.Default is worth calling out here: In the LSP server we do not have access to the Compilation, so everything we need from it has to be serialized across the wire from Roslyn. Currently that list is the Tag Helpers, C# lang version, and now the UseRoslynTokenizer flag.
@333fred what is the impact of using the default parse options in the IDE going to be, and does it matter much?
Serializing the entire CSharpParseOptions is not impossible, but it's a bit annoying keeping up with it. Cohosting has direct access so maybe if the impact is low we don't worry?
There was a problem hiding this comment.
@333fred what is the impact of using the default parse options in the IDE going to be, and does it matter much?
Constants defined in the project file won't line up correctly. That's somewhat important.
There was a problem hiding this comment.
Does that mean we can just send csharpParseOptions.PreprocessorSymbolNames across the wire and it will be sufficient? Can we also change the Razor compiler API so it just takes that list rather than the whole parse options object?
There was a problem hiding this comment.
Alternatively, since they didn't work well previously, can we assume that people aren't desperate for changes around #if in Razor files, and the string and binary literal features are sufficient for dogfooding in 17.13 P1?
There was a problem hiding this comment.
I think the preprocessor symbols should be sufficient. Not doing for p1 is probably fine, but I do think we want to get them for 17.13 GA.
There was a problem hiding this comment.
I guess I left most of my feedback in #11092. I didn't realize there were two PRs with mostly the same code. In general, I'm uncomfortable with adding this ProjectWorkspaceState. IMO, we should be working to move state intended to configure the Razor compiler into RazorConfiguration. Right now, configuring the compiler is kind of a mess, and moving state into ProjectWorkspaceState doesn't help address that.
|
Since most of your comments were on the other PR, I replied there. I didn't think you looked at draft PRs 😛 |
|
Closing is favour of #11092 |
No, I will absolutely look at a draft PR and offer feedback if I think it's useful. For example, If I see that a draft PR is going in a particular direction that could use some discussion, I'll definitely leave feedback. Also, if there's a PR comment that indicates that feedback might be useful to person who submitted the draft PR, I'll take a look. Otherwise, I won't look at a draft PR to avoid wasting my time and the submitter's time. What I really find annoying, is when somebody "approves" a draft PR. That is an egregious bit of engineering laziness IMO. 😛 😜 😝 |
Bring's @333fred's compiler work to bear in VS by adding a new field to
ProjectWorkspaceStateto represent the flag, and serialize it across the wire etc.Makes this work:
