[plugin-ext] Fix WorkspaceEdits for rename providers #6304

AlexTugarev · 2019-10-01T14:27:23Z

What it does

WorkspaceEdits created in languages-main.ts should include monaco.Uris instead of require("vscode-uri").URIs. "Rename Symbol" is broken due to this, and this PR fixes it.

Fixes #6295

How to test

try to use vscode extension for python
run "Rename Symbol" and see it working; without this change you would see Error applying workspace edits: Error: Unexpected edit type: {}

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

JPinkney

I've just tried with the python vscode extension with a file that contained:

a = 500
print(a)
for i in range(0, 100):
    print('hello world ', i)

Attempting to use rename symbol on a and i yields:

root ERROR Rename failed to execute.
Uncaught (in promise) TypeError: Cannot read property 'edits' of undefined
    at toMonacoWorkspaceEdit (languages-main.ts:801)

With continuously calling rename I also got:

root ERROR Rename failed to execute.
Uncaught (in promise) TypeError: Cannot read property 'edits' of undefined
    at toMonacoWorkspaceEdit (languages-main.ts:801)

JPinkney · 2019-10-01T15:20:37Z

packages/plugin-ext/src/main/browser/languages-main.ts

-        for (const edit of data.edits) {
-            if (typeof (<ResourceTextEditDto>edit).resource === 'object') {
-                (<ResourceTextEditDto>edit).resource = URI.revive((<ResourceTextEditDto>edit).resource);
+export function toMonacoWorkspaceEdit(data: WorkspaceEditDto): monaco.languages.WorkspaceEdit {


Can we guarantee that data and data.edits are not undefined? It looks like the previous code had guards to check for it

thanks for testing @JPinkney and @benoitf!
indeed this was not the case with the happy path.

given this shape:

export interface WorkspaceEditDto { edits: (ResourceFileEditDto | ResourceTextEditDto)[]; rejectReason?: string; }

I adjusted the result of provideRenameEdits in case of an error to not return undefined! for edits.

actually, I don't understand why the adapter isn't rejecting in this case but returning with something to be handled down the road. well, I couldn't find any use of rejectReason either. what's the concept behind this?

Can we guarantee that data and data.edits are not undefined?

@JPinkney as long as we're are creating these objects, the answer is "yes".

@JPinkney I had to update again. Please retest.

benoitf · 2019-10-01T15:31:46Z

I got

logger-protocol.ts:112 root ERROR Rename failed to execute.
log @ logger-protocol.ts:112
(anonymous) @ logger-frontend-module.ts:41
(anonymous) @ logger.ts:312
(anonymous) @ logger.ts:304
Promise.then (async)
../../packages/core/lib/common/logger.js.Logger.log @ logger.ts:299
log @ logger.ts:45
SimpleNotificationService.notify @ simpleServices.ts:211
SimpleNotificationService.error @ simpleServices.ts:205
(anonymous) @ rename.ts:231
Promise.then (async)
(anonymous) @ rename.ts:208
Promise.then (async)
(anonymous) @ rename.ts:195
step @ arrays.ts:4
(anonymous) @ arrays.ts:4
fulfilled @ arrays.ts:4
Promise.then (async)
step @ arrays.ts:4
(anonymous) @ arrays.ts:4
__awaiter @ arrays.ts:4
RenameController.doRename @ rename.ts:151
(anonymous) @ rename.ts:146
createCancelablePromise @ async.ts:23
(anonymous) @ rename.ts:146
step @ arrays.ts:4
(anonymous) @ arrays.ts:4
(anonymous) @ arrays.ts:4
__awaiter @ arrays.ts:4
RenameController.run @ rename.ts:138
RenameAction.run @ rename.ts:306
EditorAction.runEditorCommand @ editorExtensions.ts:222
(anonymous) @ codeEditorWidget.ts:319
InstantiationService.invokeFunction @ instantiationService.ts:67
(anonymous) @ codeEditorWidget.ts:318
InternalEditorAction.run @ editorAction.ts:44
(anonymous) @ monaco-editor.js:457
step @ monaco-editor.js:83
(anonymous) @ monaco-editor.js:64
(anonymous) @ monaco-editor.js:58
push.../../packages/monaco/lib/browser/monaco-editor.js.__awaiter @ monaco-editor.js:54
push.../../packages/monaco/lib/browser/monaco-editor.js.MonacoEditor.runAction @ monaco-editor.js:450
execute @ monaco-command.ts:289
push.../../packages/monaco/lib/browser/monaco-command-registry.js.MonacoCommandRegistry.execute @ monaco-command-registry.ts:72
execute @ monaco-command-registry.ts:61
(anonymous) @ command.ts:276
step @ command.ts:15
(anonymous) @ command.ts:15
fulfilled @ command.ts:15
Promise.then (async)
step @ command.ts:15
(anonymous) @ command.ts:15
../../packages/core/lib/common/command.js.__awaiter @ command.ts:15
../../packages/core/lib/common/command.js.CommandRegistry.executeCommand @ command.ts:272
execute @ browser-menu-plugin.ts:112
../../node_modules/@phosphor/commands/lib/index.js.CommandRegistry.execute @ index.js:351
../../node_modules/@phosphor/widgets/lib/menu.js.Menu.triggerActiveItem @ menu.js:305
../../node_modules/@phosphor/widgets/lib/menu.js.Menu._evtMouseUp @ menu.js:641
../../node_modules/@phosphor/widgets/lib/menu.js.Menu.handleEvent @ menu.js:451
languages-main.ts:801 Uncaught (in promise) TypeError: Cannot read property 'map' of undefined
    at toMonacoWorkspaceEdit (languages-main.ts:801)

with https://www.pythonforbeginners.com/code-snippets-source-code/magic-8-ball-written-in-python/

Fixes #6295 Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>

AlexTugarev · 2019-10-02T08:38:10Z

packages/plugin-ext/src/plugin/languages/rename.ts

@@ -57,7 +57,7 @@ export class RenameAdapter {
            if (rejectReason) {
                return <WorkspaceEditDto>{
                    rejectReason,
-                    edits: undefined!
+                    edits: []


this shouldn't break with the expected handling of rejectReason, which BTW is commented with a todo to be a proper rejection in the origin of this.

JPinkney · 2019-10-02T13:24:51Z

I was able to get the rename refactoring to work on:

for i in range(0, 500):
    print('hello world', i)

it takes a bit of time (which I assume is just the plugin taking its time)

but when it completes I get this error in the console:

errors.ts:149 Uncaught (in promise) Canceled: Canceled
    at Object.canceled (https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:2635:21)
    at https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:8032:31
    at Emitter.fire (https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:6473:38)
    at MutableToken.cancel (https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:7887:35)
    at CancellationTokenSource.cancel (https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:7947:29)
    at class_1.cancel (https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:8046:24)
    at RenameController.onModelChanged (https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:114729:46)
    at https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:114596:96
    at Emitter.fire (https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:6473:38)
    at https://3000-fd994a22-1709-48da-94c6-b0d3b0b94642.ws-us1.gitpod.io/vs/editor/editor.main.js:104551:51

AlexTugarev · 2019-10-02T13:43:05Z

it takes a bit of time (which I assume is just the plugin taking its time)

that's usually the case for workspace edits.

but when it completes I get this error in the console:

@JPinkney please help me to reproduce it. I tried with your example and others after a clean build, and with both the jedi engine and the new LS. could you share a workspace snapshot?

svenefftinge

Looks good code-wise.

JPinkney

@AlexTugarev I can no longer reproduce that error on my side (I must have been doing something wrong). Code looks good to me!

AlexTugarev · 2019-10-02T15:01:28Z

@JPinkney, @benoitf, @svenefftinge thanks for checking! 🙏

AlexTugarev requested review from akosyakov, svenefftinge, akurinnoy and JPinkney October 1, 2019 14:27

AlexTugarev force-pushed the GH-6295 branch from 9914cba to 2219c65 Compare October 1, 2019 14:29

JPinkney requested changes Oct 1, 2019

View reviewed changes

akosyakov added the plug-in system issues related to the plug-in system label Oct 2, 2019

AlexTugarev force-pushed the GH-6295 branch from 2219c65 to 4beeedc Compare October 2, 2019 07:43

[plugin-ext] fix WorkspaceEdit conversion

25a5e40

Fixes #6295 Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>

AlexTugarev force-pushed the GH-6295 branch from 4beeedc to 25a5e40 Compare October 2, 2019 08:29

akosyakov requested a review from JPinkney October 2, 2019 08:37

AlexTugarev commented Oct 2, 2019

View reviewed changes

svenefftinge mentioned this pull request Oct 2, 2019

Fixes are not working anymore gitpod-io/gitpod#826

Closed

svenefftinge approved these changes Oct 2, 2019

View reviewed changes

JPinkney approved these changes Oct 2, 2019

View reviewed changes

AlexTugarev merged commit 8f03fc0 into master Oct 2, 2019

akosyakov deleted the GH-6295 branch October 2, 2019 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[plugin-ext] Fix WorkspaceEdits for rename providers #6304

[plugin-ext] Fix WorkspaceEdits for rename providers #6304

AlexTugarev commented Oct 1, 2019

JPinkney left a comment

JPinkney Oct 1, 2019

AlexTugarev Oct 2, 2019

AlexTugarev Oct 2, 2019

AlexTugarev Oct 2, 2019

benoitf commented Oct 1, 2019

AlexTugarev Oct 2, 2019

JPinkney commented Oct 2, 2019

AlexTugarev commented Oct 2, 2019

svenefftinge left a comment

JPinkney left a comment

AlexTugarev commented Oct 2, 2019

[plugin-ext] Fix WorkspaceEdits for rename providers #6304

[plugin-ext] Fix WorkspaceEdits for rename providers #6304

Conversation

AlexTugarev commented Oct 1, 2019

What it does

How to test

Review checklist

Reminder for reviewers

JPinkney left a comment

Choose a reason for hiding this comment

JPinkney Oct 1, 2019

Choose a reason for hiding this comment

AlexTugarev Oct 2, 2019

Choose a reason for hiding this comment

AlexTugarev Oct 2, 2019

Choose a reason for hiding this comment

AlexTugarev Oct 2, 2019

Choose a reason for hiding this comment

benoitf commented Oct 1, 2019

AlexTugarev Oct 2, 2019

Choose a reason for hiding this comment

JPinkney commented Oct 2, 2019

AlexTugarev commented Oct 2, 2019

svenefftinge left a comment

Choose a reason for hiding this comment

JPinkney left a comment

Choose a reason for hiding this comment

AlexTugarev commented Oct 2, 2019