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.

Sign up for GitHub

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

Jump to bottom

Add usings on paste #48501

Merged
merged 37 commits into from
Nov 20, 2020
Merged

Add usings on paste #48501

merged 37 commits into from
Nov 20, 2020

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Oct 11, 2020 •
edited
Loading

Addresses #45853

Hook up paste command to also automatically add usings. Operation uses thread await dialog to pop up to a user if it is taking a while with a "cancel" button. The dialog will not always show, but follows the IUIThreadOperationContext scope rules

Edit:

This is disabled by default. The following followup issues were filed
#49442
#49443
#49444

Example GIF

note that the string has been changed

usings_on_paste_with_dialog

Undo Behavior

Currently undo works to undo added usings, and then added pasted code (two undos)

Failure Behavior

If the operation fails, the code is still pasted but no usings are added. No dialog or exception is currently provided to the user.

@ryzngard
Hook up paste command to also automatically add usings. Operation uses
2568592 
thread await dialog to pop up to a user if it is taking a while with a
"cancel" button
@Youssef1313
Copy link
Member

If there are more than one namespace containing the same class name, will it add some random using? or it will ignore it?

@ryzngard
Copy link
Contributor Author

If there are more than one namespace containing the same class name, will it add some random using? or it will ignore it?

Right now if there are conflicting usings it doesn't add any. This follows the design of the lightbulb command that does the same thing. IMO We should pursue fixes in this area separately as we find them, but I'm open to discussion on it.

@Youssef1313
Copy link
Member

Right now if there are conflicting usings it doesn't add any.

This is the behavior I'm expecting ❤️

@ryzngard
Copy link
Contributor Author

Looking into Integration Test failures, but I think this behavior represents final design.

@ryzngard ryzngard marked this pull request as ready for review October 12, 2020 23:24
@ryzngard ryzngard requested a review from a team as a code owner October 12, 2020 23:24
@ryzngard ryzngard requested a review from JoeRobich October 12, 2020 23:25
@ryzngard ryzngard added this to the 16.9.P1 milestone Oct 12, 2020
sharwell
sharwell reviewed Oct 12, 2020
View reviewed changes
src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpAddMissingUsingsOnPaste.cs Outdated
using (var telemetry = VisualStudio.EnableTestTelemetryChannel())
{
VisualStudio.Editor.Paste(@"
Task DoThingAsync() => Task.CompletedTask;");
Copy link
Member

Choose a reason for hiding this comment

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

I would not expect any using directives to be added in this case, because the code was not originally copied from source code inside Visual Studio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was discussed as a design, but ultimately from #45853 we indicated that timeboxing was more appropriate. If the code is originally copied from Visual Studio we could speed up the process by knowing the usings without having to calculate. That work is not covered in this PR, and out of scope for #45853

@CyrusNajmabadi
Copy link
Member

Design questions:

  1. why not have paste just do a regular text paste, but also then pop-up a lightbulb with the option of adding usings?
  2. if this is going to happen automatically, i think we need an opt out switch. semantics can be very expensive. And having paste now incur them is pretty worrying for me.
  3. a cleaner way of handling this might be to use hte richer info from copy/paste to drive things. i.e. when we copy, we can know if the source was vs and we can bind just what is copied in the source location. We can see what usings that code needs and we can pull in those usings if not in scope.

--

Major concerns: copy/pastes can be large. i.e. full multi-thousand line methods/classes. What is our perf on somehting like that?

@CyrusNajmabadi
Copy link
Member

I'm going to hold off on reviewing until we discuss the design/implications here :)

@ryzngard
Copy link
Contributor Author

why not have paste just do a regular text paste, but also then pop-up a lightbulb with the option of adding usings?

We already have that. This request was made specifically to automatically do it.

if this is going to happen automatically, i think we need an opt out switch. semantics can be very expensive. And having paste now incur them is pretty worrying for me.

Would you consider this blocking this checkin, or blocking the feature complete story as a whole? I'd like to file a follow up item for a disable checkbox.

a cleaner way of handling this might be to use hte richer info from copy/paste to drive things. i.e. when we copy, we can know if the source was vs and we can bind just what is copied in the source location. We can see what usings that code needs and we can pull in those usings if not in scope.

I agree this would help in cases where we are copying from VS, but we want to cover scopes where we are not. I'm happy to do a follow up item to improve performance for operations only done in VS

Major concerns: copy/pastes can be large. i.e. full multi-thousand line methods/classes. What is our perf on somehting like that?

Likely slow, but I also think that's okay. Cancel should work immediately and allow the user to unblock if they wish.

ryzngard and others added 3 commits October 14, 2020 17:53
@ryzngard
Update xlf files
3873ece
@ryzngard @JoeRobich
Update src/Features/Core/Portable/CodeRefactorings/AddMissingImports/…
27d5d86 
…AddMissingImportsAnalysisResult.cs

Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
@ryzngard
String updates
97ac747
CyrusNajmabadi
CyrusNajmabadi reviewed Nov 19, 2020
View reviewed changes
src/EditorFeatures/Core/Implementation/AddImports/AbstractAddImportsPasteCommandHandler.cs

#pragma warning disable VSTHRD102 // Implement internal logic asynchronously
var updatedDocument = _threadingContext.JoinableTaskFactory.Run(() => addMissingImportsService.AddMissingImportsAsync(document, textSpan, cancellationToken));
#pragma warning restore VSTHRD102 // Implement internal logic asynchronously
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 what we do elsewhere? I'm not super familiar with .OperationContext. but if this is our regular pattern then 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OperationContext just requires that we have a disposable scope, which will block the UI interaction. Since command handlers are synchronous, this is the only way to take the async work and do it all in a synchronous fashion. If we go full async, we no longer will need an OperationContext or the disposable scope.

CyrusNajmabadi
CyrusNajmabadi reviewed Nov 19, 2020
View reviewed changes
.../Core/Portable/CodeRefactorings/AddMissingImports/AbstractAddMissingImportsFeatureService.cs
}

public async Task<Project> AddMissingImportsAsync(Document document, TextSpan textSpan, CancellationToken cancellationToken)
/// <inheritdoc/>
public async Task<AddMissingImportsAnalysisResult> AnalyzeAsync(Document document, TextSpan textSpan, CancellationToken cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

AddMissingImportsAnalysisResult is an internal type right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

CyrusNajmabadi
CyrusNajmabadi approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Signing off on the conditional the final review feedback is addressed.

Awesome work! This will be very nice to get in. Hopefully we can also get some good perf numbers and then enable by default :)

@ghost
Copy link

ghost commented Nov 20, 2020

Hello @ryzngard!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost
ghost approved these changes Nov 20, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ryzngard ryzngard merged commit 8d278c2 into dotnet:master Nov 20, 2020
@ghost ghost modified the milestones: 16.9.P2, Next Nov 20, 2020
@ryzngard ryzngard deleted the feature/usings_on_paste branch November 20, 2020 08:21
This was referenced Nov 20, 2020
Closed
Closed
Closed
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 20, 2020
@333fred
Merge remote-tracking branch 'upstream/master' into merge-master
1e9e240 
* upstream/master: (47 commits)
  Make compiler server logging explicit (dotnet#48557)
  [master] Update dependencies from dotnet/arcade (dotnet#48274)
  Handle removed project in ExternalErrorDiagnosticUpdateSource
  Report an error for ref-returning auto-properties declared in an interface. (dotnet#49510)
  Add usings on paste (dotnet#48501)
  Determinism test needs to use bootstrap compiler (dotnet#49483)
  Simplify and reduce amount of work performed by SourcePropertySymbolBase constructor. (dotnet#49360)
  Updates test
  Simplify
  Split Document/Workspace handler into only handling open/closed documents respectively.
  only report watson once.
  Additional usage of a PooledHashset. (dotnet#49459)
  Loc checkin
  Update src/Features/CSharp/Portable/CodeRefactorings/ConvertLocalFunctionToMethod/CSharpConvertLocalFunctionToMethodCodeRefactoringProvider.cs
  Preserve annotation on comment trivia when performing formatting.
  Validate arguments to IAsyncCompletionSource methods
  Determinism fixes for AnonymousTypes in VB (dotnet#49467)
  Collect nfw information for a crash we're seeing.
  Make sure to not discard text changes when no reference changes are present
  Create an unsafe method from a local function when necessary (dotnet#49389)
  ...
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

@JoeRobich JoeRobich JoeRobich approved these changes

@davidwengier davidwengier davidwengier approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@sharwell sharwell Awaiting requested review from sharwell

Assignees
No one assigned
Labels
Area-IDE auto-merge
Projects
None yet
Milestone
16.9.P2
Development

Successfully merging this pull request may close these issues.
9 participants
@ryzngard @Youssef1313 @CyrusNajmabadi @jasonmalinowski @JoeRobich @davidwengier @sharwell @vatsalyaagrawal @allisonchou