go-langserver icon indicating copy to clipboard operation
go-langserver copied to clipboard

Enable & fix limit parameter to */references requests

Open emidoots opened this issue 8 years ago • 4 comments

  • textDocument/references doesn't respect the limit parameter when streaming, only when the final results are returned.
  • workspace/references needs a limit parameter, and should respect it for the final results returned & during streaming.

Also, we should add these to our spec.

  • https://github.com/sourcegraph/language-server-protocol/blob/master/protocol.md#find-references-request is modified to look like:
interface ReferenceContext {
	/**
	 * Include the declaration of the current symbol.
	 */
	includeDeclaration: boolean;

        /* the number to limit results to */
        xlimit: number;
}

and https://github.com/sourcegraph/language-server-protocol/blob/master/extension-workspace-references.md#workspace-references-request is modified to look like:

/**
 * The parameters of a Workspace References Request.
 */
interface WorkspaceReferencesParams {
    /**
     * Metadata about the symbol that is being searched for.
     */
    query: Partial<SymbolDescriptor>;

    /**
     * Hints provides optional hints about where the language server should
     * look in order to find the symbol (this is an optimization). It is up to
     * the language server to define the schema of this object.
     */
    hints: {
        [hint: string]: any;
    }

    /* the number to limit results to */
    limit: number;
}

emidoots avatar Feb 28 '17 21:02 emidoots

I assume we should make the limit much higher than 100 then. Also do we have any concerns around quality of results when we start to limit. Previously we sorted in the langserver and then limited results, which would result in more relevant results within the limit.

Either way this should be easy to implement. Do we have any idea of how soon we want this in production?

keegancsmith avatar Mar 01 '17 08:03 keegancsmith

From my discussion with @nicksnyder -- we settled on him using the specs I wrote above and limiting the results on the frontend temporarily (b/c Go/Java LS do not respect it). So nothing is blocked on this / not a super high priority, but good to implement + spec out properly sometime soon.

emidoots avatar Mar 01 '17 08:03 emidoots

Previously we sorted in the langserver and then limited results, which would result in more relevant results within the limit.

This doesn't help in the context of streaming though. Client does its own sorting anyway.

Also do we have any concerns around quality of results when we start to limit.

Yes and no. To the extent that the language server can stream the most relevant results first (e.g. walk "close" files/dirs first) then it should. Other then that, the UI is just going to take the first N results it and filter. We can adjust N if we feel like results aren't relevant enough.

Since the frontend wants to limit anyway, the backend might as well not waste the time/resources sending the rest of the results.

nicksnyder avatar Mar 01 '17 09:03 nicksnyder

I have added https://github.com/sourcegraph/go-langserver/pull/173 which adds a limit param to xrefs. I think once that lands, we can close this out. However, we still need to update the spec and use the limit parameter in the frontend. + We should remove the 15s timeout once both of those have been completed.

keegancsmith avatar Mar 06 '17 15:03 keegancsmith