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

Allow LSP and cohosting to provide specialized methods to get a syntax tree #10765

Merged
merged 8 commits into from
Aug 21, 2024

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Aug 20, 2024

This PR is a classic self-nerd-snipe, and resolves this comment: #10750 (comment)

Also inadvertently found a slight "bug" with the existing go to def code in cohosting. Bug is in quotes because the actual user behaviour probably was identical in 99% of cases.

This was a subtle future bug, that we would have hit when we removed the C# syntax tree bits in future. By checking for Razor GTD first we weren't deferring to Roslyn for plain attributes (ie, the case without `@bind-` in the tests) which means at best we do some extra unnecessary work, and at worst we could lose features for things Roslyn supports but we don't.

I did not intend to find this bug when I started. Sorry for missing it in the PR Dustin. The key is that `ignoreAttributes` should have been `true` when porting over the code, and changing that would have made the test fail in your branch.
See comment in the file, but essentially this is the only way for the code that returns the generated documents syntax tree to be hit in cohosting.
In light of "should we get rid of document context" convo this morning, figured this made sense to do (and I needed _something_ on IDocumentSnapshot in order to actually make this whole idea work)
@davidwengier davidwengier requested a review from a team as a code owner August 20, 2024 07:15
@davidwengier davidwengier changed the title Allow LSP and cohosting to provide specialized methods to get a synta… Allow LSP and cohosting to provide specialized methods to get a syntax tree Aug 20, 2024
@davidwengier
Copy link
Contributor Author

Thanks for the feedback @DustinCampbell, I ended up adding a method to get the syntax tree from a document snapshot, as you suggested, and it's much neater now.

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Very nice!

@davidwengier davidwengier merged commit acc84f6 into dotnet:main Aug 21, 2024
12 checks passed
@davidwengier davidwengier deleted the GoToDefImprovements branch August 21, 2024 01:08
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 21, 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