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

x/tools/gopls: find-references results returned in random order #40904

Closed
muirdm opened this issue Aug 19, 2020 · 14 comments
Closed

x/tools/gopls: find-references results returned in random order #40904

muirdm opened this issue Aug 19, 2020 · 14 comments
Labels
Milestone

Comments

@muirdm
Copy link

@muirdm muirdm commented Aug 19, 2020

Find-references (and find-implementations, and go-to-definition) should give results back in a consistent order. Currently I see a random order (sometimes it is random every time, other times it seems consistent for a while then changes). For example:

package main

func _() {
	var foo int
	
	_ = foo
	_ = foo
}
Finding references on "var foo int" I get back lines [5, 6] then [6, 5] in the next request.
Trace - 09:43:37 AM] Sending request 'textDocument/references - (14764)'.
Params: {
  "textDocument": {
    "uri": "file:///Users/muir/scratch/foo/foo.go"
  },
  "position": {
    "line": 3,
    "character": 5
  },
  "context": {
    "includeDeclaration": false
  }
}


[Trace - 09:43:37 AM] Received response 'textDocument/references - (14764)' in 5ms.
Result: [
  {
    "range": {
      "end": {
        "character": 8,
        "line": 5
      },
      "start": {
        "character": 5,
        "line": 5
      }
    },
    "uri": "file:///Users/muir/scratch/foo/foo.go"
  },
  {
    "range": {
      "end": {
        "character": 8,
        "line": 6
      },
      "start": {
        "character": 5,
        "line": 6
      }
    },
    "uri": "file:///Users/muir/scratch/foo/foo.go"
  }
]

[Trace - 09:43:42 AM] Sending request 'textDocument/references - (14766)'.
Params: {
  "textDocument": {
    "uri": "file:///Users/muir/scratch/foo/foo.go"
  },
  "position": {
    "line": 3,
    "character": 5
  },
  "context": {
    "includeDeclaration": false
  }
}


[Trace - 09:43:42 AM] Received response 'textDocument/references - (14766)' in 0ms.
Result: [
  {
    "range": {
      "end": {
        "character": 8,
        "line": 6
      },
      "start": {
        "character": 5,
        "line": 6
      }
    },
    "uri": "file:///Users/muir/scratch/foo/foo.go"
  },
  {
    "range": {
      "end": {
        "character": 8,
        "line": 5
      },
      "start": {
        "character": 5,
        "line": 5
      }
    },
    "uri": "file:///Users/muir/scratch/foo/foo.go"
  }
]

Perhaps we can sort first by file name, then secondarily by offset within file? Maybe also sort files from the starting package first?

Clients can still customize the sorting if they want, but I think gopls should return consistent results and results within a file should be ordered as they appear in the file.

@gopherbot gopherbot added this to the Unreleased milestone Aug 19, 2020
@myitcv
Copy link
Member

@myitcv myitcv commented Aug 19, 2020

Related: #32572

@muirdm
Copy link
Author

@muirdm muirdm commented Aug 19, 2020

Related: #32572

Thanks, forgot about that discussion.

To cover my bases I opened emacs-lsp/lsp-mode#2099 with regard to my client not sorting.

@myitcv do govim and vim-go sort the results?

@myitcv
Copy link
Member

@myitcv myitcv commented Aug 19, 2020

@muirdm

do govim and vim-go sort the results?

govim does, yes:

https://github.com/govim/govim/blob/be5a1e7a2db7f531a4f49cc1a2982a59f9478ac2/cmd/govim/util.go#L139-L182

Notice the workaround required for not sorting when we know the first result to be the definition location.

@myitcv
Copy link
Member

@myitcv myitcv commented Aug 19, 2020

cc @bhcleek for vim-go

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 21, 2020

Given that a few LSP clients seem not to sort, I think we should revisit the conclusion of #32572. Having unsorted references can be painful when refactoring.

I can do this, but let's wait until next week so that @stamblerre has a chance to opine.

@muirdm
Copy link
Author

@muirdm muirdm commented Aug 21, 2020

lsp-mode for Emacs now also sorts 😄

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v.0.5.0 Aug 25, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 26, 2020

I continue to maintain that clients should sort if they want sorted results, but we should also probably return deterministic results. I'll send a CL to sort the results.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 26, 2020

Change https://golang.org/cl/250737 mentions this issue: internal/lsp/source: sort references and implementations results

@myitcv
Copy link
Member

@myitcv myitcv commented Aug 26, 2020

I continue to maintain that clients should sort if they want sorted results, but we should also probably return deterministic results. I'll send a CL to sort the results.

The problem as I understand it is that nowhere in the spec is it declared that the first result will be the definition (there could, in other languages, be other nuances). Hence, to avoid clients having to special case the sort, the server needs to define the meaning and hence sort the results.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 26, 2020

Oh wow, I genuinely didn't realize that wasn't in the spec. I wonder why we did it that way then. In that case, I'm really not sure how the results should be sorted at all.

@bhcleek
Copy link
Contributor

@bhcleek bhcleek commented Aug 26, 2020

I also tend to believe that sorting shouldn't be guaranteed and that it's up to clients to do that if they want to present sorted results and that a server guaranteed deterministic order isn't worth the trouble.

@myitcv
Copy link
Member

@myitcv myitcv commented Aug 26, 2020

@bhcleek

up to clients to do that if they want to present sorted results

How does a generic LSP client do this in a language agnostic way? How does the client know that it should sort references[1:] instead of the entire references slice (because I would venture to suggest it does make sense for the first reference to be the definition in the case of Go).

@stamblerre

I'm really not sure how the results should be sorted at all.

As a first cut my suggestion would be:

  • definition first
  • then results within the main module, ordered lexicographically
  • then results from outside the main module, ordered lexicographically
@bhcleek
Copy link
Contributor

@bhcleek bhcleek commented Aug 26, 2020

How does a generic LSP client do this in a language agnostic way?

I don't understand the question. What needs to be language agnostic about sorting locations?

How does the client know that it should sort references[1:] instead of the entire references slice (because I would venture to suggest it does make sense for the first reference to be the definition in the case of Go).

Why should the definition be first as a general rule? I'll grant you that I can see reasons for that, but ultimately I think it should be up to the client, not the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.