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: highlight deprecated symbols #40447

Open
ainar-g opened this issue Jul 28, 2020 · 13 comments
Open

x/tools/gopls: highlight deprecated symbols #40447

ainar-g opened this issue Jul 28, 2020 · 13 comments

Comments

@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Jul 28, 2020

I'm using gopls (see version information below) with Vim (version 8.1) and natebosch/vim-lsc (version 632d49bf7a).

$ gopls version
golang.org/x/tools/gopls master
    golang.org/x/tools/gopls@v0.0.0-20200727233628-55644ead90ce h1:IYtWwbxt1Jl45OJO6l9OnkvIlOQlRyaHgIvg+Rm2qNQ=

Currently, when I hover over a deprecated symbol, I don't see any messages about the deprecation in Vim's statusline, and the symbol isn't highlighted. I assume, it's because gopls doesn't support that. I would like to see those messages.

@jotdl
Copy link

@jotdl jotdl commented Dec 1, 2020

May I have a take on this? I would be happy to support gopls :)

Actually, I already started to investigate and think I might found the correct place to implement:
As an additional analyzer under golang.org/x/tools/go/analysis/passes.

Do you agree that this is the correct place this needs to be implemented or would you recommend somewhere else?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 1, 2020

@jotdl: Absolutely, we'd love to have you contribute! You're right that this would be another analysis pass, though the feature may already be available through Staticcheck (which is off-by-default right now): https://staticcheck.io/docs/checks#SA1019.

LSP also has a diagnostic tag that allows us to mark something as deprecated (https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#diagnostic), so one option might be to enable this particular check by default and set the tag for any of its warnings.

@jotdl
Copy link

@jotdl jotdl commented Dec 1, 2020

Yes, you are absolutely right.

I didn't know yet that gopls comes with staticcheck. Sorry about that!

So it seems that in order to solve this, we actually don't need to implement the analysis part itself and could rely on staticcheck instead.

I did give this a try, but couldn't get it to work. A look into the staticcheck analysis code for deprecation warnings shows that it relies on facts produced by package dependencies (deprecated.go L59).

After a bit of debugging I found that we currently don't execute analysis for package dependencies anymore (due #35089 ):
internal/lsp/cache/analysis.go#L114-L117

After reenabling this code the analysis rule works again and then it's just a matter of adding the correct diagnostic tag which should be as easy as (as addition to Analyze in internal/lsp/source/diagnostics.go):

if e.Category == "SA1019" {
  tags = append(tags, protocol.Deprecated)
}

But I am not sure how to proceed in perspective to #35089 . Are there any plans to reenable it?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 1, 2020

Thank you for doing all of this investigation, and I'm sorry you had to do so much digging! Re-enabling that logic more correctly is now tracked by #38278, which will not happen for a while, but it seems like we can probably re-enable it for this case.

The changes you describe seem reasonable to me, so if you'd like to mail them, we'd love to get your contribution! One note is that we will be freezing the gopls codebase soon to focus on stability, so your change will likely not be merged until after the Go 1.16 release in February.

@jotdl
Copy link

@jotdl jotdl commented Dec 2, 2020

Thank you for doing all of this investigation, and I'm sorry you had to do so much digging!

No worries, I didn't expect something different as language servers are usually quite complicated and it's fun to read through all the code and get an idea of what's happening :)

So just to sum up things:

  • We would reactivate the analysis of package dependencies for SA1019
  • Set the correct diagnostic tag for analysis errors produced by SA1019
  • Leave it to the user to enable staticcheck via gopls configuration

Please correct me if I misunderstood something.

I'll try to get a PR up this evening. But I also need to check the contribution guidelines as this would be my first contribution.

Edit: Sorry, I didn't make it tonight. Everything is setup now, but I am not sure yet how and where this change is supposed to be tested. Need to look more into the testing part of gopls tomorrow.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 6, 2020

Yep, that all sounds correct to me! The contribution guidelines can be found at https://golang.org/doc/contribute.html, and there are some testing-related docs here: https://github.com/golang/tools/tree/master/internal/lsp/tests.

@jotdl
Copy link

@jotdl jotdl commented Dec 9, 2020

I am still on it, but sadly I am currently stuck:

When I enable the analyzation of package dependencies for SA1019 in analysis.go line 117 via

if a.Name == "fact_deprecated" { // enable SA1019
   ...
}

running the tests via go test ./... get stuck and they never finish.

It took me a while to figure out which tests are causing this behaviour and it seems like

  • internal/lsp/testdata/analyzer/bad_test
  • internal/lsp/testdata/suggestedfix/has_suggested_fix

are the cause of this issue. At least removing them fixes the hanging tests.

I could already mail the changes. But without fixing the tests I guess the changes are not worth much.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 11, 2020

If you'd like to mail the changes, I'd be happy to take a look and see if I can identify the issue.

@pjweinb
Copy link

@pjweinb pjweinb commented Mar 2, 2021

There doesn't seem to be any standard way of saying that something is deprecated. For instance, io/ioutil just says that there are better alternatives, while net/http uses the word Deprecated. Is the list the staticcheck uses (https://github.com/dominikh/go-tools/blob/v0.0.1-2020.1.6/deprecated/stdlib.go) hand generated? (last updated for go1.14)

@jotdl
Copy link

@jotdl jotdl commented Mar 2, 2021

@pjweinb There is no "enforced" way of marking something as deprecated, but I think it's widely accepted to use the Deprecated word as documented in https://github.com/golang/go/wiki/Deprecated and the majority of go developers deprecate code as documented there.

I am not sure if t the staticcheck list you mentioned is written by hand, but it looks to me like it was. I could think of a couple of reasons why they did so, but that's something you would better ask in the staticcheck repository as I sadly don't know a lot about it.

@jotdl
Copy link

@jotdl jotdl commented Mar 2, 2021

@stamblerre I am really sorry that I was unable to push this further during the last two months. As I was unable to test this and just got stuck I felt like it's not worth to push my changes as they are so minimal and even not tested.

Anyways, I'll rebase my changes and check what the current state is. Maybe I can figure out now what needs to be done in order to get the tests running.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Mar 2, 2021

@jotdl: No worries--not a problem at all! @pjweinb was just interested in adding deprecated markers to completion or document symbols, which is related but not necessarily overlapping with this issue.

@pjweinb
Copy link

@pjweinb pjweinb commented Mar 3, 2021

Yes. I apologize for the misunderstanding. My point was about trying to automate the process of finding out what was obsolete, and I thought this would be a good place to remember what I found. Some languages use @deprecated in function documentation, but Go doesn't.

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