Skip to content

Cohost inlay hint support#10672

Merged
davidwengier merged 15 commits intodotnet:mainfrom
davidwengier:CohostInlayHints
Jul 31, 2024
Merged

Cohost inlay hint support#10672
davidwengier merged 15 commits intodotnet:mainfrom
davidwengier:CohostInlayHints

Conversation

@davidwengier
Copy link
Member

@davidwengier davidwengier commented Jul 25, 2024

Part of #9519
Needs dotnet/roslyn#74548 before it will compile
Needs https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/567229 before it will work in VS

There were a few side quests on this one:

  • Roslyn OOP, at least the way we access it, doesn't have any options set, so took a few tries to get the Roslyn side of this right for our needs
  • I wrote this feature test-first so only discovered the lack of options after I modified Roslyn and our test infra to allow us to set global options, so ended up removing most of that code, Kept the bit about isolated workspaces because it just makes sense.
  • Had to re-write one of the DocumentMappingService methods to use TextChange instead of TextEdit so I could use Roslyn LSP types
  • The hacky, and fortunately temporary, way we were doing generated C# content was causing cache misses in Roslyn in tests, so fixed that up
  • When I finally got up to manual testing I found a bug in the platform that meant inlay hints just don't work with dynamic registration, so filed the above linked PR to fix that

If reviewing commit-at-a-time please note that the first commit was written before the reworking of extension methods and LSP types, and the fixes for that are in the last commit.

@davidwengier davidwengier requested a review from a team as a code owner July 25, 2024 12:07
Comment on lines +78 to +83
// We know this C# maps to Razor, but does it map to Razor that we like?
var node = syntaxTree.Root.FindInnermostNode(hostDocumentIndex);
if (node?.FirstAncestorOrSelf<MarkupTagHelperAttributeValueSyntax>() is not null)
{
continue;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly, this bit of code is pretty much the only logic that could be shared with the existing inlay hints code, because everything else operates on VS LSP types, and here we're using Roslyn LSP types. The code duplication isn't great, but I'm somewhat okay with it because we have good tests for both.

Perhaps we can re-review if/when we can move the language server to Roslyn LSP types, which could be as early as preview 2.

@davidwengier davidwengier requested a review from a team as a code owner July 30, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants