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

REPL: Don't search for ?( completions when hinting #56278

Merged

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Oct 21, 2024

Fixes #56272

@IanButterworth IanButterworth added backport 1.11 Change should be backported to release-1.11 completions Tab and autocompletion in the repl labels Oct 21, 2024
@@ -1228,7 +1228,8 @@ function completions(string::String, pos::Int, context_module::Module=Main, shif
# ?(x, y)TAB lists methods you can call with these objects
# ?(x, y TAB lists methods that take these objects as the first two arguments
# MyModule.?(x, y)TAB restricts the search to names in MyModule
rexm = match(r"(\w+\.|)\?\((.*)$", partial)
# but ignore ?( inside strings
rexm = match(r"^(?!\")(?:\w+\.|)\?\((.*)$", partial)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly this is only ignoring a line starting with a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think this needs fixing. How does the below proposal look?

(\w+\.|)\?\((.*)$ previous
^(?!\")(?:\w+\.|)\?\((.*)$ current
^\s*(?!\")(?:\w+\.|)\?\(\s*(.*)\s*$ proposed here

julia> regex = r"^\s*(?!\")(?:\w+\.|)\?\(\s*(.*)\s*$"
r"^\s*(?!\")(?:\w+\.|)\?\(\s*(.*)\s*$"

julia> match(regex, "?(")
RegexMatch("?(", 1="")

julia> match(regex, "\"?(\"")

julia> match(regex, "\"\"\"?(\"\"\"")

julia> match(regex, "\"?(\"")

julia> match(regex, " \"?(\"")

julia> match(regex, " \" ?(\"")

julia> match(regex, " ?(\"")
RegexMatch(" ?(\"", 1="\"")

julia> match(regex, " ?( ")
RegexMatch(" ?( ", 1="")

julia> match(regex, " ?(")
RegexMatch(" ?(", 1="")

ChatGPT summary:

Let's break down the regular expression ^\s*(?!\")(?:\w+\.|)\?\(\s*(.*)\s*$ step by step:

1. ^:

  • This asserts the start of the string. The pattern must begin matching from the start.

2. \s*:

  • \s matches any whitespace character (spaces, tabs, etc.).
  • * means "zero or more" occurrences of the preceding element, so this part matches zero or more whitespace characters at the start of the string.

3. (?!"):

  • This is a negative lookahead. It ensures that the next character is not a double quote (").
  • It checks if the string does not begin with a " without consuming any characters. If the string starts with a double quote, the match will fail.

4. (?:\w+.|):

  • (?: ... ): This is a non-capturing group, meaning it groups part of the pattern but does not capture it for backreferences.
  • \w+: This matches one or more word characters (letters, digits, or underscores).
  • \.: This matches a literal dot (.).
  • |: This is the "OR" operator, meaning the non-capturing group matches either \w+. (a word followed by a dot) or nothing (i.e., an empty string).

This means the string may optionally have a prefix like word. before the ?(.

5. ?(:

  • This matches the literal sequence ?(. It looks for the exact string ?(.

6. \s*:

  • This matches zero or more whitespace characters after the ?(. It allows for spaces between ?( and the content that follows.

7. (.*):

  • This is a capturing group that matches any characters (except newlines) 0 or more times.
  • The content matched by this group is captured, and it can be referenced or used later.
  • This captures the content inside the parentheses following ?(, including an empty string.

8. \s*:

  • This matches zero or more whitespace characters at the end of the string, allowing for trailing spaces after the content.

9. $:

  • This asserts the end of the string. The pattern must match up to the end.

Summary

The regular expression ^\s*(?!\")(?:\w+\.|)\?\(\s*(.*)\s*$ matches strings that:

  • Can start with optional whitespace.
  • Do not start with a double quote (").
  • Optionally have a word (letters, digits, or underscores) followed by a dot (e.g., word.).
  • Contain the literal sequence ?(, possibly surrounded by spaces.
  • Capture everything after the ?(, including an empty string.
  • Can have optional whitespace after the captured content.
  • Must match from the beginning (^) to the end ($) of the string.

Examples that would match:

  1. " ?(" (matches and captures empty content).
  2. " ?( some content ) " (matches and captures "some content").
  3. " example.?(content)" (matches and captures "content").
  4. " ?( " (matches and captures empty content with spaces).

Copy link
Member Author

Choose a reason for hiding this comment

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

@Seelengrab just saw your comment in the issue, if you can help resolve this here

Copy link
Member

Choose a reason for hiding this comment

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

I mean, you can only determine "inside string" with a complete parser, I believe. I don't think we really need to fiddle with the regex with the !hint restriction that's now here.

I had been thinking that any additional restrictions to the regex would be complementary, but both the current and proposed would limit suggestions in cases that currently work, like f(?(1,2)tab.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. I'll revert the regex change and get the painful part fixed with the !hint

@mbauman
Copy link
Member

mbauman commented Oct 21, 2024

Is there another way we can avoid these kinds of completions for the end-of-line suggestions? This seems helpful, but not quite complete.

@KristofferC
Copy link
Member

Is there another way we can avoid these kinds of completions for the end-of-line suggestions

+1, just disable for inline completions.

@IanButterworth IanButterworth changed the title REPL: ignore ?( inside strings for completions REPL: ignore ?( inside strings for completions. Disable during hinting. Oct 21, 2024
Copy link
Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

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

Easiest to review ignoring whitespace: https://github.com/JuliaLang/julia/pull/56278/files?w=1. This looks good to me, thanks @IanButterworth!

@IanButterworth IanButterworth added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Oct 21, 2024
@IanButterworth IanButterworth changed the title REPL: ignore ?( inside strings for completions. Disable during hinting. REPL: Don't search for ?( completions when hinting Oct 21, 2024
@IanButterworth IanButterworth added the merge me PR is reviewed. Merge when all tests are passing label Oct 21, 2024
@IanButterworth IanButterworth merged commit 049d92a into JuliaLang:master Oct 22, 2024
10 checks passed
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Oct 23, 2024
@PallHaraldsson PallHaraldsson mentioned this pull request Oct 24, 2024
43 tasks
@IanButterworth IanButterworth deleted the ib/repl_method_no_string branch October 24, 2024 12:57
IanButterworth added a commit that referenced this pull request Oct 24, 2024
@IanButterworth IanButterworth removed the backport 1.11 Change should be backported to release-1.11 label Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completions Tab and autocompletion in the repl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The string ?( bogs down REPL input to a crawl
4 participants