-
-
Notifications
You must be signed in to change notification settings - Fork 394
feat: added folding range support for imports #2514
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
feat: added folding range support for imports #2514
Conversation
|
Hey @Techatrix, can u please review this PR. Thanks. |
c21183a to
653c9a7
Compare
|
Hey @Techatrix, is this issue #2103 planned for the next release ? |
|
ig the checks are failing due to rebase |
653c9a7 to
ff08db3
Compare
ff08db3 to
9092d20
Compare
Techatrix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This recursively drills down field accesses to find the base.
This doc comment doesn't match the implementation. It's not recursive and doesn't look drill down a sequence of field accesses.
Field accesses should recursively check the left side of the expression. We can also assume that identifiers refer to aliases which should lead to better results than assuming the opposite. In the future, this logic should ideally be shared with code actions.
Here is how this could be implemented (without actual recursion)
fn isImportOrAlias(tree: *const Ast, init_node: Ast.Node.Index) bool {
var node = init_node;
while (true) {
node = switch (tree.nodeTag(node)) {
.builtin_call_two, .builtin_call_two_comma => {
// just like before
},
.field_access => tree.nodeData(node).node_and_token[0],
.identifier => return true,
else => return false,
};
}
}
src/features/folding_range.zig
Outdated
| const var_decl = tree.simpleVarDecl(node); | ||
| const init_node = var_decl.ast.init_node.unwrap() orelse break :blk false; | ||
|
|
||
| // Check if this is an @import() call or a field access/identifier based on an import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Check if this is an @import() call or a field access/identifier based on an import |
This comment is almost the same as the doc comment of isImportOrAlias so it doesn't carry any additional information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Techatrix, thanks for the feedback on the PR. Got your point, the earlier implementation seems to be half baked from my side. You're absolutely right that the doc comment didn't match the implementation.
I implemented your suggestion, and it works much better. However, I'm seeing a case where it might create false positives. For example:
const FoldingRange = struct { ... }; // Local struct
const std = @import("std");
const loc = FoldingRange.loc; // ← Gets included in fold rangeSince we assume all identifiers are aliases, FoldingRange.loc gets included even though FoldingRange is a local struct, not an import. Is this trade-off acceptable for the folding range use case, or should we add additional heuristics to filter these out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this trade off acceptable. A future improvement can reuse the logic that is used for code actions.
9092d20 to
9419f58
Compare
Closes #2103
.inclusiverange to include semicolonsA simple demo to see this feature in action:

