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

Improve highlighting of signature parameters when labels are of type string #2072

Merged
merged 9 commits into from
Oct 5, 2022

Conversation

predragnikolic
Copy link
Member

fixes #1694

@predragnikolic
Copy link
Member Author

Here is a code snippet that can be tried with LSP-pylsp

def ExampleFunctionName(Ex, Fun, i, name):
    pass


ExampleFunctionName() 

@@ -115,7 +116,8 @@ def _render_label(self, view: sublime.View, signature: SignatureInformation) ->
# route relies on the client being smart enough to figure where the parameter is inside of
# the signature label. The above case where the label is a tuple of (start, end) positions is much
# more robust.
start = label[prev:].find(rawlabel)
label_match = re.search(r"\b{}\b".format(rawlabel), label[prev:])
start = label_match.start() if label_match else -1
Copy link
Member

Choose a reason for hiding this comment

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

Does it work with

def f(str: str, int: int) -> str:
    return ""

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

What about f(x: str, str: int)

Copy link
Member

Choose a reason for hiding this comment

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

What about f(x: str, str: int)

Then I assume it will do the same as before, i.e. mark the first occurence of "str" it can find. But I guess there is no way to fix that without adding complex and language-dependent code about what should be considered as a parameter and what is a type, etc.

But if you want to find an example where this PR would actually be a regression compared to the status quo, you can take this one:

f(🐰, 🐺) = 🐰 + 🐺

Before (at least the underline is visible for the highlighted parameter):
before

After:
after

This happens because this PR assumes that the parameter names must always begin and end with what Python's regex considers as a "word" character (otherwise the word boundary \b wouldn't match).

But it's probably a very rare example (but not a complete joke example, actually it's adopted with some adjustments from https://www.youtube.com/watch?v=QwVO0Xh2Hbg&t=4765s), which seems to be acceptable in exchange for the improvement from this PR. I also tried with greek letters (which would be much more common than emojis in parameter identifiers), and in that case it works fine because they are considered as word characters.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed the regression for unicode chars by using \W instead of \b

Then I assume it will do the same as before, i.e. mark the first occurrence of "str" it can find.

Yes,
the goal of this PR is to improve the highlighting when the server provides just a string: (not a list with specified precise start and end ranges)

before:
example

after:
image

What about f(x: str, str: int)

The LSP client can't fix all cases with a string provided by the server.
One can argue that a LSP-* plugin can handle that, but I would say that is a responsibility of the server.

If I was ever to create a language server,
I would use label: string for a language that do not have types hints,
where there are really small changes of the same name appearing twice in the label.
If there is a possibility that a same string appear twice, I would switch to [uinteger, uinteger] on the server.

Either way, I would not look this as a client bug, but rather as an improvement on the server.

export interface ParameterInformation {

	/**
	 * The label of this parameter information.
	 *
	 * Either a string or an inclusive start and exclusive end offsets within
	 * its containing signature label. (see SignatureInformation.label). The
	 * offsets are based on a UTF-16 string representation as `Position` and
	 * `Range` does.
	 *
	 * *Note*: a label of type string should be a substring of its containing
	 * signature label. Its intended use case is to highlight the parameter
	 * label part in the `SignatureInformation.label`.
	 */
	label: string | [uinteger, uinteger];

Copy link
Member Author

Choose a reason for hiding this comment

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

Offtopic :D
This is the first time seeing emojis being used in code like this XD f(🐰, 🐺) = 🐰 + 🐺

Julia ls takes forever(maybe around 30s ~ 1min) to initialize on linux on my machine, is that normal?

Copy link
Member

Choose a reason for hiding this comment

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

This is the first time seeing emojis being used in code like this XD f(🐰, 🐺) = 🐰 + 🐺

Yeah, I think nobody would actually use emojis for things like this in real code ;)
It's more that Julia is often used for numerical calculations from a mathematical or physics background, and so the language allows a wide range of unicode characters in identifiers to make possible e.g. the use of greek letters for variables / physical constants or mathematical symbols for special operators.

Julia ls takes forever(maybe around 30s ~ 1min) to initialize on linux on my machine, is that normal?

Hm, 1min seems pretty long, but yes it takes a while for the server to initialize (I can measure ~ 20sec here). That is because the server will be compiled each time when it starts, and also you can observe that using any feature (e.g. hover) will also lag a few seconds when it is used the very first time (again, the particular code parts for that feature will be compiled just-in-time directly before usage). But after that it should be fast. Unfortunately, afaik there is no good way to fully compile Julia code (i.e. the language server) ahead of time yet. But I think it still starts a bit faster than on VSCode :)

@predragnikolic
Copy link
Member Author

I am unsure on how to fix this sre_constants.error: unexpected end of regular expression build error.

@rchl
Copy link
Member

rchl commented Sep 28, 2022

I am unsure on how to fix this sre_constants.error: unexpected end of regular expression build error.

Can't use "random" input in a regular expression as it can contain characters that would have special meaning in regexp. You'd need to escape that input.

plugin/core/signature_help.py Outdated Show resolved Hide resolved
@rchl rchl changed the title Improve higlighting ParameterInformation labels when labels are of type string Improve highlighting of signature parameters when labels are of type string Oct 5, 2022
@rchl rchl merged commit 87e072c into main Oct 5, 2022
@rchl rchl deleted the fix/parameter-information-when-label-is-string branch October 5, 2022 18:32
rchl added a commit that referenced this pull request Oct 11, 2022
* main:
  Cut 1.19.0
  Improve highlighting of signature parameters when labels are of type string (#2072)
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.

Function parameters highlight the wrong text in the tooltip
4 participants