Merge main to tokenizer branch #10724
Merged
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
…otnet#10525) Analogous to !10464
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.10 into release/dev17.11. 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.10 - https://github.com/dotnet/razor/find/release/dev17.11 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.10-to-release/dev17.11 git reset --hard upstream/release/dev17.11 git merge upstream/release/dev17.10 # Fix merge conflicts git commit git push upstream merges/release/dev17.10-to-release/dev17.11 --force ```
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.
…ck to the host document (dotnet#10712) There's a fair amount of refactoring here, but I was careful to separate everything. So, I recommend reviewing commit-by-commit.
…ator, plus some cleanup
Every single `TagHelperDescriptorProviderContext` created in Razor compiler or tooling code adds a valid compilation. So, adding the compilation to the `ItemsCollection` is pure overhead and no `ITagHelperDescriptorProvider` needs to check it for null. I removed a single test that verified the behavior if a compilation was never set. Now that a compilation is required, this test is no longer needed.
This can be safely removed now that there are no more references.
- Enable nullability - Mark all as sealed - Use `Lazy<T>` for singleton `ITagHelperDescriptorProviders` - Introduce `TagHelperDescriptorProviderBase` - Inherit from `TagHelperDescriptorProviderBase` - Remove unnecessary properties - Remove unnecessary property setters In addition, I spotted a bug in `EventHandlerTagDescriptorProvider` while addressing nullability warnings. The buggy code is right here: https://github.com/dotnet/razor/blob/fb84ae5d9bb8132972c2c23cf209721161f81deb/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/EventHandlerTagHelperDescriptorProvider.cs#L70-L76 When reading the constructor arguments of an `EventHandlerAttribute` it appears that we swap the last two arguments of the 4-argument variant. The constructor is defined like so: ```C# EventHandlerAttribute(string attributeName, Type eventArgsType, bool enableStopPropagation, bool enablePreventDefault); ``` Unfortunately, the compiler reads the third parameter as `enablePreventDefaeult` and the fourth parameter as `enableStopPropagation`. This has been broken since support for the 4-argument constructor variant was added in dotnet@7635bba. Fixing this bug is tracked by dotnet#10497.
…ommit message, and I didn't like any of them.
…et#10719) because it's really easy to get one from the other. While working on rename in cohosting, I noticed this unnecessary extra bit of work while calling `GetPositionInfo`, so I fixed it. Then I thought I'd see where else `GetSourceTextAsync` was used in an non-ideal way, and that was clearly going to be too big for the rename PR so I separated it out into it's own PR. Then I heard Dustin say he was changing `DocumentSnapshot` so the unnecessary work will end up being much less wasteful anyway, plus we'd probably just have conflicts, so I pared the whole thing right back to just this minor removal of a couple of unnecessary parameters. Enjoy.
* Move to central package pinning This should make it much easier for us to respond to CG alerts in the future. All that will need to be done is add an entry in Directory.Packages.props and it will automatically impact all consumers of it. Consider this example in Roslyn for how to respond to a CG issue dotnet/roslyn#74653
…#10720) Every `TagHelperDescriptorProviderContext` creates an `ItemCollection` to hold onto, at most, two objects: a `Compilation` and a target `ISymbol`. This is enormously wasteful because an `ItemCollection` internally creates a `ConcurrentDictionary<object, object>`. So, I've changed `TagHelperDescriptorProviderContext` to just hold onto a compilation and a target symbol and avoid the `ItemCollection` altogether. I recommend reviewing commit-by-commit. Also, I bumped into a long standing compiler bug in `EventHandlerTagHelperDescriptorProvider` that was recently filed by a customer: dotnet#10497. I opted to stay on target and not fix this issue, but I made it painfully obvious where the fix would go. 😄
…enizer * upstream/main: (53 commits) Move to central package pinning (dotnet#10716) Try to fix rename tests Unskip rename tests I spent 10 minutes looking up cool Mr Freeze catch phrases for this commit message, and I didn't like any of them. Clean up CompilationTagHelperResolver Clean up all ITagHelperDescriptorProviders a bit (and found a bug!) Make ExcludeHidden and IncludeDocumentation init-only properties Swap TagHelperDescriptorProviderContext.Create methods for constructors Remove TagHelperDescriptorProviderContext.Items property Make TargetSymbol a TagHelperDescriptorProviderContext property Make Compilation a TagHelperDescriptorProviderContext property Merge TagHelperDescriptorProviderContext and DefaultContext Don't pass code document and source text around in diagnostics translator, plus some cleanup Remove unnecessary parameter, because it can be trivially retrieved Find razor document correctly in RemoveDocumentMappingService Add extension methods that convert URIs to Roslyn file paths Use Uri.LocalPath rather than GetAbsoluteOrUNCPath() Move MapToHostDocuementUriAndRangeAsync to extension methods Fix small typo in comment Remove unused DocumentState method ...
jjonescz
approved these changes
Aug 12, 2024
chsienki
approved these changes
Aug 12, 2024
This was referenced Nov 4, 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.
Clean merge, no manual changes required