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

Cohost inlay hint support #10672

Merged
merged 15 commits into from
Jul 31, 2024
Merged

Conversation

davidwengier
Copy link
Contributor

@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.

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
Contributor 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 merged commit c78f5f2 into dotnet:main Jul 31, 2024
12 checks passed
@davidwengier davidwengier deleted the CohostInlayHints branch July 31, 2024 03:38
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 31, 2024
@dibarbet dibarbet modified the milestones: Next, 17.12 P2 Aug 26, 2024
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