Snap for 17.12 P2 #10793
Merged
Snap for 17.12 P2 #10793
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was done because originally with inlay hints I was changing global options in Roslyn, but ended up moving away from that. Still makes sense though.
…da are but kinda aren't supported in Razor
Part of dotnet#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.
This is an automatically generated pull request from release/dev17.11 into main. Once all conflicts are resolved and all the tests pass, you are free to merge the pull request. 🐯 ## Troubleshooting conflicts ### Identify authors of changes which introduced merge conflicts Scroll to the bottom, then for each file containing conflicts copy its path into the following searches: - https://github.com/dotnet/razor/find/release/dev17.11 - https://github.com/dotnet/razor/find/main Usually the most recent change to a file between the two branches is considered to have introduced the conflicts, but sometimes it will be necessary to look for the conflicting lines and check the blame in each branch. Generally the author whose change introduced the conflicts should pull down this PR, fix the conflicts locally, then push up a commit resolving the conflicts. ### Resolve merge conflicts using your local repo Sometimes merge conflicts may be present on GitHub but merging locally will work without conflicts. This is due to differences between the merge algorithm used in local git versus the one used by GitHub. ``` bash git fetch --all git checkout -t upstream/merges/release/dev17.11-to-main git reset --hard upstream/main git merge upstream/release/dev17.11 # Fix merge conflicts git commit git push upstream merges/release/dev17.11-to-main --force ```
…rojectCollectionResolver
…rojectCollectionResolver (dotnet#10706) Started this for dotnet#10688 but saw that Go To Def used the same service, so figured @DustinCampbell would need to do the same thing, and thought it would be easier to put it up separately. (Unless of course you've already done this, or similar, Dustin in which case I can just abandon this, nbd)
This change updates `DocumentContext` is several ways: 1. Fields used for caching are now assigned with `Interlocked.Initialize(...)` to ensure that the same value are used in race scenarios. 2. All public async methods now return `ValueTask` rather than `Task`. This has lower overhead since the async methods will return synchronously after the first call. 3. Members are no longer virtual. This appears to have been originally done to allow `DocumentContext` to be mocked for tests, which isn't the right way to handle testing. Instead, since `DocumentContext` delegates its implementation to its `IDocumentSnapshot`, the snapshot should be mocked to change the implementation. 4. The Identifier property has been converted to a method, since it creates a new object every time.
In order to remove `IDocumentContextFactory` from `AbstractRazorDocumentMappingService`, it's necessary to split `IRazorDocumentMappingService` into different services. The goal is to make `IRazorDocumentMappingService` provide APIs that only perform mapping using Roslyn primitives, such as `LinePosition`, `LinePositionSpan`, and `TextChange`. `IEditMappingService` contains an API moved from `IRazorDocumentMappingService` that remaps a `WorkspaceEdit`.
This allows `EditMappingService` and `AbstractRazorDocumentMappingService` to share a helper method.
This change removes `IDocumentContextFactory` from the `AbstracRazorDocumentMappingService` and replaces the single usage with a new protected abstract method.
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)
…x tree (dotnet#10765) This PR is a classic self-nerd-snipe, and resolves this comment: dotnet#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.
* Moving RazorFormattingService to common layer * Cleanup * Switch Razor LSP server code to use RazorFormattingService from common layer * Removing HTML formatting from common layer (for now) It requires services not easily available and will require extra work. For now leaving it in LSP server layer. Current need for the formatting service in cohosting is for OnAutoInsert which doesn't need HTML formatting. * Move AdhocServices, AdhoWorkspaceServices to common layer * Cleanup moved resources * Basic implementations for remote formatting classes * Removing hardcoded option value * Cleanup per CR suggestions * Cleanup doc-comments
# Conflicts: # eng/targets/Services.props # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageServer.cs # src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Remote/RazorServices.cs
Fixes dotnet#10689 Roslyn side: dotnet/roslyn#74730 Looks good, despite the Roslyn side having VS deps for images: 
This is an automatically generated pull request from release/dev17.12 into main. Once all conflicts are resolved and all the tests pass, you are free to merge the pull request. 🐯 ## Troubleshooting conflicts ### Identify authors of changes which introduced merge conflicts Scroll to the bottom, then for each file containing conflicts copy its path into the following searches: - https://github.com/dotnet/razor/find/release/dev17.12 - https://github.com/dotnet/razor/find/main Usually the most recent change to a file between the two branches is considered to have introduced the conflicts, but sometimes it will be necessary to look for the conflicting lines and check the blame in each branch. Generally the author whose change introduced the conflicts should pull down this PR, fix the conflicts locally, then push up a commit resolving the conflicts. ### Resolve merge conflicts using your local repo Sometimes merge conflicts may be present on GitHub but merging locally will work without conflicts. This is due to differences between the merge algorithm used in local git versus the one used by GitHub. ``` bash git fetch --all git checkout -t upstream/merges/release/dev17.12-to-main git reset --hard upstream/main git merge upstream/release/dev17.12 # Fix merge conflicts git commit git push upstream merges/release/dev17.12-to-main --force ```
Because of a dare from @333fred, I'm currently on a crusade to remove `ItemCollection` from the Razor Compiler completely. Previously, I removed `ItemCollection` from `TagHelperDescriptorProviderContext` (dotnet#10720). This time, I'm removing it from `CodeRenderingContext`. It turns out that every `CodeRenderingContext` greedily creates an `ItemCollection`, though there are only ever two things stored there: 1. A string to use for new lines in `CodeWriter`. 2. A string representing "suppress IDs", which is already specified in `RazorCodeGenerationOptions`. These two items were really present as a hook that compiler tests could set. However, it's much easier and less wasteful to elevate both items to `RazorCodeGenerationOptions` and make tests set the options directly. I made a few other refactorings as part of this change: - I merged several abstract base classes with their single default concrete type: - `CodeRenderingContext` and `DefaultCodeRenderingContext` - `RazorCSharpDocument` and `DefaultRazorCSharpDocument`, - `RazorCodeGenerationOptions` and `DefaultRazorCodeGenerationOptions` - `RazorCodeGenerationOptionsBuilder` and `DefaultRazorCodeGenerationOptionsBuilder`. - Reworked `RazorCodeGenerationOptions` and introduced `RazorCodeGenerationOptionsFlags` to track its boolean fields. - Cleaned up `RazorCSharpDocument` and unified its collections on `ImmutableArray<>` rather than `IReadOnlyList<>`. - Enabled nullability annotations for several types. - Used more pooled collections in `CodeRenderingContext`. Note: I was careful with my commit history, and it should be clean enough to review commit-by-commit if that's your preference.
* When @Inject is missing the member name, generate a syntactically valid c# identifier so we get intellisense * Emit an empty section when there is no typename * Add tests and update baselines
Since the end of last week, these tests have been failing. Seems like something changed on the platform side, perhaps a default value of a setting. Have started a thread with the editor team to see if we need to do more.
Cosifne
approved these changes
Aug 26, 2024
This was referenced Aug 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
merge main into release/dev17.12 to update for 17.12 P2. Main is 17.12 P3.