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

[plugin] Fixed wrong type conversion #6351

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Conversation

svenefftinge
Copy link
Contributor

What it does

The type (un)marshalling was wrongly using
language server types instead of vscode.d.ts /
theia.d.ts types.

How to test

It revealed in the gitlens extensions, which uses document symbols from other language services in order to position code lenses. The resulting ranges and positions were send back to our document implementation which failed because of an unsatisfied instanceof Positioncheck.

I.e. install the gitlens extensions (together with native VS Code git) and see an error in the console and no code lenses on the symbols (it produces a fall back code lens on top of the file, though).

Review checklist

Reminder for reviewers

@svenefftinge
Copy link
Contributor Author

I do not think an implicit conversion is the right thing to do, though. But this at least fixes the current state. In a cleaner approach we should make the protocol work with shapes solely actually using and implementing the protocol from here https://github.com/microsoft/vscode/blob/master/src/vs/workbench/api/common/extHost.protocol.ts#L369

See also #6353

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Screen Shot 2019-10-09 at 17 27 32

this works as advertised 👍

@@ -435,7 +436,9 @@ namespace ObjectsTransferrer {
// tslint:disable-next-line:no-any
const obj: any = JSON.parse(value.data);
// May require to use types-impl there instead of vscode lang server Range for the revival
return Range.create(Position.create(obj.start.line, obj.start.character), Position.create(obj.end.line, obj.end.character));
const start = new Position(obj.start.line, obj.start.character);
Copy link
Contributor

Choose a reason for hiding this comment

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

☝️ this was already foreseen, kind of

Copy link
Contributor

Choose a reason for hiding this comment

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

see one line above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, git it. I'll remove the comment

The type (un)marshalling was wrongly using
language server types instead of vscode.d.ts /
theia.d.ts types.

Signed-off-by: Sven Efftinge <[email protected]>
@svenefftinge svenefftinge force-pushed the se/plugin_type_conversion branch from a782bde to 108ec6a Compare October 9, 2019 16:18
@benoitf
Copy link
Contributor

benoitf commented Oct 9, 2019

cc @tsmaeder @tolusha @svor

@svenefftinge svenefftinge merged commit 54f646c into master Oct 10, 2019
@svenefftinge svenefftinge deleted the se/plugin_type_conversion branch October 10, 2019 11:23
@tsmaeder tsmaeder self-requested a review October 10, 2019 11:48
@tsmaeder
Copy link
Contributor

In a cleaner approach we should make the protocol work with shapes solely

Actually, defining the parameters using interface types opens us up to people passing things that don't serialize properly (the "Position" class form types-impl.ts, for example.
IMO, there are two (mutually exclusive) clean ways to handle the problem:

  1. Use only classes in the main/ext interface and make sure those serialize properly
  2. Open up the object serializer for extension and have each main/ext interface contribute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants