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

Update Roslyn package references to latest available and do minor fixup #11113

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

alexgav
Copy link
Contributor

@alexgav alexgav commented Oct 29, 2024

Summary of the changes

  • Update Roslyn package references to the latest in VS main
  • Minor fixup due to ambiguous extension method Reverse() - there is now one in System.MemoryExtensions, so we need to force type we are invoking the method on to be IEnumerable to get the Linq reverse method we want.
  • Roslyn GetDocumentSymbolAsync method now requires supportsVSExtensions parameter. Hardcoding to true for now until we plumb VSInternalClientCapabilities through init of the remote client (see Pass VSInternalClientCapabilies (or needed parts) in RemoteClientLSPInitializationOptions #11102)

@alexgav alexgav requested review from a team as code owners October 29, 2024 15:16
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 glad that you separated this out from your other PR! This is definitely a separate concern from the completion work.
 
I added a comment that describes the changes needed to correctly pass the VSInternalClientCapabilities.SupportsVisualStudioExtensions from the client. Also, it looks like this change causes test failures. Those will need to be fixed up before I can sign off.

Roslyn was requesting new IRazorMappingService and our TestRazorDocumentServiceProvider was throwing. Returning null for now per suggestion from Andrew.
@alexgav
Copy link
Contributor Author

alexgav commented Oct 29, 2024

Also, it looks like this change causes test failures. Those will need to be fixed up before I can sign off.

All appear to have been due to the new IRazorMappingService that's in Roslyn but not yet in Razor. Returning null rather that throwing for that service request in test code per Andrew.

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.

I'm OK with this if a TODO comment is added in the code to link the issue to the hard-coded bit.

@alexgav
Copy link
Contributor Author

alexgav commented Oct 30, 2024

@dotnet/razor-compiler ptal

Copy link
Member

@jjonescz jjonescz left a comment

Choose a reason for hiding this comment

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

compiler LGTM

@alexgav alexgav merged commit 41693de into main Oct 30, 2024
12 checks passed
@alexgav alexgav deleted the dev/alexgav/UpgradeRoslynPackageReferences branch October 30, 2024 13:08
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Oct 30, 2024
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 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.

5 participants