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

Jump to bottom

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
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.5.0

Comments

@muirdm
Copy link

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 Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Aug 19, 2020
@gopherbot gopherbot added this to the Unreleased milestone Aug 19, 2020
@myitcv
Copy link
Member

myitcv commented Aug 19, 2020

Related: #32572

@muirdm muirdm mentioned this issue Aug 19, 2020
Closed
@muirdm
Copy link
Author

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 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 commented Aug 19, 2020

cc @bhcleek for vim-go

@bhcleek
Copy link
Contributor

bhcleek commented Aug 19, 2020

yes, vim-go also sorts them: https://github.com/fatih/vim-go/blob/bd56f5690807d4a92652fe7a4d10dc08f260564e/autoload/go/lsp.vim#L836

@findleyr
Copy link
Contributor

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 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

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

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

@myitcv
Copy link
Member

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

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 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 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 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.

@stamblerre stamblerre added this to the gopls/v.0.5.0 milestone Aug 26, 2020
@golang golang locked and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.5.0
Development

No branches or pull requests
6 participants
@bhcleek @myitcv @stamblerre @gopherbot @muirdm @findleyr