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

fix textDocument/documentSymbol #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JJPro
Copy link

@JJPro JJPro commented Apr 10, 2020

I made symbol lookup/document outline working.
Previously it would list all words in the document and the server would crash for big documents,
Now it only looks for function definitions (define) and type definitions (define-type)

(also resolves #12 )

@jeapostrophe
Copy link
Owner

I don't think this is a good change, because it is brittle in the face of macros that perform binding. The existing approach is to give all symbols and allow the lsp to basically be like emacs' M-/ where it just helps you complete symbols that already there. A more robust change would be to do what DrRacket does and look at the expansion for binding positions.

@Eugleo
Copy link

Eugleo commented Apr 29, 2020

Do we have any way to find out which expressions are binding, without resorting to some sort of string pattern match?

@jeapostrophe
Copy link
Owner

DrRacket has a library implement for this, as part of Check Syntax, that fully-expands the program and then inspects the source code for binding information in the syntax objects.

- lexer, the procedure
loop until it is a procedure
|#
(if (procedure? lexer)
Copy link

Choose a reason for hiding this comment

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

This I think is problematic, as for empty files it seems to cycle indefinitely. It's getting too late here, but I'll push a fix tomorrow. We should also properly detect the defining, as @jeapostrophe suggested.

Copy link

@Eugleo Eugleo May 1, 2020

Choose a reason for hiding this comment

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

As per module-lexer docs:

When mode is #f (indicating the start of the stream), the lexer checks in for a #lang specification.

If a #lang line is present but the specified language does not exist, the entire in input is consumed and colored as 'error.

If the language exists and the language provides a get-info function, then it is called with 'color-lexer. If the result is not #f, then it should be a lexer function for use with color:text<%>. The result mode is the lexer—paired with #f if the lexer is a procedure arity 3—so that future calls will dispatch to the language-supplied lexer.

If the language is specified but it provides no get-info or 'color-lexer result, then racket-lexer is returned as the mode.

When the file is empty, I'd say module-lexer will just return #f, among other things — calling it again and again won't change a thing.

Copy link

Choose a reason for hiding this comment

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

Question for @jeapostrophe: the default behaviour seems to be to error out if a lexer procedure isn't returned. I'm not familiar with your codebase nor with LSP too much, but it doesn't seem right to give errors even in non-erroneous state (= plain empty file).

Copy link
Owner

Choose a reason for hiding this comment

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

An empty file is definitely an error in Racket, because a #lang or (module ...) is required.

Copy link

Choose a reason for hiding this comment

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

That makes sense, thanks.

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.

Error get-lexer: 'before-lang-line
3 participants