Fix lsp weird behaviour on block cursors (#891) (#905)

* Fix lsp weird behaviour on block cursors (#891)

Adds lookahead option to getPositionContext.

src/Server.zig

@@ -837,12 +837,12 @@ pub fn identifierFromPosition(pos_index: usize, handle: DocumentStore.Handle) []
     if (pos_index + 1 >= handle.text.len) return "";
     var start_idx = pos_index;
 
-    while (start_idx > 0 and isSymbolChar(handle.text[start_idx - 1])) {
+    while (start_idx > 0 and analysis.isSymbolChar(handle.text[start_idx - 1])) {
         start_idx -= 1;
     }
 
     var end_idx = pos_index;
-    while (end_idx < handle.text.len and isSymbolChar(handle.text[end_idx])) {
+    while (end_idx < handle.text.len and analysis.isSymbolChar(handle.text[end_idx])) {
         end_idx += 1;
     }
 
@@ -850,10 +850,6 @@ pub fn identifierFromPosition(pos_index: usize, handle: DocumentStore.Handle) []
     return handle.text[start_idx..end_idx];
 }
 
-fn isSymbolChar(char: u8) bool {
-    return std.ascii.isAlphanumeric(char) or char == '_';
-}
-
 fn gotoDefinitionSymbol(
     server: *Server,
     decl_handle: analysis.DeclWithHandle,
@@ -2124,7 +2120,7 @@ fn completionHandler(server: *Server, request: types.CompletionParams) Error!?ty
     }
 
     const source_index = offsets.positionToIndex(handle.text, request.position, server.offset_encoding);
-    const pos_context = try analysis.getPositionContext(server.arena.allocator(), handle.text, source_index);
+    const pos_context = try analysis.getPositionContext(server.arena.allocator(), handle.text, source_index, false);
 
     const maybe_completions = switch (pos_context) {
         .builtin => try server.completeBuiltin(),
@@ -2205,7 +2201,7 @@ fn gotoHandler(server: *Server, request: types.TextDocumentPositionParams, resol
     if (request.position.character == 0) return null;
 
     const source_index = offsets.positionToIndex(handle.text, request.position, server.offset_encoding);
-    const pos_context = try analysis.getPositionContext(server.arena.allocator(), handle.text, source_index);
+    const pos_context = try analysis.getPositionContext(server.arena.allocator(), handle.text, source_index, true);
 
     return switch (pos_context) {
         .var_access => try server.gotoDefinitionGlobal(source_index, handle, resolve_alias),
@@ -2245,7 +2241,7 @@ fn hoverHandler(server: *Server, request: types.HoverParams) Error!?types.Hover
     if (request.position.character == 0) return null;
 
     const source_index = offsets.positionToIndex(handle.text, request.position, server.offset_encoding);
-    const pos_context = try analysis.getPositionContext(server.arena.allocator(), handle.text, source_index);
+    const pos_context = try analysis.getPositionContext(server.arena.allocator(), handle.text, source_index, true);
 
     const response = switch (pos_context) {
         .builtin => try server.hoverDefinitionBuiltin(source_index, handle),
@@ -2391,7 +2387,7 @@ fn generalReferencesHandler(server: *Server, request: GeneralReferencesRequest)
     if (request.position().character <= 0) return null;
 
     const source_index = offsets.positionToIndex(handle.text, request.position(), server.offset_encoding);
-    const pos_context = try analysis.getPositionContext(server.arena.allocator(), handle.text, source_index);
+    const pos_context = try analysis.getPositionContext(server.arena.allocator(), handle.text, source_index, true);
 
     const decl = switch (pos_context) {
         .var_access => try server.getSymbolGlobal(source_index, handle),

src/analysis.zig

@@ -1553,15 +1553,32 @@ fn tokenLocAppend(prev: offsets.Loc, token: std.zig.Token) offsets.Loc {
     };
 }
 
+pub fn isSymbolChar(char: u8) bool {
+    return std.ascii.isAlphanumeric(char) or char == '_';
+}
+
 /// Given a byte index in a document (typically cursor offset), classify what kind of entity is at that index.
 ///
 /// Classification is based on the lexical structure -- we fetch the line containin index, tokenize it,
 /// and look at the sequence of tokens just before the cursor. Due to the nice way zig is designed (only line
 /// comments, etc) lexing just a single line is always correct.
-pub fn getPositionContext(allocator: std.mem.Allocator, text: []const u8, doc_index: usize) !PositionContext {
+pub fn getPositionContext(
-    const line_loc = offsets.lineLocAtIndex(text, doc_index);
+    allocator: std.mem.Allocator,
+    text: []const u8,
+    doc_index: usize,
+    /// Should we look to the end of the current context? Yes for goto def, no for completions
+    lookahead: bool,
+) !PositionContext {
+    var new_index = doc_index;
+    if (lookahead and new_index < text.len and isSymbolChar(text[new_index])) {
+        new_index += 1;
+    } else if (lookahead and new_index + 1 < text.len and text[new_index] == '@') {
+        new_index += 2;
+    }
+
+    const line_loc = if (!lookahead) offsets.lineLocAtIndex(text, new_index) else offsets.lineLocUntilIndex(text, new_index);
     const line = offsets.locToSlice(text, line_loc);
-    const prev_char = if (doc_index > 0) text[doc_index - 1] else 0;
+    const prev_char = if (new_index > 0) text[new_index - 1] else 0;
 
     const is_comment = std.mem.startsWith(u8, std.mem.trimLeft(u8, line, " \t"), "//");
     if (is_comment) return .comment;
@@ -1582,8 +1599,8 @@ pub fn getPositionContext(allocator: std.mem.Allocator, text: []const u8, doc_in
         while (true) {
             const tok = tokenizer.next();
             // Early exits.
-            if (tok.loc.start > doc_index) break;
+            if (tok.loc.start > new_index) break;
-            if (tok.loc.start == doc_index) {
+            if (tok.loc.start == new_index) {
                 // Tie-breaking, the curosr is exactly between two tokens, and
                 // `tok` is the latter of the two.
                 if (tok.tag != .identifier) break;

tests/utility/position_context.zig

@@ -9,16 +9,47 @@ const allocator = std.testing.allocator;
 
 test "position context - var access" {
     try testContext(
-        \\const this_var = id<cursor>entifier;
+        \\const a_var =<cursor> identifier;
+    ,
+        .empty,
+        null,
+    );
+    try testContext(
+        \\const a_var = <cursor>identifier;
+    ,
+        .var_access,
+        "i",
+    );
+    try testContext(
+        \\const a_var = iden<cursor>tifier;
+    ,
+        .var_access,
+        "ident",
+    );
+    try testContext(
+        \\const a_var = identifier<cursor>;
     ,
         .var_access,
         "identifier",
     );
     try testContext(
-        \\const this_var = identifier<cursor>;
+        \\const a_var = identifier;<cursor>
+    ,
+        .empty,
+        null,
+    );
+
+    try testContext(
+        \\    fn foo() !<cursor>Str {
     ,
         .var_access,
-        "identifier",
+        "S",
+    );
+    try testContext(
+        \\    fn foo() !St<cursor>r {
+    ,
+        .var_access,
+        "Str",
     );
     try testContext(
         \\    fn foo() !Str<cursor> {
@@ -26,74 +57,259 @@ test "position context - var access" {
         .var_access,
         "Str",
     );
-    // TODO fix failing test!
+    try testContext(
+        \\    fn foo() !Str <cursor>{
+    ,
+        .var_access,
+        "Str",
+    );
+
+    // TODO fix failing tests
+    // try testContext(
+    //     \\    fn foo() <cursor>Err!void {
+    // ,
+    //     .var_access,
+    //     "E",
+    // );
+    // try testContext(
+    //     \\    fn foo() Er<cursor>r!void {
+    // ,
+    //     .var_access,
+    //     "Err",
+    // );
     // try testContext(
     //     \\    fn foo() Err<cursor>!void {
     // ,
     //     .var_access,
     //     "Err",
     // );
+    // try testContext(
+    //     \\    fn foo() Err!<cursor>void {
+    // ,
+    //     .var_access,
+    //     "v",
+    // );
+
+    try testContext(
+        \\if (<cursor>bar.field == foo) {
+    ,
+        .var_access,
+        "b",
+    );
+    try testContext(
+        \\if (ba<cursor>r.field == foo) {
+    ,
+        .var_access,
+        "bar",
+    );
+    try testContext(
+        \\if (bar<cursor>.field == foo) {
+    ,
+        .var_access,
+        "bar",
+    );
+
+    try testContext(
+        \\if (bar[0]<cursor>.field == foo) {
+    ,
+        .var_access,
+        "bar",
+    );
 }
 
 test "position context - field access" {
     try testContext(
-        \\if (foo.<cursor>field == foo) {
+        \\if (bar.<cursor>field == foo) {
     ,
         .field_access,
-        "foo.field",
+        "bar.f",
     );
     try testContext(
-        \\if (foo.member.<cursor>field == foo) {
+        \\if (bar.fie<cursor>ld == foo) {
     ,
         .field_access,
-        "foo.member.field",
+        "bar.fiel",
     );
     try testContext(
-        \\if (foo.*.?.<cursor>field == foo) {
+        \\if (bar.field<cursor> == foo) {
     ,
         .field_access,
-        "foo.*.?.field",
+        "bar.field",
+    );
+
+    try testContext(
+        \\if (bar.member<cursor>.field == foo) {
+    ,
+        .field_access,
+        "bar.member",
     );
     try testContext(
-        \\if (foo[0].<cursor>field == foo) {
+        \\if (bar.member.<cursor>field == foo) {
     ,
         .field_access,
-        "foo[0].field",
+        "bar.member.f",
     );
     try testContext(
-        \\if (foo.<cursor>@"field" == foo) {
+        \\if (bar.member.fie<cursor>ld == foo) {
     ,
         .field_access,
-        "foo.@\"field\"",
+        "bar.member.fiel",
+    );
+    try testContext(
+        \\if (bar.member.field<cursor> == foo) {
+    ,
+        .field_access,
+        "bar.member.field",
+    );
+
+    try testContext(
+        \\if (bar.*.?<cursor>.field == foo) {
+    ,
+        .field_access,
+        "bar.*.?",
+    );
+    try testContext(
+        \\if (bar.*.?.<cursor>field == foo) {
+    ,
+        .field_access,
+        "bar.*.?.f",
+    );
+
+    try testContext(
+        \\if (bar[0].<cursor>field == foo) {
+    ,
+        .field_access,
+        "bar[0].f",
+    );
+
+    try testContext(
+        \\if (bar.<cursor>@"field" == foo) {
+    ,
+        .field_access,
+        "bar.@\"",
+    );
+    try testContext(
+        \\if (bar.@"fie<cursor>ld" == foo) {
+    ,
+        .field_access,
+        "bar.@\"fiel",
+    );
+    try testContext(
+        \\if (bar.@"field"<cursor> == foo) {
+    ,
+        .field_access,
+        "bar.@\"field\"",
+    );
+
+    try testContext(
+        \\const arr = std.ArrayList(SomeStruct(a, b, c, d)).<cursor>init(allocator);
+    ,
+        .field_access,
+        "std.ArrayList(SomeStruct(a, b, c, d)).i",
     );
     try testContext(
         \\const arr = std.ArrayList(SomeStruct(a, b, c, d)).in<cursor>it(allocator);
     ,
         .field_access,
-        "std.ArrayList(SomeStruct(a, b, c, d)).init",
+        "std.ArrayList(SomeStruct(a, b, c, d)).ini",
     );
     try testContext(
-        \\fn foo() !Foo.b<cursor> {
+        \\const arr = std.ArrayList(SomeStruct(a, b, c, d)).init<cursor>(allocator);
+    ,
+        .field_access,
+        "std.ArrayList(SomeStruct(a, b, c, d)).init",
+    );
+
+    try testContext(
+        \\fn foo() !Foo.<cursor>bar {
     ,
         .field_access,
         "Foo.b",
     );
-    // TODO fix failing test!
+    try testContext(
+        \\fn foo() !Foo.ba<cursor>r {
+    ,
+        .field_access,
+        "Foo.bar",
+    );
+    try testContext(
+        \\fn foo() !Foo.bar<cursor> {
+    ,
+        .field_access,
+        "Foo.bar",
+    );
+
+    // TODO fix failing tests
     // try testContext(
-    //     \\fn foo() Foo.b<cursor>!void {
+    //     \\fn foo() Foo.<cursor>bar!void {
     // ,
     //     .field_access,
     //     "Foo.b",
     // );
+    // try testContext(
+    //     \\fn foo() Foo.ba<cursor>r!void {
+    // ,
+    //     .field_access,
+    //     "Foo.bar",
+    // );
+    // try testContext(
+    //     \\fn foo() Foo.bar<cursor>!void {
+    // ,
+    //     .field_access,
+    //     "Foo.bar",
+    // );
 }
 
 test "position context - builtin" {
+    try testContext(
+        \\var foo = <cursor>@
+    ,
+        .empty,
+        null,
+    );
+    try testContext(
+        \\var foo = <cursor>@intC(u32, 5);
+    ,
+        .builtin,
+        "@i",
+    );
+    try testContext(
+        \\var foo = @<cursor>intC(u32, 5);
+    ,
+        .builtin,
+        "@i",
+    );
+    try testContext(
+        \\var foo = @int<cursor>C(u32, 5);
+    ,
+        .builtin,
+        "@intC",
+    );
     try testContext(
         \\var foo = @intC<cursor>(u32, 5);
     ,
         .builtin,
         "@intC",
     );
+
+    try testContext(
+        \\fn foo() void { <cursor>@setRuntime(false); };
+    ,
+        .builtin,
+        "@s",
+    );
+    try testContext(
+        \\fn foo() void { @<cursor>setRuntime(false); };
+    ,
+        .builtin,
+        "@s",
+    );
+    try testContext(
+        \\fn foo() void { @set<cursor>Runtime(false); };
+    ,
+        .builtin,
+        "@setR",
+    );
     try testContext(
         \\fn foo() void { @setRuntime<cursor>(false); };
     ,
@@ -118,17 +334,29 @@ test "position context - comment" {
 }
 
 test "position context - import/embedfile string literal" {
+    try testContext(
+        \\const std = @import("s<cursor>t");
+    ,
+        .import_string_literal,
+        "\"st", // maybe report just "st"
+    );
     try testContext(
         \\const std = @import("st<cursor>");
     ,
         .import_string_literal,
-        "\"st\"", // maybe report just "st"
+        "\"st", // maybe report just "st"
     );
     try testContext(
         \\const std = @embedFile("file.<cursor>");
     ,
         .embedfile_string_literal,
-        "\"file.\"", // maybe report just "file."
+        "\"file.", // maybe report just "file."
+    );
+    try testContext(
+        \\const std = @embedFile("file<cursor>.");
+    ,
+        .embedfile_string_literal,
+        "\"file", // maybe report just "file."
     );
 }
 
@@ -137,29 +365,49 @@ test "position context - string literal" {
         \\var foo = "he<cursor>llo world!";
     ,
         .string_literal,
-        "\"hello world!\"", // maybe report just "hello world!"
+        "\"hel", // maybe report just "he"
     );
     try testContext(
-        \\var foo = \\hello<cursor>;
+        \\var foo = \\hell<cursor>o;
     ,
         .string_literal,
-        "\\\\hello;", // maybe report just "hello;"
+        "\\\\hello", // maybe report just "hello;"
     );
 }
 
 test "position context - global error set" {
+    // TODO why is this a .var_access instead of a .global_error_set?
+    // try testContext(
+    //     \\fn foo() <cursor>error!void {
+    // ,
+    //     .global_error_set,
+    //     null,
+    // );
+    try testContext(
+        \\fn foo() erro<cursor>r!void {
+    ,
+        .global_error_set,
+        null,
+    );
     try testContext(
         \\fn foo() error<cursor>!void {
     ,
         .global_error_set,
         null,
     );
+    try testContext(
+        \\fn foo() error<cursor>.!void {
+    ,
+        .global_error_set,
+        null,
+    );
     try testContext(
         \\fn foo() error.<cursor>!void {
     ,
         .global_error_set,
         null,
     );
+
     // TODO this should probably also be .global_error_set
     // try testContext(
     //     \\fn foo() error{<cursor>}!void {
@@ -176,12 +424,30 @@ test "position context - global error set" {
 }
 
 test "position context - enum literal" {
+    try testContext(
+        \\var foo = .<cursor>tag;
+    ,
+        .enum_literal,
+        null,
+    );
+    try testContext(
+        \\var foo = .ta<cursor>g;
+    ,
+        .enum_literal,
+        null,
+    );
     try testContext(
         \\var foo = .tag<cursor>;
     ,
         .enum_literal,
         null,
     );
+    try testContext(
+        \\var foo = <cursor>.;
+    ,
+        .empty,
+        null,
+    );
     try testContext(
         \\var foo = .<cursor>;
     ,
@@ -191,6 +457,24 @@ test "position context - enum literal" {
 }
 
 test "position context - label" {
+    try testContext(
+        \\var foo = blk: { break <cursor>:blk null };
+    ,
+        .pre_label,
+        null,
+    );
+    try testContext(
+        \\var foo = blk: { break :<cursor>blk null };
+    ,
+        .label,
+        null,
+    );
+    try testContext(
+        \\var foo = blk: { break :bl<cursor>k null };
+    ,
+        .label,
+        null,
+    );
     try testContext(
         \\var foo = blk: { break :blk<cursor> null };
     ,
@@ -206,12 +490,6 @@ test "position context - empty" {
         .empty,
         null,
     );
-    try testContext(
-        \\<cursor>const foo = struct {};
-    ,
-        .empty,
-        null,
-    );
     try testContext(
         \\try foo(arg, slice[<cursor>]);
     ,
@@ -237,7 +515,7 @@ fn testContext(line: []const u8, tag: std.meta.Tag(analysis.PositionContext), ma
     const final_line = try std.mem.concat(allocator, u8, &.{ line[0..cursor_idx], line[cursor_idx + "<cursor>".len ..] });
     defer allocator.free(final_line);
 
-    const ctx = try analysis.getPositionContext(allocator, final_line, cursor_idx);
+    const ctx = try analysis.getPositionContext(allocator, final_line, cursor_idx, true);
 
     if (std.meta.activeTag(ctx) != tag) {
         std.debug.print("Expected tag `{s}`, got `{s}`\n", .{ @tagName(tag), @tagName(std.meta.activeTag(ctx)) });