zigtools
feat: Add hover for structs
This PR adds hover support for structs, enums, and unions without displaying member functions, as discussed in #1568. Going off of this comment from this issue, I also changed the final layout so that doc comments appear on top rather than the bottom of the hover window. If this needs to be discussed further/ moved into a separate PR I'm happy to do so!
A few examples:
const S = struct {
fn foo() void {
// many lines here
}
fld: u8,
};
const ComptimeReason = union(enum) {
c_import: struct {
block: *Block,
src: LazySrcLoc,
},
comptime_ret_ty: struct {
block: *Block,
func: Air.Inst.Ref,
func_src: LazySrcLoc,
return_ty: Type,
},
fn explain(cr: ComptimeReason, sema: *Sema, msg: ?*Module.ErrorMsg) !void {
const parent = msg orelse return;
// many lines later...
};
cc: @llogick Closes #1568
Hover over blah.
pub const blah = struct {
const str = "something";
};
backtrace
thread 111256 panic: attempt to use null value
/home/lee/src/zls/src/analysis.zig:515:100: 0x156596e in getContainerFieldMembers (zls)
const field_container_decl = tree.fullContainerDecl(&buf, inner_decl.ast.init_node).?;
^
/home/lee/src/zls/src/analysis.zig:453:41: 0x14a4d8f in getVariableSignature (zls)
try getContainerFieldMembers(allocator, &desc, tree, container_decl, start_token, true, 0);
^
/home/lee/src/zls/src/features/hover.zig:71:61: 0x1400e03 in hoverSymbolRecursive (zls)
break :def try Analyser.getVariableSignature(arena, tree, var_decl, true);
^
/home/lee/src/zls/src/features/hover.zig:23:32: 0x137c000 in hoverSymbol (zls)
return hoverSymbolRecursive(analyser, arena, decl_handle, markup_kind, &doc_strings);
^
/home/lee/src/zls/src/features/hover.zig:279:42: 0x12fa5c2 in hoverDefinitionGlobal (zls)
.value = (try hoverSymbol(analyser, arena, decl, markup_kind)) orelse return null,
^
/home/lee/src/zls/src/features/hover.zig:427:49: 0x12f6596 in hover (zls)
.var_access => try hoverDefinitionGlobal(analyser, arena, handle, source_index, markup_kind, offset_encoding),
^
/home/lee/src/zls/src/Server.zig:1469:41: 0x1293a71 in hoverHandler (zls)
const response = hover_handler.hover(&analyser, arena, handle, source_index, markup_kind, server.offset_encoding);
^
/home/lee/src/zls/src/Server.zig:1803:58: 0x1253827 in sendRequestSync__anon_21614 (zls)
.@"textDocument/hover" => try server.hoverHandler(arena, params),
^
/home/lee/src/zls/src/Server.zig:1881:62: 0x120d045 in processMessage (zls)
const result = try server.sendRequestSync(arena_allocator.allocator(), @tagName(method), params);
^
/home/lee/src/zls/src/Server.zig:1908:33: 0x11c2295 in processMessageReportError (zls)
return server.processMessage(message) catch |err| {
^
/home/lee/src/zls/src/Server.zig:1946:62: 0x11a405f in processJob (zls)
const response = server.processMessageReportError(parsed_message.value) orelse return;
^
/home/lee/zig/0.14.0-dev.14+ec337051a/files/lib/std/Thread/Pool.zig:152:39: 0x11a31be in runFn (zls)
@call(.auto, func, closure.arguments);
^
/home/lee/zig/0.14.0-dev.14+ec337051a/files/lib/std/Thread/Pool.zig:191:18: 0x124a2f4 in worker (zls)
runFn(&run_node.data);
^
debug: (server): Took 0ms to process notification-$/cancelRequest on Thread 111255
/home/lee/zig/0.14.0-dev.14+ec337051a/files/lib/std/Thread.zig:408:13: 0x11fdfdd in callFn__anon_19979 (zls)
@call(.auto, f, args);
^
/home/lee/zig/0.14.0-dev.14+ec337051a/files/lib/std/Thread.zig:1226:30: 0x11b1c17 in entryFn (zls)
return callFn(f, self.fn_args);
^
/home/lee/zig/0.14.0-dev.14+ec337051a/files/lib/c.zig:239:13: 0x17d4290 in clone (c)
asm volatile (
^
???:?:?: 0xaaaaaaaaaaaaaaa9 in ??? (???)
Unwind information for `???:0xaaaaaaaaaaaaaaa9` was not available, trace may be incomplete
[Info - 21:54:16] Connection to server got closed. Server will restart.
Oh wow, I completely blanked on covering that case. I'll be able to take another look later tonight. Sorry about that!
I reworked the second case to avoid the crash and added a "catchall" case for variable declarations that aren't full container decls.
Given that the number of different nodes that can get caught in the "} else if (Ast.fullVarDecl(tree, member)) |inner_decl| {" case is rather large, I wasn't sure how best to tackle displaying this information without using tokensToSlice(), pulling in directly from the source file. For example, the funky indentation for .fld and }; in the following example...
const EdgeCases = struct {
const str = "something";
const s = S{
.fld = 0,
};
};
...shows up in the hover window using this approach:
I'm not sure how to better handle such a general parsing problem without needlessly reinventing the wheel. Maybe zig fmt could be plugged in to handle this case?
Thank you for working on this !
My opinion: Every member should be a single line, except for
field: struct {
...
},
where surely no one would put more than 5 fields and absolutely no decls.
To me, the perfect balance would be
// Could be in any file
const T = struct {
inner: u8,
pub const default: T = .{ .inner = 1 };
const Self = @This();
pub fn inc(self: Self) void {
self.inner += 1;
}
};
test {
// or as fn param, ie v: T
const v = T.default; // Hover over T, hovering over `default` would still show `pub const default: T = .{ .inner = 1 };`
// much code here
_ = v;
}
Result (only fields and pub decls, ie only what's available at the point of the var decl) :
const T = struct {
inner: u8,
pub const default: T
pub fn inc(self: Self) void
};
This gives me a general idea of what v is and how it is/can be used in the code following it's decl.
I spent some more time with this and worked out how to get utilize some pieces of zig fmt for that edge case. I believe the other cases could be simplified by also using this approach, but at the cost of performance as it does perform some extra allocations, tokenization, parsing, etc. Any thoughts here regarding the tradeoff?
I also wanted to mention that I currently have an extra empty line between separate decls, e.g. the following code snippet
const EdgeCases = struct {
const str = "something";
const s = S{
.fld = 0,
};
};
has the following hover window (with the extra empty line in between str and s):
I think this helps with readability, but I'd be fine removing said extra line if that's preferred :)
@llogick Funny timing! Sorry I didn't see your comment before pushing again. I'll take another look at this tomorrow :)
Alright I added support for functions, changed the existing code so that only pub decls show up, and removed the extra space between decls (there is still an empty line going from a field to a decl). ~~I held off on writing any additional tests because I'd like to cement some of this stuff first, but I am planning on covering these new cases once the code is a bit more finalized.~~ I added an additional test with some private decls and a public member function.
Here's what llogick's example looks like with the current code. I think it might be worth it to keep default values (e.g. the = .{ .inner = 1 }; below), but I'd be happy to rework if need be.
Would it be possible to show
@sizeOf? Thanks!
I like this idea, but don't have a great idea of how I'd implement it. I'll spend some time on it this weekend. :)
Related: #1677
I have an alternative solution, just use the source code as-is and then remove extra indentation.
Nice! I reworked it a bit to also show pub functions and decls.
@Techatrix No worries if this isn't a priority or you just don't have the bandwidth to review this currently, but I wanted to check in and ask if there's anything else I can do to help this move forward. :)
Sorry neglecting this PR over the past few weeks. I was planing to give this another look but always got distracted with other stuff. I will review this now.
Some of the code was very deeply nested so I went ahead and cleaned it up a bit. I also added the missing comma for each container field.
Awesome, thanks!
Not sure how I feel moving the doc comment to the top but I can why it's done. I can give give it a test ride and see how it goes.
Fair enough. My original reason for making the switch was @llogick's comment in #1568, but I'm pretty indifferent either way.
The main issue I see is that you added variable and function declarations which is not what has been proposed in #1568 and I not think it is an improvement. Please change it back to only show container fields.
I had originally added in variable and function declarations because of @llogick's comment earlier in this thread. I personally liked including the public function signatures to show how a container can be used, but I'm ok with removing them.
I had originally added in variable and function declarations because of @llogick's comment earlier in this thread. I personally liked including the public function signatures to show how a container can be used, but I'm ok with removing them.
I get the reasoning for removing these (seems like it wasn't part of the original issue), but if I made a new issue requesting pub functions + declarations could they be added back in?
