track comptime types #2133

mfield · 2025-01-04T21:13:20Z

Adds some tracking of comptime types.

changes formatting for comptime structs, e.g. HashMap(...) -> HashMap(i32,void). maybe just a preference of mine? but i like seeing it. could easily change the formatter back to ....

fixes:

repeated use of the same comptime function Allocator usage causes zls inline types to get very confused #1841
nested generic structs e.g. incorrect completions on nested generic type like std.ArrayList #1392

mfield · 2025-01-07T01:01:15Z

sorry for the false start. saw something while reading the diff and made a small change last second without re-building :(
new version is passing tests again!

Techatrix

The analysis backend has an existing mechanism to resolve comptime function type parameters. See bound_type_params. I would have expected this field to be replaced instead of adding another solution on top of it. Removing bound_type_params will make most of the existing test fail that deal with generic functions which indicates that this PR is failing to handle them.

Also, have you tried to adding the extra properties of BoundScope to ScopeWithHandle instead of adding .bound_scope to Type.Data? If you need to wrap a container with a bound scope, just create a copy of the existing container with extra bound scopes. I tried to test this change myself but could not quite get it to work so there may be a good reason to keep it this way.

I will hold of with additional review comments until the fundamentals are in a good shape. The idea of keeping a stack of bound parameter types is also what I had in mind when thinking about how to resolve #1392.

- remove BoundScope and put params in ScopeWithHandle - remove bound_type_params hashmap

mfield · 2025-01-19T04:30:40Z

Thanks for the review. Finally had some time to try out your suggestions and got the two big ones working:

bound_type_params is removed
bound_params moved from BoundScope to ScopeWithHandle

I'm not terribly fond of my change to DeclWithHandle nor pushing and popping comptime_state from completions.zig. Would be down to iterate again there if you have any suggestions. The two-pass collectDeclarationsOfContainer then declToCompletion for each decl collected does make some sense to draw a nice line between analysis.zig and completions.zig so I wasn't sure how to keep comptime_state discovered during collectDeclarationsOfContainer available to declToCompletion...

Techatrix

Apologies for the late response. I have a bunch of comments that are mostly refactoring suggestions but overall these changes look good to me.

About the changes in completions.zig. To avoid them, I tried the following change:

patch.diff

diff --git a/src/analysis.zig b/src/analysis.zig
index 8f53529..cf1ad7b 100644
--- a/src/analysis.zig
+++ b/src/analysis.zig
@@ -4141,6 +4141,16 @@ pub const DeclWithHandle = struct {
         const tracy_zone = tracy.trace(@src());
         defer tracy_zone.end();
 
+        const starting_depth = analyser.comptime_state.depth();
+        var pushed = false;
+        if (self.from) |from| {
+            if (from.bound_params.len > 0) {
+                try analyser.comptime_state.push(analyser.gpa, from.bound_params);
+                pushed = true;
+            }
+        }
+        defer if (pushed) analyser.comptime_state.pop(starting_depth);
+
         const tree = self.handle.tree;
         const resolved_ty = switch (self.decl) {
             .ast_node => |node| try analyser.resolveTypeOfNodeInternal(
@@ -4348,6 +4358,10 @@ pub fn collectDeclarationsOfContainer(
     const scope = container_scope.scope;
     const handle = container_scope.handle;
 
+    const starting_depth = analyser.comptime_state.depth();
+    try analyser.comptime_state.push(analyser.gpa, container_scope.bound_params);
+    defer analyser.comptime_state.pop(starting_depth);
+
     const tree = handle.tree;
     const document_scope = try handle.getDocumentScope();
     const container_node = container_scope.toNode();
diff --git a/src/features/completions.zig b/src/features/completions.zig
index 2374702..9bf510b 100644
--- a/src/features/completions.zig
+++ b/src/features/completions.zig
@@ -103,9 +103,6 @@ fn typeToCompletion(builder: *Builder, ty: Analyser.Type) error{OutOfMemory}!voi
             });
         },
         .container => |scope_handle| {
-            const starting_depth = builder.analyser.comptime_state.depth();
-            try builder.analyser.comptime_state.push(builder.analyser.gpa, scope_handle.bound_params);
-            defer builder.analyser.comptime_state.pop(starting_depth);
             var decls: std.ArrayListUnmanaged(Analyser.DeclWithHandle) = .empty;
             try builder.analyser.collectDeclarationsOfContainer(scope_handle, builder.orig_handle, !ty.is_type_val, &decls);
 
@@ -173,15 +170,6 @@ fn declToCompletion(builder: *Builder, decl_handle: Analyser.DeclWithHandle, opt
         doc_comments.appendAssumeCapacity(docs);
     }
 
-    const starting_depth = builder.analyser.comptime_state.depth();
-    var pushed = false;
-    if (decl_handle.from) |from| {
-        if (from.bound_params.len > 0) {
-            try builder.analyser.comptime_state.push(builder.analyser.gpa, from.bound_params);
-            pushed = true;
-        }
-    }
-    defer if (pushed) builder.analyser.comptime_state.pop(starting_depth);
     const maybe_resolved_ty = try decl_handle.resolveType(builder.analyser);
 
     if (maybe_resolved_ty) |resolve_ty| {

It only fails a completion test about usingnamespace. I haven't investigated it further but it seems promising. If this turns out be a dead end or to complex, I would be fine to keep the change to completions.zig as is.

Techatrix · 2025-02-05T01:54:30Z

src/analysis.zig

+// nested scopes with comptime bindings
+comptime_state: ComptimeState = .{},
+
+const empty_params = ([0]ScopeWithHandle.Param{})[0..];


You can replace every occurrence of empty_params with &.{}. It will coerce to the correct type.
See https://ziglang.org/documentation/master/#toc-Slices for more information.

Techatrix · 2025-02-05T01:55:34Z

src/analysis.zig

@@ -698,7 +702,11 @@ pub fn resolveVarDeclAlias(analyser: *Analyser, node_handle: NodeWithHandle) err
 }

 fn resolveVarDeclAliasInternal(analyser: *Analyser, node_handle: NodeWithHandle, node_trail: *NodeSet) error{OutOfMemory}!?DeclWithHandle {
-    const node_with_uri: NodeWithUri = .{ .node = node_handle.node, .uri = node_handle.handle.uri };
+    const node_with_uri = NodeWithUri{


nit: The codebase uses the following syntax

Suggested change

const node_with_uri = NodeWithUri{

const node_with_uri: NodeWithUri = .{

See #2128

Techatrix · 2025-02-05T01:58:59Z

src/analysis.zig

+                const ptyp = try analyser.arena.allocator().create(Type);
+                ptyp.* = argument_type;
+
+                const symbol = offsets.tokenToSlice(func_tree, param.name_token.?);


This will deal with some escaped identifiers like @"foo".

Suggested change

const symbol = offsets.tokenToSlice(func_tree, param.name_token.?);

const symbol = offsets.identifierTokenToNameSlice(func_tree, param.name_token.?);

Techatrix · 2025-02-05T02:01:00Z

src/analysis.zig

+    pub const Iterator = struct {
+        idx: usize,
+        items: Stack.Slice,
+
+        pub fn next(iter: *Iterator) ?[]ScopeWithHandle.Param {
+            if (iter.idx > 0) {
+                iter.idx -= 1;
+                return iter.items[iter.idx];
+            }
+            return null;
+        }
+    };


You can use std.mem.reverseIterator instead.

Techatrix · 2025-02-05T02:02:51Z

src/analysis.zig

+        }
+    };
+
+    pub fn resolve(self: @This(), symbol: []const u8) ?*Type {


nit
https://zig.news/kristoff/dont-self-simple-structs-fj8

Suggested change

pub fn resolve(self: @This(), symbol: []const u8) ?*Type {

pub fn resolve(state: ComptimeState, symbol: []const u8) ?*Type {

The same should be done to all the other functions.

Techatrix · 2025-02-05T02:10:04Z

src/analysis.zig

@@ -39,6 +38,11 @@ resolve_number_literal_values: bool,
 /// handle of the doc where the request originated
 root_handle: ?*DocumentStore.Handle,

+// nested scopes with comptime bindings
+comptime_state: ComptimeState = .{},


If find the old naming with bound_type_params instead of comptime_state to more accurately represent what this is dealing with. comptime is related to almost all features of Zig but this is specifically about comptime type parameters on functions.

Suggested change

comptime_state: ComptimeState = .{},

bound_type_params: BoundTypeParams = .{},

You can also move this where bound_type_params used to be.

Techatrix · 2025-02-05T02:11:26Z

src/analysis.zig

@@ -336,7 +340,7 @@ pub fn isInstanceCall(
    else blk: {
        const func_node = func_ty.data.other; // this assumes that function types can only be Ast nodes
        const fn_token = func_node.handle.tree.nodes.items(.main_token)[func_node.node];
-        break :blk try innermostContainer(func_node.handle, func_node.handle.tree.tokens.items(.start)[fn_token]);
+        break :blk try innermostContainer(analyser, func_node.handle, func_node.handle.tree.tokens.items(.start)[fn_token]);


nit: It is usually preferred to use the method call syntax if possible:

Suggested change

break :blk try innermostContainer(analyser, func_node.handle, func_node.handle.tree.tokens.items(.start)[fn_token]);

break :blk try analyser.innermostContainer(func_node.handle, func_node.handle.tree.tokens.items(.start)[fn_token]);

See https://ziglang.org/documentation/master/#struct for more information.
There are some other occurrences of this pattern.

Techatrix · 2025-02-05T02:16:34Z

src/analysis.zig

+
+        for (scope_handle.bound_params) |param| {
+            hasher.update(param.symbol);
+            param.typ.*.hashWithHasher(hasher);


nit: When doing a field access, Zig will automatically unwrap a single item pointer if needed.

Suggested change

param.typ.*.hashWithHasher(hasher);

param.typ.hashWithHasher(hasher);

The same happens in the eql function.

Techatrix · 2025-02-05T02:28:20Z

src/analysis.zig

@@ -3175,19 +3391,22 @@ pub fn collectCImportNodes(allocator: std.mem.Allocator, tree: Ast) error{OutOfM
 pub const NodeWithUri = struct {
    node: Ast.Node.Index,
    uri: []const u8,
+    comptime_state: u64,


I would suggest a name like bound_type_params_state_hash for this field instead.

Make sure to use a trailing comma wherever a NodeWithUri is created to format it on multiple lines like this:

const key: NodeWithUri = .{ .node = usingnamespace_node.node, .uri = usingnamespace_node.handle.uri, .bound_type_params_state_hash = try analyser.comptime_state.hash(analyser.arena.allocator()), // < the comma here is important };

Techatrix · 2025-02-05T02:53:09Z

src/analysis.zig

@@ -4409,9 +4626,10 @@ pub fn innermostContainer(handle: *DocumentStore.Handle, source_index: usize) er

 fn resolveUse(analyser: *Analyser, uses: []const Ast.Node.Index, symbol: []const u8, handle: *DocumentStore.Handle) error{OutOfMemory}!?DeclWithHandle {
    for (uses) |index| {
-        const gop = try analyser.use_trail.getOrPut(analyser.gpa, .{ .node = index, .uri = handle.uri });
+        const key: NodeWithUri = .{ .node = index, .uri = handle.uri, .comptime_state = try analyser.comptime_state.hash(analyser.arena.allocator()) };


This key is used to avoid recursively resolving usingnamespace. To be safe, I would suggest to ignore the comptime state.

Suggested change

const key: NodeWithUri = .{ .node = index, .uri = handle.uri, .comptime_state = try analyser.comptime_state.hash(analyser.arena.allocator()) };

const key: NodeWithUri = .{ .node = index, .uri = handle.uri, .comptime_state = 0 };

Same should happen in collectUsingnamespaceDeclarationsOfContainer.

Techatrix requested changes Jan 9, 2025

View reviewed changes

mfield added 3 commits January 18, 2025 11:59

track comptime types

e7b9ee0

fix build

bcba9f4

PR comments:

71c6637

- remove BoundScope and put params in ScopeWithHandle - remove bound_type_params hashmap

mfield force-pushed the comptime branch from 40fdcab to 71c6637 Compare January 19, 2025 04:21

Techatrix requested changes Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

track comptime types #2133

track comptime types #2133

mfield commented Jan 4, 2025

mfield commented Jan 7, 2025

Techatrix left a comment

mfield commented Jan 19, 2025

Techatrix left a comment

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

Techatrix Feb 5, 2025

	const node_with_uri = NodeWithUri{
	const node_with_uri: NodeWithUri = .{

	const symbol = offsets.tokenToSlice(func_tree, param.name_token.?);
	const symbol = offsets.identifierTokenToNameSlice(func_tree, param.name_token.?);

	pub fn resolve(self: @This(), symbol: []const u8) ?*Type {
	pub fn resolve(state: ComptimeState, symbol: []const u8) ?*Type {

	comptime_state: ComptimeState = .{},
	bound_type_params: BoundTypeParams = .{},

	break :blk try innermostContainer(analyser, func_node.handle, func_node.handle.tree.tokens.items(.start)[fn_token]);
	break :blk try analyser.innermostContainer(func_node.handle, func_node.handle.tree.tokens.items(.start)[fn_token]);

	param.typ.*.hashWithHasher(hasher);
	param.typ.hashWithHasher(hasher);

	const key: NodeWithUri = .{ .node = index, .uri = handle.uri, .comptime_state = try analyser.comptime_state.hash(analyser.arena.allocator()) };
	const key: NodeWithUri = .{ .node = index, .uri = handle.uri, .comptime_state = 0 };

track comptime types #2133

Are you sure you want to change the base?

track comptime types #2133

Conversation

mfield commented Jan 4, 2025

mfield commented Jan 7, 2025

Techatrix left a comment

Choose a reason for hiding this comment

mfield commented Jan 19, 2025

Techatrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment