Skip to content

Commit

Permalink
Insert completion snippet when in front of local var (#7877)
Browse files Browse the repository at this point in the history
* Fix error message in position_assertions.cc

* Add a failing test case

    + exec test/lsp_test_runner --single_test=test/testdata/lsp/completion/snippet_before_variable.rb
    Pausing
    Resuming
    [doctest] doctest version is "2.4.9"
    [doctest] run with "--help" for options
    ===============================================================================
    test/lsp_test_runner.cc:525:
    TEST CASE:  LSPTest

    test/helpers/expectations.cc:209: ERROR: The expected (rbedited) file contents for this completion did not match the actual file contents post-edit
    @@ -9,7 +9,7 @@
     end

     begin
    -  A.example(${1:Integer})${0}
    +  A.example${0}
       # ^ completion: example, ...
       # ^ apply-completion: [B] item: 0

    ===============================================================================
    [doctest] test cases:  1 |  0 passed | 1 failed | 0 skipped
    [doctest] assertions: 67 | 66 passed | 1 failed |
    [doctest] Status: FAILURE!
    ================================================================================

* Fix failing test

What's happening here is that Sorbet sees

    foo0 = Foo.
    foo1 = Foo.new(1, 2)

as the same as this:

    foo0 = Foo.foo1=(Foo.new(1, 2))

They both have this parse result:

    ❯ sorbet -p desugar-tree -e $'foo0 = Foo.\n foo1 = Foo.new(1, 2)\n' 2> /dev/null
    class <emptyTree><<C <root>>> < (::<todo sym>)
      foo0 = <emptyTree>::<C Foo>.foo1=(<emptyTree>::<C Foo>.new(1, 2))
    end

Which makes it hard for sorbet to distinguish this from a differen case,
where it likely wouldn't make sense to insert a snippet:

    x = self.already_has_args(1, 2)
    #                        ^ completion: ...

This change attempts to disambiguate that by checking whether the query
location is at the end of the line. This is simplistic (even a
whitespace character at the end of the line will break this heuristic),
but I think that eventually we should probably stop attempting to
generate snippets for method completion items because other language
clients don't do it, and there's a better LSP method to guide people
towards filling in arguments, namely signature helper.
  • Loading branch information
jez committed May 9, 2024
1 parent 80c8322 commit f8ea752
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 5 deletions.
19 changes: 15 additions & 4 deletions main/lsp/requests/completion.cc
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,8 @@ vector<core::NameRef> allSimilarLocalNames(const core::GlobalState &gs, const ve
}

string methodSnippet(const core::GlobalState &gs, core::DispatchResult &dispatchResult, core::MethodRef maybeAlias,
const core::TypePtr &receiverType, const core::TypeConstraint *constraint, uint16_t totalArgs) {
const core::TypePtr &receiverType, const core::TypeConstraint *constraint, uint16_t totalArgs,
core::Loc queryLoc) {
fmt::memory_buffer result;
auto shortName = maybeAlias.data(gs)->name.shortName(gs);
auto isSetter = maybeAlias.data(gs)->name.isSetter(gs);
Expand All @@ -315,8 +316,18 @@ string methodSnippet(const core::GlobalState &gs, core::DispatchResult &dispatch
* since the rest is likely useless
*/
if (totalArgs > 0 || dispatchResult.main.blockReturnType != nullptr) {
fmt::format_to(std::back_inserter(result), "${{0}}");
return to_string(result);
auto maybeNewline = queryLoc.adjustLen(gs, 0, 1);
// ... but carve out an exception if the query location is at the end of the line, because
// then likely it only looks like there are "arguments" to this method call because of how
// Ruby allows the `x.` being split from the method name on the next line
//
// (This isn't a great solution, but I think that we're likely going to revisit generating
// snippets here in the future with a bit of an overhaul of how completion works, so I'm
// fine with it in the mean time.)
if (!maybeNewline.exists() || maybeNewline.source(gs) != "\n") {
fmt::format_to(std::back_inserter(result), "${{0}}");
return to_string(result);
}
}

if (isSetter) {
Expand Down Expand Up @@ -1044,7 +1055,7 @@ CompletionTask::getCompletionItemForMethod(LSPTypecheckerDelegate &typechecker,
string replacementText;
if (supportsSnippets) {
item->insertTextFormat = InsertTextFormat::Snippet;
replacementText = methodSnippet(gs, dispatchResult, maybeAlias, receiverType, constraint, totalArgs);
replacementText = methodSnippet(gs, dispatchResult, maybeAlias, receiverType, constraint, totalArgs, queryLoc);
} else {
item->insertTextFormat = InsertTextFormat::PlainText;
replacementText = label;
Expand Down
2 changes: 1 addition & 1 deletion test/helpers/position_assertions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1494,7 +1494,7 @@ shared_ptr<ApplyCompletionAssertion> ApplyCompletionAssertion::make(string_view

ADD_FAIL_CHECK_AT(
string(filename).c_str(), assertionLine + 1,
fmt::format("Improperly formatted apply-completion assertion. Expected '[<version>] <index>'. Found '{}'",
fmt::format("Improperly formatted apply-completion assertion. Expected '[<version>] item: <index>'. Found '{}'",
assertionContents));

return nullptr;
Expand Down
22 changes: 22 additions & 0 deletions test/testdata/lsp/completion/snippet_before_variable.A.rbedited
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# typed: true

class A
extend T::Sig
sig { params(x: Integer).void }
def self.example(x)
puts(x + 1)
end
end

begin
A.
# ^ completion: example, ...
# ^ apply-completion: [B] item: 0

x = nil # error: does not exist

A.example(${1:Integer})${0}
# ^ completion: example, ...
# ^ apply-completion: [A] item: 0

end # error: unexpected token "end"
22 changes: 22 additions & 0 deletions test/testdata/lsp/completion/snippet_before_variable.B.rbedited
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# typed: true

class A
extend T::Sig
sig { params(x: Integer).void }
def self.example(x)
puts(x + 1)
end
end

begin
A.example(${1:Integer})${0}
# ^ completion: example, ...
# ^ apply-completion: [B] item: 0

x = nil # error: does not exist

A.
# ^ completion: example, ...
# ^ apply-completion: [A] item: 0

end # error: unexpected token "end"
22 changes: 22 additions & 0 deletions test/testdata/lsp/completion/snippet_before_variable.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# typed: true

class A
extend T::Sig
sig { params(x: Integer).void }
def self.example(x)
puts(x + 1)
end
end

begin
A.
# ^ completion: example, ...
# ^ apply-completion: [B] item: 0

x = nil # error: does not exist

A.
# ^ completion: example, ...
# ^ apply-completion: [A] item: 0

end # error: unexpected token "end"

0 comments on commit f8ea752

Please sign in to comment.