Skip to content

Conversation

@FnControlOption
Copy link
Contributor

@FnControlOption FnControlOption commented May 31, 2025

Screenshot 2025-05-30 at 10 10 45 PM Screenshot 2025-06-15 at 8 51 17 AM Screenshot 2025-06-15 at 8 52 16 AM Screenshot 2025-06-15 at 8 51 30 AM Screenshot 2025-06-15 at 8 59 54 AM

@FnControlOption FnControlOption force-pushed the hover branch 4 times, most recently from c4afad2 to 27b74cb Compare June 5, 2025 21:42
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.

const foo = if (true) 1 else true;

const bar = if (true)
    .{ foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo }
else
    .{ foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo, foo };

Hovering over bar will cause ZLS to consume huge amounts of memory. Not sure when it stops but it's at least 10GB.

@FnControlOption FnControlOption marked this pull request as draft June 11, 2025 15:01
@FnControlOption FnControlOption force-pushed the hover branch 3 times, most recently from a438dfc to d8be34f Compare June 18, 2025 13:13
@FnControlOption FnControlOption marked this pull request as ready for review June 18, 2025 13:19
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.

const a = if (true) 1 else true;
const b = if (true) false else 0;
const c = if (true) .{ a, b } else .{ b, a };
const d = if (true) .{ a, c } else .{ b, c };
const e = if (true) .{ c, d } else .{ d, c };
const f = if (true) .{ d, e } else .{ e, d };
const g = if (true) .{ e, f } else .{ f, e };
const h = if (true) .{ f, g } else .{ g, f };

Hovering over h will hang. Limiting the total number of combinations that will be analyzed should be a simple solution to this.

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 shouldn't be a config option for this. We can pick a limit that is high enough to not affect users with real code but low enough to prevent hand crafted examples like mine. There is no point in giving users choices they shouldn't need to make.

For testing, the option in analysis.zig can remain with a default value. The specific tests can then override it if needed.

Set the maximum to 200 since VS Code's hover popup seems to be
limited to ~220 lines anyways.
@Techatrix Techatrix merged commit 80941aa into zigtools:master Jun 25, 2025
6 checks passed
@FnControlOption FnControlOption deleted the hover branch June 25, 2025 17:24
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