REPLCompletions: Add completions for var"" identifiers #49294
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #49280. Mostly just moving code around, but there's one extra place where we're pattern matching var"". I do hope that after the future parser replacement, we can do these things on the in-progress AST rather than textually.
aviatesk
approved these changes
Apr 10, 2023
Xnartharax
pushed a commit
to Xnartharax/julia
that referenced
this pull request
Apr 19, 2023
* REPLCompletions: Add completions for var"" identifiers Fixes JuliaLang#49280. Mostly just moving code around, but there's one extra place where we're pattern matching var"". I do hope that after the future parser replacement, we can do these things on the in-progress AST rather than textually. * Apply suggestions from code review --------- Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Keno
added a commit
that referenced
this pull request
Apr 27, 2023
The new completion for `var"` fields (#49294) failed when the `var"` was at the end of the completion query, e.g. in `WeirdNames().var"`. This is because we have the following behavior: ``` julia> findprev(==('x'), "x", 1) 1 julia> findprev(==('x'), "x", 2) ``` REPLCompletions attempt to find `.` starting after the `var"`, which in this case is at the end of the string. Of course, the index was probably off by one anyway, because we didn't want to match `var".`, but nevertheless, I find this behavior surprising (ref also [1]). For now, fix this by starting the search before the `var"`, but we may want to revisit the `findprev` behavior also. [1] https://github.com/JuliaLang/julia/pull/35742/files#r420945975
Keno
added a commit
that referenced
this pull request
Apr 28, 2023
The new completion for `var"` fields (#49294) failed when the `var"` was at the end of the completion query, e.g. in `WeirdNames().var"`. This is because we have the following behavior: ``` julia> findprev(==('x'), "x", 1) 1 julia> findprev(==('x'), "x", 2) ``` REPLCompletions attempt to find `.` starting after the `var"`, which in this case is at the end of the string. Of course, the index was probably off by one anyway, because we didn't want to match `var".`, but nevertheless, I find this behavior surprising (ref also [1]). For now, fix this by starting the search before the `var"`, but we may want to revisit the `findprev` behavior also. [1] https://github.com/JuliaLang/julia/pull/35742/files#r420945975
This was referenced Sep 18, 2023
KristofferC
pushed a commit
that referenced
this pull request
Oct 12, 2023
Fix #51194 This PR fixes a regression introduced in #49294, so I believe it should be backported to v1.10. In the current code, completion of `qux(foo, bar.` is detected by parsing `foo(qux, bar` as an incomplete expression, and then looking for the sub-expression to complete (here, `bar.`). This approach fails however for infix calls, since completing `foo + bar.` starts by parsing `foo + bar`, which is a complete call expression, and so the code behaves as if completing `(foo + bar).` instead of `bar.`. This leads to the current problematic behaviour: ```julia julia> Complex(1, 3) + (4//5).#TAB im re ``` which would be correct for `(Complex(1, 3) + (4//5)).#TAB`, but here we expect ```julia julia> Complex(1, 3) + (4//5).#TAB den num ``` This PR fixes that by trying to detect infix calls. In the long term, all this ad-hoc and probably somewhat wrong string processing should be replaced by proper use of `JuliaSyntax` (as mentioned in #49294 (comment), #50817 (comment) and probably other places), but for now at least this fixes the regression.
KristofferC
pushed a commit
that referenced
this pull request
Oct 12, 2023
Fix #51194 This PR fixes a regression introduced in #49294, so I believe it should be backported to v1.10. In the current code, completion of `qux(foo, bar.` is detected by parsing `foo(qux, bar` as an incomplete expression, and then looking for the sub-expression to complete (here, `bar.`). This approach fails however for infix calls, since completing `foo + bar.` starts by parsing `foo + bar`, which is a complete call expression, and so the code behaves as if completing `(foo + bar).` instead of `bar.`. This leads to the current problematic behaviour: ```julia julia> Complex(1, 3) + (4//5).#TAB im re ``` which would be correct for `(Complex(1, 3) + (4//5)).#TAB`, but here we expect ```julia julia> Complex(1, 3) + (4//5).#TAB den num ``` This PR fixes that by trying to detect infix calls. In the long term, all this ad-hoc and probably somewhat wrong string processing should be replaced by proper use of `JuliaSyntax` (as mentioned in #49294 (comment), #50817 (comment) and probably other places), but for now at least this fixes the regression. (cherry picked from commit e949236)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #49280. Mostly just moving code around, but there's one extra place where we're pattern matching var"". I do hope that after the future parser replacement, we can do these things on the in-progress AST rather than textually.