Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
davidwengier opened this issue Oct 29, 2024 · 7 comments
Open

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

davidwengier opened this issue Oct 29, 2024 · 7 comments

Comments

@davidwengier
Copy link
Contributor

When the new Roslyn tokenizer is enabled in the compiler, it also requires a CSharpParseOptions to be provided. Currently the IDE does not serialize this information across the wire, so the language server cannot supply the right one for IDE scenarios, and users get default behaviour.

It would be good if we can focus the compiler dependency down to just what is needed (proprocessor directives is a best guess) and then ensure we are including that information in the RazorConfiguration that the IDE provides.

And if we're changing how UseRoslynTokenizer gets to the compiler, it would be worth reviewing this comment too:
#11092 (comment)

@ryzngard
Copy link
Contributor

❓ is there a reason we don't just add CSharpParseOptions to RazorConfiguration? Is it just serialization problems or something else?

@davidwengier
Copy link
Contributor Author

Yes, just serialization. We need Json and MessagePack serialization for it, which it doesn't natively support, so we'd have to write out own formatters, and that would mean keeping up with new properties added to it, which sounds like a future bug farm when someone pulls on a random property from CSharpParseOptions, not realising we didn't serialize that one, so they get the default value in the IDE, and expected value in the command line.

@chsienki chsienki added this to the 17.13 P2 milestone Oct 31, 2024
@chsienki
Copy link
Contributor

@333fred Please take a look.

@chsienki chsienki added area-compiler Umbrella for all compiler issues and removed untriaged labels Oct 31, 2024
@333fred
Copy link
Member

333fred commented Oct 31, 2024

@davidwengier @chsienki I'm sorry, but I have absolutely no idea what the actual ask here is. What is this issue actually tracking, and what are we expecting to do about it?

@davidwengier
Copy link
Contributor Author

What is this issue actually tracking, and what are we expecting to do about it?

Right now the Razor compiler takes in CSharpParseOptions which is something the LSP server cannot provide, so in the IDE everything will just use the default. From your comments in #11091 (comment) it seemed like only PreprocessorSymbolNames were actually needed, however. So if the compiler can be changed to only take that value, then we can plumb it around everywhere, and IDE will reflect the users real project intentions.

@333fred
Copy link
Member

333fred commented Oct 31, 2024

So if the compiler can be changed to only take that value, then we can plumb it around everywhere, and IDE will reflect the users real project intentions.

This isn't something we can do; we should use the real CSharpParseOptions, either from cohosting where no serializer is required, or from exposing the serializer/deserializer that the Roslyn IDE already uses via EAP. We are passing the entire parse options object to the Roslyn tokenizer, and I do not think the Razor compiler can or should make guarantees about which pieces of that object it needs; it is likely to get it wrong.

@333fred 333fred added untriaged and removed area-compiler Umbrella for all compiler issues labels Oct 31, 2024
@333fred 333fred removed their assignment Oct 31, 2024
@333fred 333fred removed this from the 17.13 P2 milestone Oct 31, 2024
@phil-allen-msft phil-allen-msft added this to the 17.13 Planning milestone Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants