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

Implement handling of rename intent #59410

Merged
merged 6 commits into from
Apr 7, 2022
Merged

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Feb 9, 2022

The next refactoring intellicode is interested in

/// </summary>
[Obsolete("Use DocumentChanges instead")]
Copy link
Member Author

Choose a reason for hiding this comment

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

rename requires us to support multiple document changes. instead of outright removing this, obsoleting until intellicode can react to avoid dual insertion

/// <summary>
/// The text changes that should be applied to each document.
/// </summary>
public readonly ImmutableDictionary<Uri, ImmutableArray<TextChange>> DocumentChanges;
Copy link
Member Author

@dibarbet dibarbet Feb 9, 2022

Choose a reason for hiding this comment

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

@pgroene you'll need to switch to using this to get the text edits to apply. Should be fairly self explanatory, contains the text changes that should be applied to each URI.

Copy link
Member

Choose a reason for hiding this comment

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

why URIs instead of DocumentIds?

{
}

public async Task<ImmutableArray<IntentProcessorResult>> ComputeIntentAsync(Document priorDocument, TextSpan priorSelection, Document currentDocument, string? serializedIntentData, CancellationToken cancellationToken)
Copy link
Member Author

Choose a reason for hiding this comment

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

@pgroene to invoke rename, you'll pass the name plus a json like { "newName": "<new name>" }. The prior selection can just be the caret position of any of instances of the symbol.

@@ -146,7 +147,18 @@ public static Uri GetUriFromFilePath(string filePath)
if (filePath is null)
throw new ArgumentNullException(nameof(filePath));

return new Uri(filePath, UriKind.Absolute);
if (Path.IsPathRooted(filePath))
Copy link
Member Author

Choose a reason for hiding this comment

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

this was mainly for tests. test documents are created as non-rooted paths (e.g. "test1.cs") so we can't use absolute paths. If its non-rooted we just create a relative URI instead.

Copy link
Member

Choose a reason for hiding this comment

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

why are we using URIs :D

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi - you mean in general for exposing the doc changes to intellicode? technically we could use roslyn documents I suppose

@pgroene do you have a preference?

Copy link
Member Author

Choose a reason for hiding this comment

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

confirmed offline intellicode can use documentid. will update this to match

Copy link
Member

Choose a reason for hiding this comment

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

can we revert this change then?

@dibarbet dibarbet marked this pull request as ready for review February 9, 2022 19:04
@dibarbet dibarbet requested a review from a team as a code owner February 9, 2022 19:04
@dibarbet
Copy link
Member Author

@CyrusNajmabadi if you have a chance could use a review on this for intellicode experimentation

public async Task<ImmutableArray<IntentProcessorResult>> ComputeIntentAsync(Document priorDocument, TextSpan priorSelection, Document currentDocument, IntentDataProvider intentDataProvider, CancellationToken cancellationToken)
{
var renameIntentData = intentDataProvider.GetIntentData<RenameIntentData>();
Contract.ThrowIfNull(renameIntentData);
Copy link
Member

Choose a reason for hiding this comment

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

is this like a json deserialization thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, and the string they pass us is nullable in the API so we have to account for it

return ImmutableArray<IntentProcessorResult>.Empty;
}

Contract.ThrowIfNull(renameIntentData);
Copy link
Member

Choose a reason for hiding this comment

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

akready checked above.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️

RenameFile: false);

var renameLocationSet = await renameInfo.FindRenameLocationsAsync(options, cancellationToken).ConfigureAwait(false);
var renameReplacementInfo = await renameLocationSet.GetReplacementsAsync(renameIntentData.NewName, options, cancellationToken).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

it's a pity this has to be computed up front. It seems like we want a 'compute + resolve' approach where we determine if the rename is valid, then prose asks the user "do you want to rename?" then it calls back into us for the expensive bit.

renames (esp. on large solutions take a lot of time) so i'm worried this will basically make the feature not work unless you have small solutions.

Copy link
Member Author

@dibarbet dibarbet Apr 7, 2022

Choose a reason for hiding this comment

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

discussed offline - for now this is just for testing to make sure we can reliably detect this and give something valuable. For actual release we will need to consider potentially different UX that allows us to potentially lazily resolve the actual edits, maybe with something like a background work indicator.

Determining if rename can be done is cheap, but actually calculating the edits is really expensive , like 20s expensive on something like roslyn since its basically a FAR operation. This is applicable in general to all our code actions as they can be expensive to compute.

There are some optimizations here we could consider - like for example providing the rename locations only in the current document upfront to intellicode (less expensive) and then only doing the full rename on commit of the action.

This is something we'll need to design and discuss (cc @pgroene)

@dibarbet dibarbet enabled auto-merge (squash) April 7, 2022 01:38
@dibarbet dibarbet merged commit e79be97 into dotnet:main Apr 7, 2022
@ghost ghost added this to the Next milestone Apr 7, 2022
@dibarbet dibarbet deleted the rename_intent branch April 7, 2022 17:34
333fred added a commit that referenced this pull request Apr 11, 2022
* Simplify

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Excluded Task.CompletionTask from completion list in async member declarations

* Fix merge

* Use existing assert

* Use correct assert

* Use isInferred: false when creating BoundArrayInitialization in CreateUTF8ByteRepresentation

* Move few 'CodeFixProvider's to Analyzers layer

* Fix

* Fix

* Remove ForegroundThreadAffinitizedObject usage

* fix

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Remove ForegroundThreadAffinitizedObject usage

* Revert bad changes

* Fix errors and remove a hard CodeFixProvider back to Features

* Update projitems

* Move tests

* Remove unused abstract property

* Remove the single unused read of CodeFixCategory

* Remove overrides in Analyzers

* Remove overrides in Features

* Fixed char recomendations test

* Got rid of AsyncModifierName constants

* Remove unnecessary `<Compile Remove`

* Fixed tests for all other keywords

* Cleaned up keyword tests

* Move formatting analyzer to shared Analyzers layer

* Move tests to shared Analyzers layer and more other fixes

* Few more fixes

* Fix bad changes

* Fix bad changes

* Fix bad changes

* Remove IVTs of deleted test projects

* Address review comment

* Fix failing test

* Cleanup unused resources in Features layer

* Update Roslyn.Diagnostics.Analyzers and remove RS0005 suppressions

* Move to SyntaxEditorBasedCodeFixProvider

* Remove some 'MyCodeAction's

* Remove more 'MyCodeAction's

* One more that's not named MyCodeAction

* Fix up changes between main and main-vs-deps

* Colorize async as keyword in some cases

* Few fixes

* Added a bunch of tests, where async can never be a keyword, moved logic one level up

* Fix typo

* Note auto-default merged in feature status doc (#60564)

* Update PublishData.json for 17.3 P1 (#60559)

* Fix formatting

* Added case with equals syntax, moved logic to a separate method

* Add Rebuild badge to README (#60298)

* Avoid using --blame-crash with CollectDumps

This flag causes the test harness to attach ProcDump, which removes our
ability to collect heap dumps on crash using Windows' built-in
functionality.

* Ensure current OOP calls for the same solution-checksum can share the same OOP solution computation

* Update src/Workspaces/Remote/ServiceHub/Services/DocumentHighlights/RemoteDocumentHighlightsService.cs

* Update src/Workspaces/Remote/ServiceHub/Services/SemanticClassification/RemoteSemanticClassificationService.cs

* Revert name

* Update tests

* Fix

* Add comment

* Add comment

* Update src/Workspaces/Remote/ServiceHub/Services/CodeLensReferences/RemoteCodeLensReferencesService.cs

* fix grammar

* Formatting

* Revert

* revert

* Revert

* Fix the Compiler VSIX extension so it works with CPS projects again

This has been broken for quite some time. We were MEF importing
a type that was no longer a MEF part, and referencing very old
reference assemblies that needed to be updated as a part of responding
to that original breaking change.

* Make NotificationOption2 a serializable struct (#60573)

* Update status for UTF8 literals and checked operators (#60587)

* Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800

* Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800

* Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800

* Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1700800

* Simplify

* Simplify

* Simplify

* Simplify

* [LSP] Modify semanticTokens `isFinalized` logic (#60484)

* try/finally

* Simplify

* Move comment

* Simplify

* Doc

* Doc

* Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033

* Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033

* Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033

* Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 1701033

* Workspace config options (#59790)

* REnames

* Expand tuple

* Rename parameter

* Use deconstruction

* Fix merge from main

* Do not assert we are on the main thread in the constructor

* Add support for unsafe local functions in IDE0062 code fix (#59297)

* Track "numeric IntPtr" feature (#60579)

* Address review comment

* Add helpers

* Remove unneeded IVTs

* Simplify

* Simplify

* Pause computation of navbar items when a buffer is not visible

* Rename

* PR feedback part 1

* Change insertion branch

* Update PublishData.json

* PR feedback part 2

* lint

* Implement handling of rename intent (#59410)

* Implement handling of rename intent

* Use documentId instead of URI

* Move record definition up top

* feedback

* Address feedback

* Remove unused files in CodeStyle layer

* Remove unused resources in CodeStyle layer

* Changed algorithm of detecting IAsyncEnumerable and IAsyncEnumerator, reverted some refactoring

* Got rid of IsSystemRuntimeOrMscorlibAssembly method

* Fix crash when combining reordered arguments with params array (#60622)

* Fix string

* Always use text edits in completion for correctness (#60466)

* Filter down to task-like types when in an async context

* Update providers

* Cleanup

* Revert

* Revert

* Revert

* Simplify

* Restore logic

* Update test

* Pull feature-specific APIs out of ISyntaxFacts

* Extract out IBlockFacts out of ISyntaxFacts

* Simplify

* Instrument ReferenceCachingCS for flakiness (#60585)

* Fix our INavigableSymbol to not root an entire solution-snapshot

* Remove the TEmporaryWorkspace entirely from the RemoteWorkspace code.

* Lint

* Fixed adding a redundant blank line when suppressing with local attribute

* [main] Update dependencies from dotnet/source-build-externals (#60359)

[main] Update dependencies from dotnet/source-build-externals

* Fix

* Added more tests & refactoring

* Clean up syntax context

* Clean up syntax context

* Clean up syntax context

* Clean up syntax context

* Simplify

* Delay starting the work to scan for todo-items

* move properties

* Cleanup

* Cleanup

* Update tests

* Fixup

* Run continuation to dispose of cancellation token source (#60653)

* Run continuation to dispose of cancellation token source

* Remove cancellation token and execute synchronously

* Update src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

* Global indentation options - take 2 (#60565)

* Revert "Revert "Global indentation options (#59679)" (#60199)"

This reverts commit ab57ce8.

* Move IFormattingInteractionService to Editor Features and add inferred indentation detection to its implementation.
Move GetFormattingChangesOnTypedCharacterAsync, GetFormattingChangesOnPasteAsync to ISyntaxFormattingService - these do not depend on the editor.

* Test fix

* Update struct field definite assignment tests

* Restore CodeStyle test projects

* Lint

* Fallout

* Pass options to FixAllAsync, simplify CodeAction registration (#60665)

* Trim unnessasary leading lines when removing usings (#60672)

* Formatting and code generation options (#60127)

* Remove duplicate package references (#60658)

* Remove duplicate package references

* Delete additional Tools.props

* Update for definite assignment changes

Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
Co-authored-by: DoctorKrolic <mapmyp03@gmail.com>
Co-authored-by: David Barbet <dabarbet@microsoft.com>
Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
Co-authored-by: Youssef1313 <youssefvictor00@gmail.com>
Co-authored-by: DoctorKrolic <70431552+DoctorKrolic@users.noreply.github.com>
Co-authored-by: Manish Vasani <mavasani@microsoft.com>
Co-authored-by: Jason Malinowski <jason.malinowski@microsoft.com>
Co-authored-by: Rikki Gibson <rigibson@microsoft.com>
Co-authored-by: Allison Chou <allichou@microsoft.com>
Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com>
Co-authored-by: Tomáš Matoušek <tmat@users.noreply.github.com>
Co-authored-by: msftbot[bot] <48340428+msftbot[bot]@users.noreply.github.com>
Co-authored-by: Julien Couvreur <jcouv@users.noreply.github.com>
Co-authored-by: Gérald Barré <meziantou@users.noreply.github.com>
Co-authored-by: Jonathon Marolf <jmarolf@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>
Co-authored-by: Fredric Silberberg <frsilb@microsoft.com>
@dibarbet dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants