Skip to content
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

Inlay Hints for Control Flow Capture Values #1512

Merged
merged 11 commits into from
Oct 21, 2023

Conversation

Koranir
Copy link
Contributor

@Koranir Koranir commented Oct 11, 2023

Adds inlay hints hints for the capture values of the control flow statements (if, while, for, switch).
image
image
image

Fixed multiple capture values not working
image

@llogick
Copy link
Member

llogick commented Oct 11, 2023

No guarantees that this is the best way, or even the right way

diff --git a/src/features/inlay_hints.zig b/src/features/inlay_hints.zig
index c3f9151..2431a34 100644
--- a/src/features/inlay_hints.zig
+++ b/src/features/inlay_hints.zig
@@ -315,7 +315,13 @@ fn writeForCaptureHint(builder: *Builder, for_node: Ast.Node.Index) !void {
     defer tracy_zone.end();
 
     const hint = builder.handle.tree.fullFor(for_node) orelse return;
-    try appendTypeHintString(builder, hint.payload_token, try typeStrOfToken(builder, hint.payload_token) orelse return);
+    const token_tags = builder.handle.tree.tokens.items(.tag);
+    var token_index: u32 = hint.payload_token;
+    while (token_tags[token_index] != .pipe) : (token_index += 1) {
+        if (token_tags[token_index] == .comma) continue;
+        if (token_tags[token_index] == .asterisk) // Prepend `*` to the inlay hint
+        try appendTypeHintString(builder, token_index, try typeStrOfToken(builder, token_index) orelse return);
+    }
 }
 
 fn writeWhileCaptureHint(builder: *Builder, while_node: Ast.Node.Index) !void {

@Techatrix
Copy link
Member

I would suggest the copy the code from analysis.zig here because it has already been tested, fuzzed and matches to code used by AstGen.

@Koranir Koranir marked this pull request as ready for review October 11, 2023 22:08
Copy link
Member

@llogick llogick left a comment

Choose a reason for hiding this comment

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

Techatrix is correct

src/features/inlay_hints.zig Outdated Show resolved Hide resolved
Copy link
Member

@llogick llogick left a comment

Choose a reason for hiding this comment

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

Other than the type needing to be future-proofed this looks ready.

Some subjective remarks:

  • I wish zig itself wasn't using node <-> node_index interchangeably
  • Ditto token <-> token_index
  • hint isn't a good fit for the name of the result of fns that return (full)X node
  • These are so short, they could've been part of the switch

src/features/inlay_hints.zig Outdated Show resolved Hide resolved
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.

There is a missing test case for switch case captures and a test to see if for("",0..) |_, here<usize>| works.

I am also wondering if there is any need to add a new config option. Shouldn't inlay_hints_show_variable_declaration and inlay_hints_show_capture_variables be merged into a single option since they both are type hints or is there interest in having only one of them enabled? Keep in mind that something like #1531 may even add another option to the mix.

src/features/inlay_hints.zig Outdated Show resolved Hide resolved
src/features/inlay_hints.zig Outdated Show resolved Hide resolved
src/features/inlay_hints.zig Show resolved Hide resolved
- Combined inlay_hints_show_variable_declaration & inlay_hints_show_capture_variables into inlay_hints_show_variable_type_hints
- Use .? instead of orelse return (i checked there is no way to return null)
- Added tests for discards
@Koranir
Copy link
Contributor Author

Koranir commented Oct 16, 2023

When opting for inlay hints for variables, I suppose you'd want it on all the variables.

@llogick llogick merged commit 5b056e5 into zigtools:master Oct 21, 2023
3 checks passed
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.

None yet

3 participants