-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Improve sharing of trees when changing project configuration #65974
Conversation
[InlineData("// File", true, LanguageNames.CSharp)] | ||
[InlineData("#if DEBUG", false, LanguageNames.VisualBasic)] | ||
[InlineData(@"#region ""goo""", true, LanguageNames.VisualBasic)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woot. now documents with other pp directives can be reused when changing project configs. In roslyn this is ~4000 docs.
if (onlyPreprocessorDirectiveChange && | ||
_treeSource.TryGetValue(out var existingTreeAndVersion)) | ||
{ | ||
var existingTree = existingTreeAndVersion.Tree; | ||
|
||
SyntaxTree? newTree = null; | ||
|
||
if (existingTree.TryGetRoot(out var existingRoot) && !existingRoot.ContainsDirectives) | ||
var syntaxKinds = _languageServices.GetRequiredService<ISyntaxKindsService>(); | ||
if (existingTree.TryGetRoot(out var existingRoot) && !existingRoot.ContainsDirective(syntaxKinds.IfDirectiveTrivia)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utilizes new compiler API we added for this exact purpose. .ContainsDirectives
is too broad, firing even for things that do not affect parsing. .ContainsDirective(SyntaxKind.IfDirectiveTrivia)
means we only fire on trees hat could genuinely parse differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any broken code scenarios we need to worry about, like there's not an #if but there's an #elif that still results in different trees? I sure don't care about optimizing this case; this is entirely a "I hope we don't crash" question.
if (onlyPreprocessorDirectiveChange && | ||
_treeSource.TryGetValue(out var existingTreeAndVersion)) | ||
{ | ||
var existingTree = existingTreeAndVersion.Tree; | ||
|
||
SyntaxTree? newTree = null; | ||
|
||
if (existingTree.TryGetRoot(out var existingRoot) && !existingRoot.ContainsDirectives) | ||
var syntaxKinds = _languageServices.GetRequiredService<ISyntaxKindsService>(); | ||
if (existingTree.TryGetRoot(out var existingRoot) && !existingRoot.ContainsDirective(syntaxKinds.IfDirectiveTrivia)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any broken code scenarios we need to worry about, like there's not an #if but there's an #elif that still results in different trees? I sure don't care about optimizing this case; this is entirely a "I hope we don't crash" question.
Tested at compiler layer already. Note: |
No description provided.