Skip to content

Fixed incorrect handling of client-initiated progress reporting for "…#2546

Merged
erictraut merged 2 commits intomainfrom
erictr/progressReporting
Nov 8, 2021
Merged

Fixed incorrect handling of client-initiated progress reporting for "…#2546
erictraut merged 2 commits intomainfrom
erictr/progressReporting

Conversation

@erictraut
Copy link
Copy Markdown
Collaborator

…onReferences" and "onExecuteCommand" handlers in language server.

…onReferences" and "onExecuteCommand" handlers in language server.
@rchl
Copy link
Copy Markdown
Contributor

rchl commented Nov 6, 2021

Looks as nice as it could to me, given the circumstances. :)

I suppose the library could expose some property (isNull?) on the common WorkDoneProgressReporter interface to make it cleaner. @dbaeumer

@dbaeumer
Copy link
Copy Markdown
Member

dbaeumer commented Nov 8, 2021

// is an actual client-side progress reporter or a dummy (null) progress reporter
// created by the LSP library. If it's the latter, we'll create a server-initiated
// progress reporter.
if (reporter.constructor.name !== 'NullProgressReporter') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure that this will work when we package (here or pylance), as webpack will run our code through terser which strips out names. It'd appear to work when just debugging as that runs unoptimized.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh, that's a good point. Can you think of any other ways for us to do this check? If not, we may need to wait for a change in the LSP library.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The best hack I can come up with is to call attachWorkDone to get a NullProgressReporter, then compare the constructors for identity.

const nullProgressReporter = attachWorkDone(undefined as any, undefined);

// ...
private async _getProgressReporter(reporter: WorkDoneProgressReporter, title: string, token: CancellationToken) {
    if (reporter.constructor !== nullProgressReporter.constructor) { .... }
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That's clever. Very hacky, but clever.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oop, no, that doesn't work.

> class Foo {}
undefined
> Foo.constructor === Foo.constructor
true
> class Bar {}
undefined
> Foo.constructor === Bar.constructor
true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, I'm silly. Those are the class's constructors, which are of course equal because that's object.

> (new Foo()).constructor === (new Foo()).constructor
true
> (new Foo()).constructor === (new Bar()).constructor
false

The hack should work. Sorry for the noise.

@erictraut erictraut merged commit c7a6a71 into main Nov 8, 2021
@erictraut erictraut deleted the erictr/progressReporting branch November 8, 2021 20:36
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.

Incorrect handling of client-initiatied progress

5 participants