Skip to content

Conversation

@quaint
Copy link

@quaint quaint commented Aug 21, 2023

This pull request adapts zls as lib demo to zls version 0.11.0.

Things I'm not sure:

  • is there any replacement for server.maybeFreeArena
  • why unwrap is needed on line 84 even though orelse provides default value

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

I've noticed some things that need to be resolved before this can be merged.
If you have any more question, please let me know.

.optimize = optimize,
});
exe.addModule("zls", b.dependency("zls", .{}).module("zls"));
exe.install();
Copy link
Member

Choose a reason for hiding this comment

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

this should be replaced with b.installArtifact(exe);

// Free the server arena if it's past
// a certain threshold
defer server.maybeFreeArena();
//defer server.maybeFreeArena();
Copy link
Member

Choose a reason for hiding this comment

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

there is no equivalent function for maybeFreeArena. Instead you should create your own ArenaAllocator here and use it when sending request to the zls.

const completions: []const zls.types.CompletionItem = (try server.completionHandler(.{

const emptyList: zls.Server.ResultType("textDocument/completion") = .{ .CompletionList = zls.types.CompletionList{ .isIncomplete = true, .items = &.{} } };
const completions: []const zls.types.CompletionItem = (try server.completionHandler(allocator, .{
Copy link
Member

Choose a reason for hiding this comment

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

I didn't intend for the completionHandler to be directly accesible here. Could you please call server.sendRequestSync(arena, "textDocument/completion", params) here.

You are also leaking memory here because completionHandler expects a arena-like allocator but you are passing std.heap.page_allocator.

.character = @intCast(4 + input.len),
},
}) orelse zls.types.CompletionList{ .isIncomplete = true, .items = &.{} }).items;
}) orelse emptyList).?.CompletionList.items;
Copy link
Member

Choose a reason for hiding this comment

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

the unwrap is needed here because zls.Server.ResultType("textDocument/completion") is the following type:
https://github.com/zigtools/zls/blob/5bfff2a4b9ee01a7bab5fc26fad6e174f289c28d/src/lsp.zig#L7441-L7445

?union(enum) {
    array_of_CompletionItem: []const CompletionItem,
    CompletionList: CompletionList,
    pub usingnamespace UnionParser(@This());
}

This means that ((try server.completionHandler(...)) orelse emptyList) resolves to an optional because emptyList is optional.

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.

2 participants