track comptime types

[mfield]

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]

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.

[mfield]

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]

@mfield Do you plan to apply the suggested changes anytime soon? If not, I'd be happy to apply them myself. This PR is a very nice improvement that would be unfortunate to miss out on.

[mfield]

Hi, so sorry, completely lost track of this.

I tried your approach to moving bound_type_params out of completions and into analysis. The usingnamespace case is problematic enough I've backed out of it as well. The difficulty, as I see it, is that resolving the type of a function is too shallow. The Type that comes back doesn't describe the types of the parameters or the return type. So you have to remember to apply the same bound_type_params context to any operations that involve the function type, e.g. firstParamIs.

I think I've applied all the other suggestions. Please let me know if I've missed any.