Skip to content

give placeholder name to document symbols for decls with empty names #2484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

give placeholder name to document symbols for decls with empty names #2484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/features/document_symbol.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,11 @@ fn convertSymbolsInternal(
const to: []types.DocumentSymbol = symbol_buffer.items[prev_len..];

for (from, to) |symbol, *out| {
// LSP spec requires that a symbol name is not empty or consisting only of whitespace
const name_is_empty = symbol.name.len == 0 or
std.mem.indexOfNone(u8, symbol.name, &std.ascii.whitespace) == null;
out.* = .{
.name = symbol.name,
.name = if (name_is_empty) "<unnamed>" else symbol.name,
.detail = symbol.detail,
.kind = symbol.kind,
// will be set later through the mapping below
Expand Down
14 changes: 14 additions & 0 deletions tests/lsp_features/document_symbol.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,20 @@ test "nested struct with self" {
);
}

test "empty decl names return non-empty document symbol" {
try testDocumentSymbol(
\\test "" {}
\\test " " {}
\\const @"" = 0;
\\const @" " = 0;
,
\\Method <unnamed>
\\Method <unnamed>
\\Constant <unnamed>
\\Constant <unnamed>
Comment on lines +103 to +106
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for a placeholder like <unnamed>. If the symbol name is not valid then render the name token as is:

Suggested change
\\Method <unnamed>
\\Method <unnamed>
\\Constant <unnamed>
\\Constant <unnamed>
\\Method ""
\\Method " "
\\Constant @""
\\Constant @" "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be ok to just always include the @/""? it'd make this logic a lot simpler

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symbol names shouldn't need to include the @ or "" unless it's needed for some reason. What I suggest isn't as simple as it could be but at least it should hopefully avoid any further issues we may have with how we display document symbol names:

Identifier names should be displayed without @"" unless they need to be escaped. We can assume that the identifier has already been canonicalized with zig fmt so rendering the raw identifier token is fine.

Test names with string literals can be displayed without "" unless one of these cases applies:

  • the string literal is empty
  • the string literal has leading or trailing whitespace
  • the string literal contains escape sequences
    Otherwise it's possible to have two test names that can't be distinguished from each other.

);
}

fn testDocumentSymbol(source: []const u8, expected: []const u8) !void {
var ctx: Context = try .init();
defer ctx.deinit();
Expand Down
Loading