Wire up the UseRoslynTokenizer feature properly #11386
Conversation
|
RPS and Speedometer runs in https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/602694 |
|
|
||
| // We use the new tokenizer only when requested for now. | ||
| var useRoslynTokenizer = parseOptions.Features.TryGetValue("use-roslyn-tokenizer", out var useRoslynTokenizerValue) | ||
| && string.Equals(useRoslynTokenizerValue, "true", StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| var razorConfiguration = new RazorConfiguration(razorLanguageVersion, configurationName ?? "default", Extensions: [], UseConsolidatedMvcViews: true, SuppressAddComponentParameter: !isComponentParameterSupported); |
There was a problem hiding this comment.
Why aren't you passing usesRoslynTokenizer to this ctor?
|
|
||
| // We use the new tokenizer only when requested for now. | ||
| var useRoslynTokenizer = parseOptions.Features.TryGetValue("use-roslyn-tokenizer", out var useRoslynTokenizerValue) | ||
| && string.Equals(useRoslynTokenizerValue, "true", StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| var razorConfiguration = new RazorConfiguration(razorLanguageVersion, configurationName ?? "default", Extensions: [], UseConsolidatedMvcViews: true, SuppressAddComponentParameter: !isComponentParameterSupported); | ||
|
|
||
| var razorSourceGenerationOptions = new RazorSourceGenerationOptions() |
There was a problem hiding this comment.
Thiss constructor ends up with an object where options.UseRoslynTokenizer != options.Configuration.UseRoslyntokenizer. Is there a place where we could add validation to catch this dissagreement? Or is it a valid setup and I'm missing it. Could RazorSourceGenerationOptions just have UseRoslynTokenizer be a passthrough to the property on Configuration?
There was a problem hiding this comment.
RazorConfiguration is currently not ideal IMO. It's why I logged #11182. Tooling passes a RazorConfiguration to the compiler when configuring the project engine, but the compiler doesn't use all of the configuration, so we also have to read properties off that configuration again, and do more config in the configure lambda. Ideally we'd just give the compiler some type it needed, and it would go from there.
So yeah, UseRoslynTokenizer could be passed in here, and in fact thats why I moved it, but then I realised it wasn't necessary and forgot to move it back.
...soft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.RazorProviders.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorConfiguration.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs
Outdated
Show resolved
Hide resolved
|
Perf DDRITs and Speedometer are all happy 🎉🎉🎉 (Nobody breathe on the PR) |
Fixes #11120
This wires up UseRoslynTokenizer properly, so we read from the project not from our feature flag, in time for it being on by default for .NET 10. It also serializes pre-processor symbols so they will work with it. It is not serializing the whole C# parse options because we need to do work in Roslyn to share that code properly.