Skip to content
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.

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

Poor result ranking for workspaceSymbol #81

Closed
HighCommander4 opened this issue Oct 4, 2020 · 5 comments · Fixed by #118
Closed

Poor result ranking for workspaceSymbol #81

HighCommander4 opened this issue Oct 4, 2020 · 5 comments · Fixed by #118

Comments

@HighCommander4
Copy link
Contributor

If I'm editing the LLVM codebase with vscode + clangd and I perform a workspaceSymbol search for TypeDecl, this is what I get:

workspacesymbol-screenshot

Note that clang::TypeDecl is the very last result, and all sorts of identifiers where the leaf segment is only a partial as opposed to an exact match are ranked higher than it.

I believe this is clearly suboptimal. Symbols where the leaf segment of the qualified name is an exact match should be ranked higher than ones where it's a partial match.

@kadircet
Copy link
Member

kadircet commented Oct 5, 2020

This is unfortunate and it looks like vscode is tripping us over somehow. SymbolInformation itself doesn't have any way to indicate score of a symbol, but clangd does its best to return results in a sorted order, see the attachement below for the top N results. But apparently vscode just throws away our ordering :/

Moving the issue into vscode-clangd.

{
   "id":341,
   "jsonrpc":"2.0",
   "result":[
      {
         "containerName":"clang",
         "kind":5,
         "location":{
            "range":{
               "end":{
                  "character":14,
                  "line":3063
               },
               "start":{
                  "character":6,
                  "line":3063
               }
            },
            "uri":"file:///usr/local/google/home/kadircet/repos/llvm/clang/include/clang/AST/Decl.h"
         },
         "name":"TypeDecl"
      },
      {
         "containerName":"clang::TypeDecl",
         "kind":9,
         "location":{
            "range":{
               "end":{
                  "character":10,
                  "line":3078
               },
               "start":{
                  "character":2,
                  "line":3078
               }
            },
            "uri":"file:///usr/local/google/home/kadircet/repos/llvm/clang/include/clang/AST/Decl.h"
         },
         "name":"TypeDecl"
      },
      {
         "containerName":"clang",
         "kind":5,
         "location":{
            "range":{
               "end":{
                  "character":21,
                  "line":3105
               },
               "start":{
                  "character":6,
                  "line":3105
               }
            },
            "uri":"file:///usr/local/google/home/kadircet/repos/llvm/clang/include/clang/AST/Decl.h"
         },
         "name":"TypedefNameDecl"
      },
      {
         "containerName":"clang",
         "kind":5,
         "location":{
            "range":{
               "end":{
                  "character":17,
                  "line":3207
               },
               "start":{
                  "character":6,
                  "line":3207
               }
            },
            "uri":"file:///usr/local/google/home/kadircet/repos/llvm/clang/include/clang/AST/Decl.h"
         },
         "name":"TypedefDecl"
      },
      {
         "containerName":"clang::ASTContext",
         "kind":6,
         "location":{
            "range":{
               "end":{
                  "character":26,
                  "line":1395
               },
               "start":{
                  "character":11,
                  "line":1395
               }
            },
            "uri":"file:///usr/local/google/home/kadircet/repos/llvm/clang/include/clang/AST/ASTContext.h"
         },
         "name":"getTypeDeclType"
      }

@kadircet kadircet transferred this issue from clangd/clangd Oct 5, 2020
@sam-mccall
Copy link
Member

Yeah, I'm happy to add a score extension to our responses, but there's not that much we can do here, this is a bug somewhere between VSCode and LSP.
(LSP doesn't let us communicate ordering beyond providing an order, and VSCode ignores the order we provide).

Any ideas before we close this?

@Trass3r
Copy link
Contributor

Trass3r commented Oct 5, 2020

The question is where the order is lost. Can barely find the relevant code in vscode, only this

sam-mccall added a commit to llvm/llvm-project that referenced this issue Oct 6, 2020
The protocol doesn't really incorporate ranking.
As with code completion, most clients respect what the server sends, but
VSCode re-ranks items, with predictable results.
See clangd/vscode-clangd#81

There's no filterText field so we may be unable to construct a good workaround.
But expose the score so we may be able to do this on the client in future.

Differential Revision: https://reviews.llvm.org/D88844
@HighCommander4
Copy link
Contributor Author

It looks like the culprit is our fix for #31.

What VSCode appears to be doing is:

  • compare symbol.name to the query string to discriminate between exact (prefix) matches and partial matches
  • rank exact matches above partial matches
  • within the group of partial matches, sort them alphabetically

(I'm basing this on experimentation, I haven't tracked down the relevant VSCode source.)

So, if symbol.name contains the leaf name only, and symbol.containerName contains the qualifier, as the API intends, then all is well, and exact matches get ranked above partial matches.

This is indeed how the server provides the symbols. However, we added middleware code to the client to manipulate the server results in the following way:

        provideWorkspaceSymbols: async (query, token, next) => {
          let symbols = await next(query, token);
          return symbols.map(symbol => {
            if (symbol.containerName)
              symbol.name = `${symbol.containerName}::${symbol.name}`;
            // Always clean the containerName to avoid displaying it twice.
            symbol.containerName = '';
            return symbol;
          })
        },

By including the qualifier in symbol.name, every match becomes a partial match (for names with qualifiers where the full qualifier is not included in the query), and we just get an alphabetical ordering of all the matches. So, by doing this, we solved one problem (the one in #31, that qualified names could not be used as a search query) but created another (this one, the poor ordering).

@HighCommander4
Copy link
Contributor Author

I proposed a fix which limits the manipulation added for #31, to just the cases where the query string is in fact qualified. That should solve the problem for the vast majority of cases. (Technically, the problem can still occur for partially-qualified query strings, but those are much less likely to have a mixture of exact and partial matches.)

arichardson pushed a commit to arichardson/llvm-project that referenced this issue Mar 24, 2021
The protocol doesn't really incorporate ranking.
As with code completion, most clients respect what the server sends, but
VSCode re-ranks items, with predictable results.
See clangd/vscode-clangd#81

There's no filterText field so we may be unable to construct a good workaround.
But expose the score so we may be able to do this on the client in future.

Differential Revision: https://reviews.llvm.org/D88844
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
The protocol doesn't really incorporate ranking.
As with code completion, most clients respect what the server sends, but
VSCode re-ranks items, with predictable results.
See clangd/vscode-clangd#81

There's no filterText field so we may be unable to construct a good workaround.
But expose the score so we may be able to do this on the client in future.

Differential Revision: https://reviews.llvm.org/D88844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants