Move the semantics checks of Inheritance margin to OOP#61592
Move the semantics checks of Inheritance margin to OOP#61592Cosifne merged 7 commits intodotnet:mainfrom
Conversation
| internal interface IRemoteInheritanceMarginService | ||
| { | ||
| ValueTask<ImmutableArray<InheritanceMarginItem>> GetGlobalImportItemsAsync( | ||
| ValueTask<ImmutableArray<InheritanceMarginItem>> GetInheritanceMarginItemsAsync( |
There was a problem hiding this comment.
This is the core part of the change.
For both features, just send a document and text span to the OOP, and do all the semantics work there.
So we only need one method in the remote interface.
src/Features/Core/Portable/InheritanceMargin/AbstractInheritanceMarginService.cs
Outdated
Show resolved
Hide resolved
genlu
left a comment
There was a problem hiding this comment.
I love this change. One fewer feature that needs symbols in proc!
src/Features/Core/Portable/InheritanceMargin/AbstractInheritanceMarginService_Helpers.cs
Outdated
Show resolved
Hide resolved
| Document document, | ||
| TextSpan spanToSearch, | ||
| bool includeGlobalImports, | ||
| bool frozenPartialSemantics, |
There was a problem hiding this comment.
Why adding a parameter for frozen partial? I don't see this is set to false in the code. Does inheritance-margin need to operate on full semantic?
There was a problem hiding this comment.
This parameter is introduced in #61286
As Cyrus and Sam mentioned it is a strange pattern to indicate we are using the frozen partial semantics both in devenv and OOP.
Does inheritance-margin need to operate on full semantic?
See here https://sourceroslyn.io/#Microsoft.VisualStudio.LanguageServices/InheritanceMargin/InheritanceMarginTaggerProvider.cs,59
When the full semantic is ready the tagger would refresh all the tag again to make sure we get the latest semantics.
And we use the frozen partial one to make sure user could see it the glyph as early as possible
There was a problem hiding this comment.
I was wondering if the feature needs to operate explicitly on full-semantic. My understanding is, when full semantic is available, using frozen partial would do nothing special. The callsite in the tagger also just set frozenPartialSemantics to true, which was what prompted my question :)
There was a problem hiding this comment.
I was wondering if the feature needs to operate explicitly on full-semantic
No, we generate the InheritanceMarginItems whenever a partial semantic is ready.
And later when the full semantic is ready InheritanceMarginItems are generated again.
|
so one thing the old system did was remap from a MAS workpsace to the normal workspaec (presumbly so inheritnace margin works in MAS files). does that continue to work? |
Are you asking this? https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/InheritanceMargin/AbstractInheritanceMarginService.cs,96 which is if a metadata symbol could be found in workspace, we should try to navigate to the 'in source' symbol? Emm, I manually test it and there are some scenarios don't work. I will open another PR to fix that. |
|
Do your tests exercise the OOP paths as well as the non-oop paths? Importantly, are tehy testing the MetadataAsSource scenario? |
I have changed all the tests to cover both in-process and OOP. For the MetadataAsSource regression, as we talked offline, it is a problem probably related to DocumentIdAndSpan, not related to this PR. |
This PR is a cleanup PR and a potential solution for https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1528673/
Currently, the inheritance margin service would first get the declared symbols for the given document in devenv, then send all the symbols to OOP, then call FAR services and get the result back.
And as Cyrus mentioned in the issue, somehow this 'GetDeclarationSymbol' call is using a lot of memory. (maybe it hits some cache miss issue I guess)
Also, this is a little bit different from the 'Global imports' features (which is just sending a document and text span to the OOP), so it introduces some similar code.
This PR moves the 'GetDeclaredSymbol' part also to OOP and solves both problems.