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 onWorkspaceSymbol, onSignatureHelp, onDocumentSymbol, onReferences, improve onDefinition #191

Conversation

triwav
Copy link
Contributor

@triwav triwav commented Sep 29, 2020

This PR covers moving onSignatureHelp, onWorkspaceSymbol, onDocumentSymbol support in. It also adds a much larger support for onDefinition as well.

…orRange helpers, fix getFullyQualifiedClassName misspelling,
…ceSymbol_and_onDocumentSymbol_and_improve_onDefinition

# Conflicts:
#	src/interfaces.ts
#	src/util.ts
…ceSymbol_and_onDocumentSymbol_and_improve_onDefinition

# Conflicts:
#	src/files/BrsFile.ts
#	src/parser/ClassStatement.ts
…moving, fix warnings for sourcemaps by excluding typescript and vscode-languageserver node modules
…ceSymbol_and_onDocumentSymbol_and_improve_onDefinition

# Conflicts:
#	src/LanguageServer.ts
#	src/Scope.ts
#	src/validators/ClassValidator.ts
@georgejecook
Copy link
Contributor

@triwav I pinged you on slack; but a cursory read, and my trial with your vsix gives me the impression that brighterscript is not supported at all by this.

I've got code in maestro's binding plugin that looks up methods. I proprose in the short term, seeing as we don't have type support, and to save you a ton of work, that we merely dump all classes, and class members into the symbols repo, maybe in getDocumentSymbol.. I think @elsassph and @TwitchBronBron would have the best idea around this though, as they've got the current up-to-date knowledge on where the cached references are kept, for max performance.

As it stands, this totally regresses bs support.; hopefully it will not be too much work to add brighterscript namespace and class support; fwiw - Bron makes me support brs projects whenever I do bs changes, too :)

@georgejecook
Copy link
Contributor

georgejecook commented Oct 15, 2020

one odd thing.. I noticed that callfunc @. completions method signature popups were also not working. that vexed me somewhat, as we get those for free from document symbols, which you currently support: not sure whats happening there.

@TwitchBronBron
Copy link
Member

@georgejecook, you say " gives me the impression that brighterscript is not supported at all by this.". What specifically makes you think that? Do you have any reproducible steps to show that classes/methods/fields, etc, are not showing up in some situation? I know that I worked with @triwav specifically to get brighterscript support in this effort, so while it's possible we missed something, it would be helpful to know what exactly is missing and how to reproduce it.

@georgejecook
Copy link
Contributor

Here's the current lsp:
current-lsp
Here's the vsix from this branch:
lsp-change

Note the missing method sig popups. Ill get symbol gifs, presently.

@georgejecook
Copy link
Contributor

go to symbol, find symbol, from vsix from this branch: note - no symbols; does nothing.
lsp-change-symbols-defs
current extension - does a resonable job of finding defs/symbols, even for the call func case:
lsp-current-symbols-defs

@georgejecook
Copy link
Contributor

I only had a cursory look at the pr, and didn't see anything in the code that seemed to look up class functions, in the way I've been doing over the last 2 weeks; but, like I say; I only skimmed it; so apologies for wrongly assuming this isn't accounted for. Clearly a bug then.

…ceSymbol_and_onDocumentSymbol_and_improve_onDefinition

# Conflicts:
#	.vscode/launch.json
#	src/Program.ts
#	src/files/BrsFile.ts
#	src/interfaces.ts
…tion and onSignatureHelp didn't work for class methods
…ceSymbol_and_onDocumentSymbol_and_improve_onDefinition
@triwav triwav marked this pull request as ready for review November 3, 2020 18:52
src/files/BrsFile.ts Outdated Show resolved Hide resolved
Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Hey. I have requested a few changes. I haven't finished reviewing, but figured I should get these to you sooner rather than later.

src/LanguageServer.spec.ts Show resolved Hide resolved
src/LanguageServer.spec.ts Show resolved Hide resolved
src/LanguageServer.spec.ts Outdated Show resolved Hide resolved
src/LanguageServer.spec.ts Show resolved Hide resolved
src/LanguageServer.ts Show resolved Hide resolved
src/LanguageServer.ts Show resolved Hide resolved
src/LanguageServer.ts Outdated Show resolved Hide resolved
src/LanguageServer.ts Outdated Show resolved Hide resolved
src/LanguageServer.ts Show resolved Hide resolved
src/LanguageServer.ts Outdated Show resolved Hide resolved
src/Program.ts Outdated Show resolved Hide resolved
//get the token at the position
const token = this.getTokenAt(position);

let definitionTokenTypes = [
Copy link
Member

Choose a reason for hiding this comment

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

Certain keywords are allowed to be used as local variable names, so we should probably include those in this list.

Also, I'm not overly familiar with the full reach of this function, but some keywords are allowed to be object property names, so you might want to include these as well:

src/util.ts Outdated Show resolved Hide resolved
@TwitchBronBron
Copy link
Member

Looks great. Thanks for addressing all of the concerns.

@TwitchBronBron TwitchBronBron merged commit 615ca9c into master Nov 11, 2020
@TwitchBronBron TwitchBronBron deleted the implement_onWorkspaceSymbol_and_onDocumentSymbol_and_improve_onDefinition branch November 11, 2020 03:38
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.

None yet

4 participants