From 7a8a4e1ec5350160fca3ae8be47b134640f1c5fa Mon Sep 17 00:00:00 2001 From: Alexandros Naskos Date: Thu, 14 May 2020 12:12:04 +0300 Subject: [PATCH] Fixed crash when completing import with dot access --- src/analysis.zig | 53 ++++++++++++++++++++++-------------------- src/document_store.zig | 44 ++++++++++++++++++++--------------- src/main.zig | 13 ++++------- 3 files changed, 58 insertions(+), 52 deletions(-) diff --git a/src/analysis.zig b/src/analysis.zig index 3a8ffa2..e3a3341 100644 --- a/src/analysis.zig +++ b/src/analysis.zig @@ -178,23 +178,21 @@ pub fn getChild(tree: *ast.Tree, node: *ast.Node, name: []const u8) ?*ast.Node { } /// Resolves the type of a node -pub fn resolveTypeOfNode(tree: *ast.Tree, node: *ast.Node, import_ctx: *ImportContext) ?*ast.Node { - var latest_tree = import_ctx.lastTree() orelse tree; - +pub fn resolveTypeOfNode(import_ctx: *ImportContext, node: *ast.Node) ?*ast.Node { switch (node.id) { .VarDecl => { const vari = node.cast(ast.Node.VarDecl).?; - return resolveTypeOfNode(latest_tree, vari.type_node orelse vari.init_node.?, import_ctx) orelse null; + return resolveTypeOfNode(import_ctx, vari.type_node orelse vari.init_node.?) orelse null; }, .FnProto => { const func = node.cast(ast.Node.FnProto).?; switch (func.return_type) { - .Explicit, .InferErrorSet => |return_type| {return resolveTypeOfNode(latest_tree, return_type, import_ctx);} + .Explicit, .InferErrorSet => |return_type| return resolveTypeOfNode(import_ctx, return_type), } }, .Identifier => { - if (getChild(latest_tree, &latest_tree.root_node.base, latest_tree.getNodeSource(node))) |child| { - return resolveTypeOfNode(latest_tree, child, import_ctx); + if (getChild(import_ctx.tree, &import_ctx.tree.root_node.base, import_ctx.tree.getNodeSource(node))) |child| { + return resolveTypeOfNode(import_ctx, child); } else return null; }, .ContainerDecl => { @@ -202,13 +200,13 @@ pub fn resolveTypeOfNode(tree: *ast.Tree, node: *ast.Node, import_ctx: *ImportCo }, .ContainerField => { const field = node.cast(ast.Node.ContainerField).?; - return resolveTypeOfNode(latest_tree, field.type_expr.?, import_ctx); + return resolveTypeOfNode(import_ctx, field.type_expr.?); }, .SuffixOp => { const suffix_op = node.cast(ast.Node.SuffixOp).?; switch (suffix_op.op) { .Call => { - return resolveTypeOfNode(latest_tree, suffix_op.lhs.node, import_ctx); + return resolveTypeOfNode(import_ctx, suffix_op.lhs.node); }, else => {} } @@ -217,10 +215,14 @@ pub fn resolveTypeOfNode(tree: *ast.Tree, node: *ast.Node, import_ctx: *ImportCo const infix_op = node.cast(ast.Node.InfixOp).?; switch (infix_op.op) { .Period => { - var left = resolveTypeOfNode(latest_tree, infix_op.lhs, import_ctx) orelse return null; - if (nodeToString(latest_tree, infix_op.rhs)) |string| { - return getChild(latest_tree, left, string); - } else return null; + // Save the child string from this tree since the tree may switch when processing + // an import lhs. + var rhs_str = nodeToString(import_ctx.tree, infix_op.rhs) orelse return null; + // @TODO: This is hackish, pass an explicit allocator or smth + rhs_str = std.mem.dupe(import_ctx.store.allocator, u8, rhs_str) catch return null; + defer import_ctx.store.allocator.free(rhs_str); + const left = resolveTypeOfNode(import_ctx, infix_op.lhs) orelse return null; + return getChild(import_ctx.tree, left, rhs_str); }, else => {} } @@ -229,22 +231,24 @@ pub fn resolveTypeOfNode(tree: *ast.Tree, node: *ast.Node, import_ctx: *ImportCo const prefix_op = node.cast(ast.Node.PrefixOp).?; switch (prefix_op.op) { .PtrType => { - return resolveTypeOfNode(latest_tree, prefix_op.rhs, import_ctx); + return resolveTypeOfNode(import_ctx, prefix_op.rhs); }, else => {} } }, .BuiltinCall => { const builtin_call = node.cast(ast.Node.BuiltinCall).?; - if (!std.mem.eql(u8, latest_tree.tokenSlice(builtin_call.builtin_token), "@import")) return null; + if (!std.mem.eql(u8, import_ctx.tree.tokenSlice(builtin_call.builtin_token), "@import")) return null; if (builtin_call.params.len > 1) return null; const import_param = builtin_call.params.at(0).*; if (import_param.id != .StringLiteral) return null; - const import_str = latest_tree.tokenSlice(import_param.cast(ast.Node.StringLiteral).?.token); - // @TODO: Handle this error better. - return (import_ctx.onImport(import_str[1 .. import_str.len - 1]) catch unreachable); + const import_str = import_ctx.tree.tokenSlice(import_param.cast(ast.Node.StringLiteral).?.token); + return import_ctx.onImport(import_str[1 .. import_str.len - 1]) catch |err| block: { + std.debug.warn("Error {} while proessing import {}\n", .{err, import_str}); + break :block null; + }; }, else => { std.debug.warn("Type resolution case not implemented; {}\n", .{node.id}); @@ -253,11 +257,10 @@ pub fn resolveTypeOfNode(tree: *ast.Tree, node: *ast.Node, import_ctx: *ImportCo return null; } -pub fn getFieldAccessTypeNode(tree: *ast.Tree, tokenizer: *std.zig.Tokenizer, import_ctx: *ImportContext) ?*ast.Node { - var current_node = &tree.root_node.base; +pub fn getFieldAccessTypeNode(import_ctx: *ImportContext, tokenizer: *std.zig.Tokenizer) ?*ast.Node { + var current_node = &import_ctx.tree.root_node.base; while (true) { - var latest_tree = import_ctx.lastTree() orelse tree; var next = tokenizer.next(); switch (next.id) { .Eof => { @@ -266,8 +269,8 @@ pub fn getFieldAccessTypeNode(tree: *ast.Tree, tokenizer: *std.zig.Tokenizer, im .Identifier => { // var root = current_node.cast(ast.Node.Root).?; // current_node. - if (getChild(latest_tree, current_node, tokenizer.buffer[next.start..next.end])) |child| { - if (resolveTypeOfNode(latest_tree, child, import_ctx)) |node_type| { + if (getChild(import_ctx.tree, current_node, tokenizer.buffer[next.start..next.end])) |child| { + if (resolveTypeOfNode(import_ctx, child)) |node_type| { current_node = node_type; } else return null; } else return null; @@ -277,8 +280,8 @@ pub fn getFieldAccessTypeNode(tree: *ast.Tree, tokenizer: *std.zig.Tokenizer, im if (after_period.id == .Eof) { return current_node; } else if (after_period.id == .Identifier) { - if (getChild(latest_tree, current_node, tokenizer.buffer[after_period.start..after_period.end])) |child| { - if (resolveTypeOfNode(latest_tree, child, import_ctx)) |child_type| { + if (getChild(import_ctx.tree, current_node, tokenizer.buffer[after_period.start..after_period.end])) |child| { + if (resolveTypeOfNode(import_ctx, child)) |child_type| { current_node = child_type; } else return null; } else return null; diff --git a/src/document_store.zig b/src/document_store.zig index a737dc9..d0fb4bd 100644 --- a/src/document_store.zig +++ b/src/document_store.zig @@ -220,12 +220,13 @@ pub fn applyChanges(self: *DocumentStore, handle: *Handle, content_changes: std. pub const ImportContext = struct { store: *DocumentStore, handle: *Handle, - trees: std.ArrayList(*std.zig.ast.Tree), + tree: *std.zig.ast.Tree, - pub fn lastTree(self: *ImportContext) ?*std.zig.ast.Tree { - if (self.trees.items.len == 0) return null; - return self.trees.items[self.trees.items.len - 1]; - } + // @TODO RemoveMe + // pub fn lastTree(self: *ImportContext) ?*std.zig.ast.Tree { + // if (self.trees.items.len == 0) return null; + // return self.trees.items[self.trees.items.len - 1]; + // } pub fn onImport(self: *ImportContext, import_str: []const u8) !?*std.zig.ast.Node { const allocator = self.store.allocator; @@ -260,9 +261,11 @@ pub const ImportContext = struct { // If we did, set our new handle and return the parsed tree root node. if (std.mem.eql(u8, uri, final_uri)) { self.handle = self.store.getHandle(final_uri) orelse return null; + + self.tree.deinit(); if (try self.handle.saneTree(allocator)) |tree| { - try self.trees.append(tree); - return &tree.root_node.base; + self.tree = tree; + return &self.tree.root_node.base; } return null; } @@ -274,9 +277,11 @@ pub const ImportContext = struct { // If it is, increment the count, set our new handle and return the parsed tree root node. new_handle.count += 1; self.handle = new_handle; + + self.tree.deinit(); if (try self.handle.saneTree(allocator)) |tree| { - try self.trees.append(tree); - return &tree.root_node.base; + self.tree = tree; + return &self.tree.root_node.base; } return null; } @@ -311,27 +316,28 @@ pub const ImportContext = struct { self.handle = try newDocument(self.store, try std.mem.dupe(allocator, u8, final_uri), file_contents); } + // Free old tree, add new one if it exists. + // If we return null, no one should access the tree. + self.tree.deinit(); if (try self.handle.saneTree(allocator)) |tree| { - try self.trees.append(tree); - return &tree.root_node.base; + self.tree = tree; + return &self.tree.root_node.base; } return null; } pub fn deinit(self: *ImportContext) void { - for (self.trees.items) |tree| { - tree.deinit(); - } - - self.trees.deinit(); + self.tree.deinit(); } }; -pub fn importContext(self: *DocumentStore, handle: *Handle) ImportContext { - return .{ +pub fn importContext(self: *DocumentStore, handle: *Handle) !?ImportContext { + const tree = (try handle.saneTree(self.allocator)) orelse return null; + + return ImportContext{ .store = self, .handle = handle, - .trees = std.ArrayList(*std.zig.ast.Tree).init(self.allocator), + .tree = tree, }; } diff --git a/src/main.zig b/src/main.zig index 0f19a99..8f4d8a1 100644 --- a/src/main.zig +++ b/src/main.zig @@ -254,8 +254,8 @@ fn completeGlobal(id: i64, handle: DocumentStore.Handle, config: Config) !void { } fn completeFieldAccess(id: i64, handle: *DocumentStore.Handle, position: types.Position, config: Config) !void { - const tree = (try handle.saneTree(allocator)) orelse { - return try send(types.Response{ + var import_ctx = (try document_store.importContext(handle)) orelse { + return send(types.Response{ .id = .{.Integer = id}, .result = .{ .CompletionList = .{ @@ -265,7 +265,7 @@ fn completeFieldAccess(id: i64, handle: *DocumentStore.Handle, position: types.P }, }); }; - defer tree.deinit(); + defer import_ctx.deinit(); // We use a local arena allocator to deallocate all temporary data without iterating var arena = std.heap.ArenaAllocator.init(allocator); @@ -276,13 +276,10 @@ fn completeFieldAccess(id: i64, handle: *DocumentStore.Handle, position: types.P var line = try handle.document.getLine(@intCast(usize, position.line)); var tokenizer = std.zig.Tokenizer.init(line); - // @TODO Pass import ctx. - var import_ctx = document_store.importContext(handle); - defer import_ctx.deinit(); - if (analysis.getFieldAccessTypeNode(tree, &tokenizer, &import_ctx)) |node| { + if (analysis.getFieldAccessTypeNode(&import_ctx, &tokenizer)) |node| { var index: usize = 0; while (node.iterate(index)) |child_node| { - if (try nodeToCompletion(&arena.allocator, import_ctx.lastTree() orelse tree, child_node, config)) |completion| { + if (try nodeToCompletion(&arena.allocator, import_ctx.tree, child_node, config)) |completion| { try completions.append(completion); } index += 1;