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

Hover improvements #1282

Merged
merged 5 commits into from
Jul 2, 2023
Merged

Hover improvements #1282

merged 5 commits into from
Jul 2, 2023

Conversation

FnControlOption
Copy link
Contributor

@FnControlOption FnControlOption commented Jun 30, 2023

Follow up to #1281

Resolve underlying type recursively

new recursive underlying
Before old recursive underlying

Avoid duplicate "Go to <type>"

new duplicates
Before old duplicates

Add hover info for optional type and error union

optional type error union

Simplify resolved if/switch type when branches return same type

page allocator

Resolve type to alias for "Go to <type>" when possible

new function new variable
Before old function old variable

For reference, Ast.Node.Index is defined as const Index = u32;

This changes the behavior of "Go to std.mem.Allocator". Instead of opening std/mem/Allocator.zig, now it will open std/mem.zig and go to the line that has const Allocator = @import("mem/Allocator.zig");

allocator parameter allocator alias

@llogick
Copy link
Contributor

llogick commented Jun 30, 2023

Maybe it's too early in the morning here and I'm not getting the full picture ..but I have reservations
about the "Resolve each type as an alias when possible" change as showcased in the image on the right:

1 Knowing the underlying type at a glance is valuable,
2 It presents no information that couldn't be gleaned by reading the line above it.

@InKryption
Copy link
Contributor

Is there some way in which both the alias and the actual type could be provided?

@FnControlOption
Copy link
Contributor Author

1 Knowing the underlying type at a glance is valuable,
2 It presents no information that couldn't be gleaned by reading the line above it.

Thank you for the feedback. These are valid objections! My original thought process was that since composite types like []Ast.Node.Index currently don't get translated into []u32, it would make sense not to worry about showing the underlying type of an individual Ast.Node.Index. The other option I was considering is reconstructing type strings from scratch to get e.g. []u32 but that seemed a like a lot of work for relatively little gain. However, now that I know that some people are interested, I think I'll go down that route instead.

Is there some way in which both the alias and the actual type could be provided?

Great suggestion, that definitely should be possible.

@llogick
Copy link
Contributor

llogick commented Jun 30, 2023

This looks excellent!

Regarding

This changes the behavior of "Go to std.mem.Allocator". Instead of opening std/mem/Allocator.zig, now it will open std/mem.zig and go to the line that has const Allocator = @import("mem/Allocator.zig");

I have no strong opinion on the matter, this is what Go To Def does as it is, and although it might be convenient to provide the choice it might get cramped.

Technically Go To Def should pop open std/mem/Allocator.zig, and Go To Decl land you at const Allocator = @import("mem/Allocator.zig");, but I believe Auguste feels strongly about this being the way it is.

@leecannon leecannon merged commit 46fd5c5 into zigtools:master Jul 2, 2023
@FnControlOption FnControlOption deleted the go-to branch July 2, 2023 13:25
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.

4 participants