From 396f4b887e6d7fe50957dce2a7c06199a07b670e Mon Sep 17 00:00:00 2001 From: moonz Date: Tue, 27 Jan 2026 00:47:12 +0100 Subject: refactor: cleaning up the methods and config reading --- src/testing/testing.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/testing') diff --git a/src/testing/testing.odin b/src/testing/testing.odin index 373ef62..2013f79 100644 --- a/src/testing/testing.odin +++ b/src/testing/testing.odin @@ -346,7 +346,7 @@ expect_definition_locations :: proc(t: ^testing.T, src: ^Source, expect_location setup(src) defer teardown(src) - locations, ok := server.get_definition_location(src.document, src.position) + locations, ok := server.get_definition_location(src.document, src.position, &src.config) if !ok { log.error("Failed get_definition_location") -- cgit v1.2.3 From 23b5c69f52b0007a6727bd04a5fe373e97df0739 Mon Sep 17 00:00:00 2001 From: pc Date: Tue, 27 Jan 2026 02:03:56 +0100 Subject: feat: suggest proc group instead of individual procs --- src/server/collector.odin | 285 ++++++++++++++++++++++++++++++++++++-------- src/server/methods.odin | 281 +++++++++++++++++++++++++++++++------------ src/testing/testing.odin | 3 + tests/completions_test.odin | 114 ++++++++++++++++++ 4 files changed, 558 insertions(+), 125 deletions(-) (limited to 'src/testing') diff --git a/src/server/collector.odin b/src/server/collector.odin index d8a4e65..dc7e052 100644 --- a/src/server/collector.odin +++ b/src/server/collector.odin @@ -32,10 +32,11 @@ Method :: struct { } SymbolPackage :: struct { - symbols: map[string]Symbol, - objc_structs: map[string]ObjcStruct, //mapping from struct name to function - methods: map[Method][dynamic]Symbol, - imports: [dynamic]string, //Used for references to figure whether the package is even able to reference the symbol + symbols: map[string]Symbol, + objc_structs: map[string]ObjcStruct, //mapping from struct name to function + methods: map[Method][dynamic]Symbol, + imports: [dynamic]string, //Used for references to figure whether the package is even able to reference the symbol + proc_group_members: map[string]bool, // Tracks procedure names that are part of proc groups (used by fake methods) } get_index_unique_string :: proc { @@ -437,46 +438,187 @@ add_comp_lit_fields :: proc( generic.ranges = ranges[:] } +/* + Records the names of procedures that are part of a proc group. + This is used by the fake methods feature to hide individual procs + when the proc group should be shown instead. +*/ +record_proc_group_members :: proc(collection: ^SymbolCollection, group: ^ast.Proc_Group, pkg_name: string) { + pkg := get_or_create_package(collection, pkg_name) + + for arg in group.args { + name := get_proc_group_member_name(arg) or_continue + pkg.proc_group_members[get_index_unique_string(collection, name)] = true + } +} + +@(private = "file") +get_proc_group_member_name :: proc(expr: ^ast.Expr) -> (name: string, ok: bool) { + #partial switch v in expr.derived { + case ^ast.Ident: + return v.name, true + case ^ast.Selector_Expr: + // For package.proc_name, we only care about the proc name + if field, is_ident := v.field.derived.(^ast.Ident); is_ident { + return field.name, true + } + } + return "", false +} + +@(private = "file") +get_or_create_package :: proc(collection: ^SymbolCollection, pkg_name: string) -> ^SymbolPackage { + pkg := &collection.packages[pkg_name] + if pkg.symbols == nil { + collection.packages[pkg_name] = {} + pkg = &collection.packages[pkg_name] + pkg.symbols = make(map[string]Symbol, 100, collection.allocator) + pkg.methods = make(map[Method][dynamic]Symbol, 100, collection.allocator) + pkg.objc_structs = make(map[string]ObjcStruct, 5, collection.allocator) + pkg.proc_group_members = make(map[string]bool, 10, collection.allocator) + } + return pkg +} + +/* + Collects a procedure as a fake method if it's not part of a proc group. +*/ collect_method :: proc(collection: ^SymbolCollection, symbol: Symbol) { pkg := &collection.packages[symbol.pkg] - if value, ok := symbol.value.(SymbolProcedureValue); ok { - if len(value.arg_types) == 0 { - return + // Skip procedures that are part of proc groups + if symbol.name in pkg.proc_group_members { + return + } + + value, ok := symbol.value.(SymbolProcedureValue) + if !ok { + return + } + if len(value.arg_types) == 0 { + return + } + + method, method_ok := get_method_from_first_arg(collection, value.arg_types[0].type, symbol.pkg) + if !method_ok { + return + } + add_symbol_to_method(collection, pkg, method, symbol) +} + +/* + Collects a proc group as a fake method based on its member procedures' first arguments. + The proc group is registered as a method for each distinct first-argument type + across all its members. +*/ +collect_proc_group_method :: proc(collection: ^SymbolCollection, symbol: Symbol) { + pkg := &collection.packages[symbol.pkg] + + group_value, ok := symbol.value.(SymbolProcedureGroupValue) + if !ok { + return + } + + proc_group, is_proc_group := group_value.group.derived.(^ast.Proc_Group) + if !is_proc_group || len(proc_group.args) == 0 { + return + } + + // Track which method keys we've already registered to avoid duplicates + registered_methods := make(map[Method]bool, len(proc_group.args), context.temp_allocator) + + // Register the proc group as a method for each distinct first-argument type + for member_expr in proc_group.args { + member_name, name_ok := get_proc_group_member_name(member_expr) + if !name_ok { + continue } - expr, _, ok := unwrap_pointer_ident(value.arg_types[0].type) + member_symbol, found := pkg.symbols[member_name] + if !found { + continue + } - if !ok { - return + member_proc, is_proc := member_symbol.value.(SymbolProcedureValue) + if !is_proc || len(member_proc.arg_types) == 0 { + continue } - method: Method + method, method_ok := get_method_from_first_arg(collection, member_proc.arg_types[0].type, symbol.pkg) + if !method_ok { + continue + } - #partial switch v in expr.derived { - case ^ast.Selector_Expr: - if ident, ok := v.expr.derived.(^ast.Ident); ok { - method.pkg = get_index_unique_string(collection, ident.name) - method.name = get_index_unique_string(collection, v.field.name) - } else { - return - } - case ^ast.Ident: - method.pkg = symbol.pkg - method.name = get_index_unique_string(collection, v.name) - case: - return + // Only add once per distinct method key + if method not_in registered_methods { + registered_methods[method] = true + add_symbol_to_method(collection, pkg, method, symbol) } + } +} - symbols := &pkg.methods[method] +@(private = "file") +get_method_from_first_arg :: proc( + collection: ^SymbolCollection, + first_arg_type: ^ast.Expr, + default_pkg: string, +) -> ( + method: Method, + ok: bool, +) { + expr, _, unwrap_ok := unwrap_pointer_ident(first_arg_type) + if !unwrap_ok { + return {}, false + } - if symbols == nil { - pkg.methods[method] = make([dynamic]Symbol, collection.allocator) - symbols = &pkg.methods[method] + #partial switch v in expr.derived { + case ^ast.Selector_Expr: + ident, is_ident := v.expr.derived.(^ast.Ident) + if !is_ident { + return {}, false } + method.pkg = get_index_unique_string(collection, ident.name) + method.name = get_index_unique_string(collection, v.field.name) + case ^ast.Ident: + // Check if this is a builtin type + if is_builtin_type_name(v.name) { + method.pkg = "$builtin" + } else { + method.pkg = default_pkg + } + method.name = get_index_unique_string(collection, v.name) + case: + return {}, false + } - append(symbols, symbol) + return method, true +} + +is_builtin_type_name :: proc(name: string) -> bool { + // Check all builtin type names from untyped_map + for names in untyped_map { + for builtin_name in names { + if name == builtin_name { + return true + } + } + } + // Also check some other builtin types not in untyped_map + switch name { + case "rawptr", "uintptr", "typeid", "any", "rune": + return true + } + return false +} + +@(private = "file") +add_symbol_to_method :: proc(collection: ^SymbolCollection, pkg: ^SymbolPackage, method: Method, symbol: Symbol) { + symbols := &pkg.methods[method] + if symbols == nil { + pkg.methods[method] = make([dynamic]Symbol, collection.allocator) + symbols = &pkg.methods[method] } + append(symbols, symbol) } collect_objc :: proc(collection: ^SymbolCollection, attributes: []^ast.Attribute, symbol: Symbol) { @@ -554,6 +696,20 @@ collect_symbols :: proc(collection: ^SymbolCollection, file: ast.File, uri: stri } } + // Compute pkg early so it's available inside the switch + if expr.builtin || strings.contains(uri, "builtin.odin") { + symbol.pkg = "$builtin" + } else if strings.contains(uri, "intrinsics.odin") { + intrinsics_path := filepath.join( + elems = {common.config.collections["base"], "/intrinsics"}, + allocator = context.temp_allocator, + ) + intrinsics_path, _ = filepath.to_slash(intrinsics_path, context.temp_allocator) + symbol.pkg = get_index_unique_string(collection, intrinsics_path) + } else { + symbol.pkg = get_index_unique_string(collection, directory) + } + #partial switch v in col_expr.derived { case ^ast.Matrix_Type: token = v^ @@ -601,6 +757,10 @@ collect_symbols :: proc(collection: ^SymbolCollection, file: ast.File, uri: stri symbol.value = SymbolProcedureGroupValue { group = clone_type(col_expr, collection.allocator, &collection.unique_strings), } + // Record proc group members for fake methods feature + if collection.config != nil && collection.config.enable_fake_method { + record_proc_group_members(collection, v, symbol.pkg) + } case ^ast.Struct_Type: token = v^ token_type = .Struct @@ -712,20 +872,7 @@ collect_symbols :: proc(collection: ^SymbolCollection, file: ast.File, uri: stri comment, _ := get_file_comment(file, symbol.range.start.line + 1) symbol.comment = get_comment(comment, collection.allocator) - if expr.builtin || strings.contains(uri, "builtin.odin") { - symbol.pkg = "$builtin" - } else if strings.contains(uri, "intrinsics.odin") { - path := filepath.join( - elems = {common.config.collections["base"], "/intrinsics"}, - allocator = context.temp_allocator, - ) - - path, _ = filepath.to_slash(path, context.temp_allocator) - - symbol.pkg = get_index_unique_string(collection, path) - } else { - symbol.pkg = get_index_unique_string(collection, directory) - } + // symbol.pkg was already set earlier before the switch if is_distinct { symbol.flags |= {.Distinct} @@ -764,16 +911,13 @@ collect_symbols :: proc(collection: ^SymbolCollection, file: ast.File, uri: stri pkg.symbols = make(map[string]Symbol, 100, collection.allocator) pkg.methods = make(map[Method][dynamic]Symbol, 100, collection.allocator) pkg.objc_structs = make(map[string]ObjcStruct, 5, collection.allocator) + pkg.proc_group_members = make(map[string]bool, 10, collection.allocator) } if .ObjC in symbol.flags { collect_objc(collection, expr.attributes, symbol) } - if symbol.type == .Function && common.config.enable_fake_method { - collect_method(collection, symbol) - } - if v, ok := pkg.symbols[symbol.name]; !ok || v.name == "" { pkg.symbols[symbol.name] = symbol } else { @@ -781,12 +925,59 @@ collect_symbols :: proc(collection: ^SymbolCollection, file: ast.File, uri: stri } } + // Second pass: collect fake methods after all symbols and proc group members are recorded + if collection.config != nil && collection.config.enable_fake_method { + collect_fake_methods(collection, exprs, directory, uri) + } + collect_imports(collection, file, directory) return .None } +/* + Collects fake methods for all procedures and proc groups. + This is done as a second pass after all symbols are collected, + so that we know which procedures are part of proc groups. +*/ +@(private = "file") +collect_fake_methods :: proc(collection: ^SymbolCollection, exprs: []GlobalExpr, directory: string, uri: string) { + for expr in exprs { + // Determine the package name (same logic as in collect_symbols) + pkg_name: string + if expr.builtin || strings.contains(uri, "builtin.odin") { + pkg_name = "$builtin" + } else if strings.contains(uri, "intrinsics.odin") { + intrinsics_path := filepath.join( + elems = {common.config.collections["base"], "/intrinsics"}, + allocator = context.temp_allocator, + ) + intrinsics_path, _ = filepath.to_slash(intrinsics_path, context.temp_allocator) + pkg_name = get_index_unique_string(collection, intrinsics_path) + } else { + pkg_name = get_index_unique_string(collection, directory) + } + + pkg, ok := &collection.packages[pkg_name] + if !ok { + continue + } + + symbol, found := pkg.symbols[expr.name] + if !found { + continue + } + + #partial switch _ in symbol.value { + case SymbolProcedureValue: + collect_method(collection, symbol) + case SymbolProcedureGroupValue: + collect_proc_group_method(collection, symbol) + } + } +} + Reference :: struct { identifiers: [dynamic]common.Location, selectors: map[string][dynamic]common.Range, diff --git a/src/server/methods.odin b/src/server/methods.odin index 757b37d..797a089 100644 --- a/src/server/methods.odin +++ b/src/server/methods.odin @@ -73,7 +73,7 @@ append_method_completion :: proc( for c in cases { method := Method { name = c, - pkg = selector_symbol.pkg, + pkg = "$builtin", // Untyped values are always builtin types } collect_methods( ast_context, @@ -86,9 +86,14 @@ append_method_completion :: proc( ) } } else { + // For typed values, check if it's a builtin type + method_pkg := selector_symbol.pkg + if is_builtin_type_name(selector_symbol.name) { + method_pkg = "$builtin" + } method := Method { name = selector_symbol.name, - pkg = selector_symbol.pkg, + pkg = method_pkg, } collect_methods( ast_context, @@ -114,83 +119,203 @@ collect_methods :: proc( results: ^[dynamic]CompletionResult, ) { for k, v in indexer.index.collection.packages { - if symbols, ok := &v.methods[method]; ok { - for &symbol in symbols { - if should_skip_private_symbol(symbol, ast_context.current_package, ast_context.fullpath) { - continue - } - resolve_unresolved_symbol(ast_context, &symbol) - - range, ok := get_range_from_selection_start_to_dot(position_context) - - if !ok { - return - } - - value: SymbolProcedureValue - value, ok = symbol.value.(SymbolProcedureValue) - - if !ok { - continue - } - - if len(value.arg_types) == 0 || value.arg_types[0].type == nil { - continue - } - - first_arg: Symbol - first_arg, ok = resolve_type_expression(ast_context, value.arg_types[0].type) - - if !ok { - continue - } - - pointers_to_add := first_arg.pointers - pointers - - references := "" - dereferences := "" - - if pointers_to_add > 0 { - for i in 0 ..< pointers_to_add { - references = fmt.tprintf("%v&", references) - } - } else if pointers_to_add < 0 { - for i in pointers_to_add ..< 0 { - dereferences = fmt.tprintf("%v^", dereferences) - } - } - - new_text := "" - - if symbol.pkg != ast_context.document_package { - new_text = fmt.tprintf( - "%v.%v", - path.base(get_symbol_pkg_name(ast_context, &symbol), false, ast_context.allocator), - symbol.name, - ) - } else { - new_text = fmt.tprintf("%v", symbol.name) - } - - if len(symbol.value.(SymbolProcedureValue).arg_types) > 1 { - new_text = fmt.tprintf("%v(%v%v%v$0)", new_text, references, receiver, dereferences) - } else { - new_text = fmt.tprintf("%v(%v%v%v)$0", new_text, references, receiver, dereferences) - } - - item := CompletionItem { - label = symbol.name, - kind = symbol_type_to_completion_kind(symbol.type), - detail = get_short_signature(ast_context, symbol), - additionalTextEdits = remove_edit, - textEdit = TextEdit{newText = new_text, range = {start = range.end, end = range.end}}, - insertTextFormat = .Snippet, - InsertTextMode = .adjustIndentation, - documentation = construct_symbol_docs(symbol), - } - - append(results, CompletionResult{completion_item = item}) + symbols, ok := &v.methods[method] + if !ok { + continue + } + + for &symbol in symbols { + if should_skip_private_symbol(symbol, ast_context.current_package, ast_context.fullpath) { + continue + } + resolve_unresolved_symbol(ast_context, &symbol) + + #partial switch &sym_value in symbol.value { + case SymbolProcedureValue: + add_proc_method_completion( + ast_context, + position_context, + &symbol, + sym_value, + pointers, + receiver, + remove_edit, + results, + ) + case SymbolProcedureGroupValue: + add_proc_group_method_completion( + ast_context, + position_context, + &symbol, + sym_value, + pointers, + receiver, + remove_edit, + results, + ) } } } } + +@(private = "file") +add_proc_method_completion :: proc( + ast_context: ^AstContext, + position_context: ^DocumentPositionContext, + symbol: ^Symbol, + value: SymbolProcedureValue, + pointers: int, + receiver: string, + remove_edit: []TextEdit, + results: ^[dynamic]CompletionResult, +) { + if len(value.arg_types) == 0 || value.arg_types[0].type == nil { + return + } + + range, ok := get_range_from_selection_start_to_dot(position_context) + if !ok { + return + } + + first_arg: Symbol + first_arg, ok = resolve_type_expression(ast_context, value.arg_types[0].type) + if !ok { + return + } + + references, dereferences := compute_pointer_adjustments(first_arg.pointers, pointers) + + new_text := build_method_call_text( + ast_context, + symbol, + receiver, + references, + dereferences, + len(value.arg_types) > 1, + ) + + item := CompletionItem { + label = symbol.name, + kind = symbol_type_to_completion_kind(symbol.type), + detail = get_short_signature(ast_context, symbol^), + additionalTextEdits = remove_edit, + textEdit = TextEdit{newText = new_text, range = {start = range.end, end = range.end}}, + insertTextFormat = .Snippet, + InsertTextMode = .adjustIndentation, + documentation = construct_symbol_docs(symbol^), + } + + append(results, CompletionResult{completion_item = item}) +} + +@(private = "file") +add_proc_group_method_completion :: proc( + ast_context: ^AstContext, + position_context: ^DocumentPositionContext, + symbol: ^Symbol, + value: SymbolProcedureGroupValue, + pointers: int, + receiver: string, + remove_edit: []TextEdit, + results: ^[dynamic]CompletionResult, +) { + proc_group, is_group := value.group.derived.(^ast.Proc_Group) + if !is_group || len(proc_group.args) == 0 { + return + } + + range, ok := get_range_from_selection_start_to_dot(position_context) + if !ok { + return + } + + // Get first member to determine pointer adjustments + first_member: Symbol + first_member, ok = resolve_type_expression(ast_context, proc_group.args[0]) + if !ok { + return + } + + member_proc, is_proc := first_member.value.(SymbolProcedureValue) + if !is_proc || len(member_proc.arg_types) == 0 || member_proc.arg_types[0].type == nil { + return + } + + first_arg: Symbol + first_arg, ok = resolve_type_expression(ast_context, member_proc.arg_types[0].type) + if !ok { + return + } + + references, dereferences := compute_pointer_adjustments(first_arg.pointers, pointers) + + // Proc groups always have multiple args (the receiver plus at least one overload's additional args) + new_text := build_method_call_text(ast_context, symbol, receiver, references, dereferences, true) + + item := CompletionItem { + label = symbol.name, + kind = symbol_type_to_completion_kind(symbol.type), + detail = get_short_signature(ast_context, symbol^), + additionalTextEdits = remove_edit, + textEdit = TextEdit{newText = new_text, range = {start = range.end, end = range.end}}, + insertTextFormat = .Snippet, + InsertTextMode = .adjustIndentation, + documentation = construct_symbol_docs(symbol^), + } + + append(results, CompletionResult{completion_item = item}) +} + +@(private = "file") +compute_pointer_adjustments :: proc( + first_arg_pointers: int, + current_pointers: int, +) -> ( + references: string, + dereferences: string, +) { + pointers_to_add := first_arg_pointers - current_pointers + + if pointers_to_add > 0 { + for _ in 0 ..< pointers_to_add { + references = fmt.tprintf("%v&", references) + } + } else if pointers_to_add < 0 { + for _ in pointers_to_add ..< 0 { + dereferences = fmt.tprintf("%v^", dereferences) + } + } + + return references, dereferences +} + +@(private = "file") +build_method_call_text :: proc( + ast_context: ^AstContext, + symbol: ^Symbol, + receiver: string, + references: string, + dereferences: string, + has_additional_args: bool, +) -> string { + new_text: string + + if symbol.pkg != ast_context.document_package { + new_text = fmt.tprintf( + "%v.%v", + path.base(get_symbol_pkg_name(ast_context, symbol), false, ast_context.allocator), + symbol.name, + ) + } else { + new_text = fmt.tprintf("%v", symbol.name) + } + + if has_additional_args { + new_text = fmt.tprintf("%v(%v%v%v$0)", new_text, references, receiver, dereferences) + } else { + new_text = fmt.tprintf("%v(%v%v%v)$0", new_text, references, receiver, dereferences) + } + + return new_text +} diff --git a/src/testing/testing.odin b/src/testing/testing.odin index 2013f79..20327a7 100644 --- a/src/testing/testing.odin +++ b/src/testing/testing.odin @@ -68,6 +68,9 @@ setup :: proc(src: ^Source) { server.setup_index() + // Set the collection's config to the test's config to enable feature flags like enable_fake_method + server.indexer.index.collection.config = &src.config + server.document_setup(src.document) server.document_refresh(src.document, &src.config, nil) diff --git a/tests/completions_test.odin b/tests/completions_test.odin index 19f5363..8ed9b43 100644 --- a/tests/completions_test.odin +++ b/tests/completions_test.odin @@ -5327,3 +5327,117 @@ ast_completion_parapoly_struct_with_parapoly_child :: proc(t: ^testing.T) { } test.expect_completion_docs(t, &source, "", {"ChildStruct.GenericParam: test.SomeEnum", "ChildStruct.Something: string"}) } + +@(test) +ast_completion_fake_method_simple :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + import "methods" + main :: proc() { + n: int + n.{*} + } + `, + packages = { + { + pkg = "methods", + source = `package methods + double :: proc(x: int) -> int { return x * 2 } + `, + }, + }, + config = {enable_fake_method = true}, + } + // Should show 'double' as a fake method for int + test.expect_completion_labels(t, &source, ".", {"double"}) +} + +@(test) +ast_completion_fake_method_proc_group :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + import "methods" + main :: proc() { + n: int + n.{*} + } + `, + packages = { + { + pkg = "methods", + source = `package methods + add_int :: proc(a, b: int) -> int { return a + b } + add_something :: proc(a: int, b: string) {} + add_float :: proc(a, b: f32) -> f32 { return a + b } + add :: proc { add_float, add_int, add_something } + `, + }, + }, + config = {enable_fake_method = true}, + } + // Should show 'add' (the proc group), not 'add_int' or 'add_something' (individual procs) + test.expect_completion_labels(t, &source, ".", {"add"}, {"add_int", "add_something"}) +} + +@(test) +ast_completion_fake_method_proc_group_only_shows_group :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + import "methods" + main :: proc() { + s: methods.My_Struct + s.{*} + } + `, + packages = { + { + pkg = "methods", + source = `package methods + My_Struct :: struct { x: int } + + do_thing_int :: proc(s: My_Struct, v: int) {} + do_thing_str :: proc(s: My_Struct, v: string) {} + do_thing :: proc { do_thing_int, do_thing_str } + + // standalone proc not in a group + standalone_method :: proc(s: My_Struct) {} + `, + }, + }, + config = {enable_fake_method = true}, + } + // Should show 'do_thing' (group) and 'standalone_method', but NOT 'do_thing_int' or 'do_thing_str' + test.expect_completion_labels(t, &source, ".", {"do_thing", "standalone_method"}, {"do_thing_int", "do_thing_str"}) +} + +@(test) +ast_completion_fake_method_builtin_type_uses_builtin_pkg :: proc(t: ^testing.T) { + // This test verifies that fake methods for builtin types (int, f32, string, etc.) + // are correctly looked up using "$builtin" as the package, not the package where + // the variable is declared. Without this fix, the method lookup would fail because: + // - Storage: method stored with key {pkg = "$builtin", name = "int"} + // - Lookup (wrong): would use {pkg = "test", name = "int"} based on variable's declaring package + // - Lookup (correct): uses {pkg = "$builtin", name = "int"} for builtin types + source := test.Source { + main = `package test + import "math_utils" + main :: proc() { + x: f32 + x.{*} + } + `, + packages = { + { + pkg = "math_utils", + source = `package math_utils + square :: proc(v: f32) -> f32 { return v * v } + cube :: proc(v: f32) -> f32 { return v * v * v } + `, + }, + }, + config = {enable_fake_method = true}, + } + // Both methods should appear as fake methods for f32, proving that + // the lookup correctly uses "$builtin" instead of "test" for the package + test.expect_completion_labels(t, &source, ".", {"square", "cube"}) +} -- cgit v1.2.3 From 219d0157cf409f23751b719080aa212cc1ebc1f5 Mon Sep 17 00:00:00 2001 From: pc Date: Tue, 27 Jan 2026 02:13:42 +0100 Subject: feat: add completion edit text test for proc group with single argument --- src/server/methods.odin | 19 +++++++++++++++++-- src/testing/testing.odin | 44 ++++++++++++++++++++++++++++++++++++++++++++ tests/completions_test.odin | 27 +++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) (limited to 'src/testing') diff --git a/src/server/methods.odin b/src/server/methods.odin index 797a089..19d9ff4 100644 --- a/src/server/methods.odin +++ b/src/server/methods.odin @@ -250,8 +250,23 @@ add_proc_group_method_completion :: proc( references, dereferences := compute_pointer_adjustments(first_arg.pointers, pointers) - // Proc groups always have multiple args (the receiver plus at least one overload's additional args) - new_text := build_method_call_text(ast_context, symbol, receiver, references, dereferences, true) + // Check if any member of the proc group has additional arguments beyond the receiver + has_additional_args := false + for member_expr in proc_group.args { + member: Symbol + member, ok = resolve_type_expression(ast_context, member_expr) + if !ok { + continue + } + if proc_val, is_proc_val := member.value.(SymbolProcedureValue); is_proc_val { + if len(proc_val.arg_types) > 1 { + has_additional_args = true + break + } + } + } + + new_text := build_method_call_text(ast_context, symbol, receiver, references, dereferences, has_additional_args) item := CompletionItem { label = symbol.name, diff --git a/src/testing/testing.odin b/src/testing/testing.odin index 20327a7..0ba7b0f 100644 --- a/src/testing/testing.odin +++ b/src/testing/testing.odin @@ -321,6 +321,50 @@ expect_completion_insert_text :: proc( } } +expect_completion_edit_text :: proc( + t: ^testing.T, + src: ^Source, + trigger_character: string, + label: string, + expected_text: string, +) { + setup(src) + defer teardown(src) + + completion_context := server.CompletionContext { + triggerCharacter = trigger_character, + } + + completion_list, ok := server.get_completion_list(src.document, src.position, completion_context, &src.config) + + if !ok { + log.error("Failed get_completion_list") + } + + found := false + for completion in completion_list.items { + if completion.label == label { + found = true + if text_edit, has_edit := completion.textEdit.(server.TextEdit); has_edit { + if text_edit.newText != expected_text { + log.errorf( + "Completion '%v' expected textEdit.newText %q, but received %q", + label, + expected_text, + text_edit.newText, + ) + } + } else { + log.errorf("Completion '%v' has no textEdit", label) + } + break + } + } + if !found { + log.errorf("Expected completion label '%v' not found in %v", label, completion_list.items) + } +} + expect_hover :: proc(t: ^testing.T, src: ^Source, expect_hover_string: string) { setup(src) defer teardown(src) diff --git a/tests/completions_test.odin b/tests/completions_test.odin index 8ed9b43..f0fa8c1 100644 --- a/tests/completions_test.odin +++ b/tests/completions_test.odin @@ -5441,3 +5441,30 @@ ast_completion_fake_method_builtin_type_uses_builtin_pkg :: proc(t: ^testing.T) // the lookup correctly uses "$builtin" instead of "test" for the package test.expect_completion_labels(t, &source, ".", {"square", "cube"}) } + +@(test) +ast_completion_fake_method_proc_group_single_arg_cursor_position :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + import "methods" + main :: proc() { + n: int + n.{*} + } + `, + packages = { + { + pkg = "methods", + source = `package methods + // All members only take a single argument (the receiver) + negate_a :: proc(x: int) -> int { return -x } + negate_b :: proc(x: int) -> int { return 0 - x } + negate :: proc { negate_a, negate_b } + `, + }, + }, + config = {enable_fake_method = true}, + } + // The proc group 'negate' should have cursor AFTER parentheses since no additional args + test.expect_completion_edit_text(t, &source, ".", "negate", "methods.negate(n)$0") +} -- cgit v1.2.3 From 8e5580815af631401dfa46fbf2997e44128681a6 Mon Sep 17 00:00:00 2001 From: pc Date: Tue, 27 Jan 2026 03:50:56 +0100 Subject: feat: add invert if action --- src/server/action.odin | 2 + src/server/action_invert_if_statements.odin | 389 ++++++++++++++++++++++++++++ src/testing/testing.odin | 38 +++ tests/action_test.odin | 380 +++++++++++++++++++++++++++ 4 files changed, 809 insertions(+) create mode 100644 src/server/action_invert_if_statements.odin create mode 100644 tests/action_test.odin (limited to 'src/testing') diff --git a/src/server/action.odin b/src/server/action.odin index d1608b5..cdf06cb 100644 --- a/src/server/action.odin +++ b/src/server/action.odin @@ -71,6 +71,8 @@ get_code_actions :: proc(document: ^Document, range: common.Range, config: ^comm remove_unused_imports(document, strings.clone(document.uri.uri), config, &actions) } + add_invert_if_action(document, position_context.position, strings.clone(document.uri.uri), &actions) + return actions[:], true } diff --git a/src/server/action_invert_if_statements.odin b/src/server/action_invert_if_statements.odin new file mode 100644 index 0000000..ab0c664 --- /dev/null +++ b/src/server/action_invert_if_statements.odin @@ -0,0 +1,389 @@ +#+private file + +package server + +import "core:fmt" +import "core:log" +import "core:odin/ast" +import "core:odin/tokenizer" +import path "core:path/slashpath" +import "core:strings" + +import "src:common" + +/* + * The general idea behind inverting if statements is to allow + * if statements to be inverted without changing their behavior. + * The examples of these changes are provided in the tests. + * We should be careful to only allow this code action when it is safe to do so. + * So for now, we only support only one level of if statements without else-if chains. + */ + +@(private="package") +add_invert_if_action :: proc( + document: ^Document, + position: common.AbsolutePosition, + uri: string, + actions: ^[dynamic]CodeAction, +) { + if_stmt := find_if_stmt_at_position(document.ast.decls[:], position) + if if_stmt == nil { + return + } + + new_text, ok := generate_inverted_if(document, if_stmt) + if !ok { + return + } + + range := common.get_token_range(if_stmt^, document.ast.src) + + textEdits := make([dynamic]TextEdit, context.temp_allocator) + append(&textEdits, TextEdit{range = range, newText = new_text}) + + workspaceEdit: WorkspaceEdit + workspaceEdit.changes = make(map[string][]TextEdit, 0, context.temp_allocator) + workspaceEdit.changes[uri] = textEdits[:] + + append( + actions, + CodeAction { + kind = "refactor.more", + isPreferred = false, + title = "Invert if", + edit = workspaceEdit, + }, + ) +} + +// Find the innermost if statement that contains the given position +// This will NOT return else-if statements, only top-level if statements +// Also will not return an if statement if the position is in its else clause +find_if_stmt_at_position :: proc(stmts: []^ast.Stmt, position: common.AbsolutePosition) -> ^ast.If_Stmt { + for stmt in stmts { + if stmt == nil { + continue + } + if result := find_if_stmt_in_node(stmt, position, false); result != nil { + return result + } + } + return nil +} + +find_if_stmt_in_node :: proc(node: ^ast.Node, position: common.AbsolutePosition, in_else_clause: bool) -> ^ast.If_Stmt { + if node == nil { + return nil + } + + if !(node.pos.offset <= position && position <= node.end.offset) { + return nil + } + + #partial switch n in node.derived { + case ^ast.If_Stmt: + // First check if position is in the else clause + if n.else_stmt != nil && position_in_node(n.else_stmt, position) { + // Position is in the else clause - look for nested ifs inside it + // but mark that we're in an else clause + if nested := find_if_stmt_in_node(n.else_stmt, position, true); nested != nil { + return nested + } + // Position is in else clause but not on a valid nested if + // Don't return the current if statement + return nil + } + + if n.body != nil && position_in_node(n.body, position) { + if nested := find_if_stmt_in_node(n.body, position, false); nested != nil { + return nested + } + } + + // Position is in the condition/init part or we're the closest if + // Only return this if statement if we're NOT in an else clause + // (i.e., this is not an else-if) + if !in_else_clause { + return n + } + return nil + + case ^ast.Block_Stmt: + for stmt in n.stmts { + if result := find_if_stmt_in_node(stmt, position, false); result != nil { + return result + } + } + + case ^ast.Proc_Lit: + if n.body != nil { + return find_if_stmt_in_node(n.body, position, false) + } + + case ^ast.Value_Decl: + for value in n.values { + if result := find_if_stmt_in_node(value, position, false); result != nil { + return result + } + } + + case ^ast.For_Stmt: + if n.body != nil { + return find_if_stmt_in_node(n.body, position, false) + } + + case ^ast.Range_Stmt: + if n.body != nil { + return find_if_stmt_in_node(n.body, position, false) + } + + case ^ast.Switch_Stmt: + if n.body != nil { + return find_if_stmt_in_node(n.body, position, false) + } + + case ^ast.Type_Switch_Stmt: + if n.body != nil { + return find_if_stmt_in_node(n.body, position, false) + } + + case ^ast.Case_Clause: + for stmt in n.body { + if result := find_if_stmt_in_node(stmt, position, false); result != nil { + return result + } + } + + case ^ast.When_Stmt: + if n.body != nil { + if result := find_if_stmt_in_node(n.body, position, false); result != nil { + return result + } + } + if n.else_stmt != nil { + if result := find_if_stmt_in_node(n.else_stmt, position, false); result != nil { + return result + } + } + + case ^ast.Defer_Stmt: + if n.stmt != nil { + return find_if_stmt_in_node(n.stmt, position, false) + } + } + + return nil +} + +// Generate the inverted if statement text +generate_inverted_if :: proc(document: ^Document, if_stmt: ^ast.If_Stmt) -> (string, bool) { + src := document.ast.src + + indent := get_line_indentation(src, if_stmt.pos.offset) + + sb := strings.builder_make(context.temp_allocator) + + if if_stmt.label != nil { + label_text := src[if_stmt.label.pos.offset:if_stmt.label.end.offset] + strings.write_string(&sb, label_text) + strings.write_string(&sb, ": ") + } + + strings.write_string(&sb, "if ") + + if if_stmt.init != nil { + init_text := src[if_stmt.init.pos.offset:if_stmt.init.end.offset] + strings.write_string(&sb, init_text) + strings.write_string(&sb, "; ") + } + + if if_stmt.cond != nil { + inverted_cond, ok := invert_condition(src, if_stmt.cond) + if !ok { + return "", false + } + strings.write_string(&sb, inverted_cond) + } + + strings.write_string(&sb, " ") + + // Now we need to swap the bodies + + if if_stmt.else_stmt != nil { + else_body_text := get_block_body_text(src, if_stmt.else_stmt, indent) + then_body_text := get_block_body_text(src, if_stmt.body, indent) + + strings.write_string(&sb, "{\n") + strings.write_string(&sb, else_body_text) + strings.write_string(&sb, indent) + strings.write_string(&sb, "} else {\n") + strings.write_string(&sb, then_body_text) + strings.write_string(&sb, indent) + strings.write_string(&sb, "}") + } else { + then_body_text := get_block_body_text(src, if_stmt.body, indent) + + strings.write_string(&sb, "{\n") + strings.write_string(&sb, indent) + strings.write_string(&sb, "} else {\n") + strings.write_string(&sb, then_body_text) + strings.write_string(&sb, indent) + strings.write_string(&sb, "}") + } + + return strings.to_string(sb), true +} + +// Get the indentation (leading whitespace) of the line containing the given offset +get_line_indentation :: proc(src: string, offset: int) -> string { + line_start := offset + for line_start > 0 && src[line_start - 1] != '\n' { + line_start -= 1 + } + + indent_end := line_start + for indent_end < len(src) && (src[indent_end] == ' ' || src[indent_end] == '\t') { + indent_end += 1 + } + + return src[line_start:indent_end] +} + +// Extract the body text from a block statement (without the braces) +get_block_body_text :: proc(src: string, stmt: ^ast.Stmt, base_indent: string) -> string { + if stmt == nil { + return "" + } + + #partial switch block in stmt.derived { + case ^ast.Block_Stmt: + if len(block.stmts) == 0 { + return "" + } + + sb := strings.builder_make(context.temp_allocator) + + for s in block.stmts { + if s == nil { + continue + } + stmt_indent := get_line_indentation(src, s.pos.offset) + stmt_text := src[s.pos.offset:s.end.offset] + strings.write_string(&sb, stmt_indent) + strings.write_string(&sb, stmt_text) + strings.write_string(&sb, "\n") + } + + return strings.to_string(sb) + + case ^ast.If_Stmt: + // This is an else-if, need to handle it recursively + if_text, ok := generate_inverted_if_for_else(src, block, base_indent) + if ok { + return if_text + } + } + + // Fallback: just return the statement text + stmt_text := src[stmt.pos.offset:stmt.end.offset] + return fmt.tprintf("%s%s\n", base_indent, stmt_text) +} + +// For else-if chains, we don't invert them, just preserve +generate_inverted_if_for_else :: proc(src: string, if_stmt: ^ast.If_Stmt, base_indent: string) -> (string, bool) { + stmt_indent := get_line_indentation(src, if_stmt.pos.offset) + stmt_text := src[if_stmt.pos.offset:if_stmt.end.offset] + return fmt.tprintf("%s%s\n", stmt_indent, stmt_text), true +} + +// Invert a condition expression +invert_condition :: proc(src: string, cond: ^ast.Expr) -> (string, bool) { + if cond == nil { + return "", false + } + + #partial switch c in cond.derived { + case ^ast.Binary_Expr: + inverted_op, can_invert := get_inverted_operator(c.op.kind) + if can_invert { + left_text := src[c.left.pos.offset:c.left.end.offset] + right_text := src[c.right.pos.offset:c.right.end.offset] + return fmt.tprintf("%s %s %s", left_text, inverted_op, right_text), true + } + + if c.op.kind == .Cmp_And || c.op.kind == .Cmp_Or { + // Just wrap with !() + cond_text := src[cond.pos.offset:cond.end.offset] + return fmt.tprintf("!(%s)", cond_text), true + } + + case ^ast.Unary_Expr: + // If it's already negated with !, remove the negation + if c.op.kind == .Not { + inner_text := src[c.expr.pos.offset:c.expr.end.offset] + return inner_text, true + } + + case ^ast.Paren_Expr: + inner_inverted, ok := invert_condition(src, c.expr) + if ok { + if needs_parentheses(inner_inverted) { + return fmt.tprintf("(%s)", inner_inverted), true + } + return inner_inverted, true + } + } + + // Default: wrap the whole condition with !() + cond_text := src[cond.pos.offset:cond.end.offset] + if is_simple_expr(cond) { + return fmt.tprintf("!%s", cond_text), true + } + return fmt.tprintf("!(%s)", cond_text), true +} + +// Check if an expression is simple (identifier, call, or already parenthesized) +is_simple_expr :: proc(expr: ^ast.Expr) -> bool { + if expr == nil { + return false + } + #partial switch e in expr.derived { + case ^ast.Ident: + return true + case ^ast.Paren_Expr: + return true + case ^ast.Call_Expr: + return true + case ^ast.Selector_Expr: + return true + case ^ast.Index_Expr: + return true + } + return false +} + +// Check if a string needs parentheses (simple heuristic) +needs_parentheses :: proc(s: string) -> bool { + // If it starts with ! and is not wrapped in parens, it might need them + // This is a simple heuristic + return strings.contains(s, " && ") || strings.contains(s, " || ") +} + +// Get the inverted comparison operator +get_inverted_operator :: proc(op: tokenizer.Token_Kind) -> (string, bool) { + #partial switch op { + case .Cmp_Eq: + return "!=", true + case .Not_Eq: + return "==", true + case .Lt: + return ">=", true + case .Lt_Eq: + return ">", true + case .Gt: + return "<=", true + case .Gt_Eq: + return "<", true + } + return "", false +} diff --git a/src/testing/testing.odin b/src/testing/testing.odin index 0ba7b0f..5e927a7 100644 --- a/src/testing/testing.odin +++ b/src/testing/testing.odin @@ -553,6 +553,44 @@ expect_action :: proc(t: ^testing.T, src: ^Source, expect_action_names: []string } } +expect_action_with_edit :: proc(t: ^testing.T, src: ^Source, action_name: string, expected_new_text: string) { + setup(src) + defer teardown(src) + + input_range := common.Range { + start = src.position, + end = src.position, + } + actions, ok := server.get_code_actions(src.document, input_range, &src.config) + if !ok { + log.error("Failed to find actions") + return + } + + for action in actions { + if action.title == action_name { + // Get the text edit for the document + if edits, found := action.edit.changes[src.document.uri.uri]; found { + if len(edits) > 0 { + actual_text := edits[0].newText + testing.expectf( + t, + actual_text == expected_new_text, + "\nExpected edit text:\n%s\n\nGot:\n%s", + expected_new_text, + actual_text, + ) + return + } + } + log.errorf("Action '%s' found but has no edits", action_name) + return + } + } + + log.errorf("Action '%s' not found in actions: %v", action_name, actions) +} + expect_semantic_tokens :: proc(t: ^testing.T, src: ^Source, expected: []server.SemanticToken) { setup(src) defer teardown(src) diff --git a/tests/action_test.odin b/tests/action_test.odin new file mode 100644 index 0000000..f5c57a5 --- /dev/null +++ b/tests/action_test.odin @@ -0,0 +1,380 @@ +package tests + +import "core:testing" + +import test "src:testing" + +@(test) +action_invert_if_simple :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x := 5 + if x{*} >= 0 { + foo() + } +} +`, + packages = {}, + } + + test.expect_action(t, &source, {"Invert if"}) +} + +@(test) +action_invert_if_simple_edit :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x := 5 + if x{*} >= 0 { + foo() + } +} +`, + packages = {}, + } + + expected := `if x < 0 { + } else { + foo() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_with_else :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x := 5 + if x{*} == 0 { + foo() + } else { + bar() + } +} +`, + packages = {}, + } + + test.expect_action(t, &source, {"Invert if"}) +} + +@(test) +action_invert_if_with_else_edit :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x := 5 + if x{*} == 0 { + foo() + } else { + bar() + } +} +`, + packages = {}, + } + + expected := `if x != 0 { + bar() + } else { + foo() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_with_init :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + if x{*} := foo(); x < 0 { + bar() + } +} +`, + packages = {}, + } + + test.expect_action(t, &source, {"Invert if"}) +} + +@(test) +action_invert_if_with_init_edit :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + if x{*} := foo(); x < 0 { + bar() + } +} +`, + packages = {}, + } + + expected := `if x := foo(); x >= 0 { + } else { + bar() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_not_on_if :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x :={*} 5 +} +`, + packages = {}, + } + + // Should not have the invert action when not on an if statement + test.expect_action(t, &source, {}) +} + +@(test) +action_invert_if_not_eq :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + if x{*} != 0 { + foo() + } +} +`, + packages = {}, + } + + expected := `if x == 0 { + } else { + foo() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_lt :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + if x{*} < 5 { + foo() + } +} +`, + packages = {}, + } + + expected := `if x >= 5 { + } else { + foo() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_gt :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + if x{*} > 5 { + foo() + } +} +`, + packages = {}, + } + + expected := `if x <= 5 { + } else { + foo() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_le :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + if x{*} <= 5 { + foo() + } +} +`, + packages = {}, + } + + expected := `if x > 5 { + } else { + foo() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_negated :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + if !x{*} { + foo() + } +} +`, + packages = {}, + } + + expected := `if x { + } else { + foo() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_boolean :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + if x{*} { + foo() + } +} +`, + packages = {}, + } + + expected := `if !x { + } else { + foo() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_else_if_chain :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x := something() + if x{*} > 0 { + statement1() + } else if x < 0 { + statement2() + } else { + statement3() + } +} +`, + packages = {}, + } + + expected := `if x <= 0 { + if x < 0 { + statement2() + } else { + statement3() + } + } else { + statement1() + }` + + test.expect_action_with_edit(t, &source, "Invert if", expected) +} + +@(test) +action_invert_if_not_on_else_if :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x := something() + if x > 0 { + statement1() + } else if x{*} < 0 { + statement2() + } else { + statement3() + } +} +`, + packages = {}, + } + + // Should not have the invert action when on an else-if statement + test.expect_action(t, &source, {}) +} + +@(test) +action_invert_if_not_on_else :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x := something() + if x > 0 { + statement1() + } else { + statement3(){*} + } +} +`, + packages = {}, + } + + // Should not have the invert action when in the else block (not on an if) + test.expect_action(t, &source, {}) +} + +@(test) +action_invert_if_nested_in_else_if_body :: proc(t: ^testing.T) { + source := test.Source { + main = `package test + +main :: proc() { + x := something() + if x > 0 { + statement1() + } else if x < 0 { + if y{*} > 0 { + statement2() + } + } else { + statement3() + } +} +`, + packages = {}, + } + + // Should have the invert action for an if statement nested inside an else-if body + test.expect_action(t, &source, {"Invert if"}) +} -- cgit v1.2.3