Skip to content

[REPLCompletions] use better version of listing module imports for completions #58218

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

Merged
merged 1 commit into from
Apr 28, 2025

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 24, 2025

The test for this crashes on my machine, leading me to notice that the implementation of this function was nonsense. Fortunately, we already had a more correct implementation of this function in the REPL docview code that we could borrow from to fix this.

While here, also filter out macros, since those are rather strange looking to see appearing in the results. We could alternatively use Base.is_valid_identifier, since the main point here is to print functions that are accessible in the module by identifier.

julia> @eval Base.MainInclude export broken

julia> broken = 2
2

julia>  ?(┌ Error: Error in the keymap
│   exception =
│    UndefVarError: `broken` not defined in `Base.MainInclude`

@vtjnash vtjnash added REPL Julia's REPL (Read Eval Print Loop) backport 1.12 Change should be backported to release-1.12 labels Apr 24, 2025
@mbauman mbauman changed the title [REPLCompletions] use less asinine version of listing module imports for completions [REPLCompletions] use better version of listing module imports for completions Apr 24, 2025
…mpletions

The test crashed on my machine, and I noticed the implementation of this
function was entirely nonsense. ~~Fortunately, we already had a more
correct implementation of this function in the REPL docview code that we
could borrow from to fix this.~~ Turns out this implementation was also
wrong, since it failed to account for the difference between
explicit-using and explicit-import. Instead borrow the hopefully more
correct implementation of this function in REPLCompletions
append_filtered_mod_names instead.

While here, also filter out macros, since those are rather strange
looking to see appearing in the results. We could alternatively use
`Base.is_valid_identifier`, since the main point here is to print
functions that are accessible in the module by identifier.

```
julia> @eval Base.MainInclude export broken

julia> broken = 2
2

julia>  ?(┌ Error: Error in the keymap
│   exception =
│    UndefVarError: `broken` not defined in `Base.MainInclude`
```
@vtjnash vtjnash force-pushed the jn/repl-accessible-complete branch from 06797f0 to f8fe09b Compare April 25, 2025 18:16
@vtjnash vtjnash requested a review from xal-0 April 25, 2025 18:16
@vtjnash
Copy link
Member Author

vtjnash commented Apr 25, 2025

(assigning Sam as a reviewer since he recently fixed a lot of other parsing issues here, and there is another similar regex-as-parser use here as well for turning ascii sequences of a.b.c.? into symbols, completely ignoring the existence of non-ascii unicode module names, which might be fixable in a followup PR)

@vtjnash vtjnash merged commit 8780aa9 into master Apr 28, 2025
7 checks passed
@vtjnash vtjnash deleted the jn/repl-accessible-complete branch April 28, 2025 15:03
KristofferC pushed a commit that referenced this pull request Apr 29, 2025
…mpletions (#58218)

The test for this crashes on my machine, leading me to notice that the
implementation of this function was nonsense. Fortunately, we already
had a more correct implementation of this function in the REPL docview
code that we could borrow from to fix this.

While here, also filter out macros, since those are rather strange
looking to see appearing in the results. We could alternatively use
`Base.is_valid_identifier`, since the main point here is to print
functions that are accessible in the module by identifier.

```
julia> @eval Base.MainInclude export broken

julia> broken = 2
2

julia>  ?(┌ Error: Error in the keymap
│   exception =
│    UndefVarError: `broken` not defined in `Base.MainInclude`
```

(cherry picked from commit 8780aa9)
@KristofferC KristofferC mentioned this pull request Apr 29, 2025
44 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12 REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant