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: add "implement missing function definition" quick fix #47558

Open
enkeyz opened this issue Aug 5, 2021 · 16 comments
Open

x/tools/gopls: add "implement missing function definition" quick fix #47558

enkeyz opened this issue Aug 5, 2021 · 16 comments

Comments

@enkeyz
Copy link

@enkeyz enkeyz commented Aug 5, 2021

Is your feature request related to a problem? Please describe.
It would be a QoL change, and save time(Javascript, C# and other vscode language extensions also have this feature).

Describe the solution you'd like
When you want to call a function but it's not yet defined and implemented, it would be nice that if you could choose from the context menu, to define the method or function.
for example:
go handleConnection(conn)
then you click on the quick fixes content menu, and it automatically defines the function for you, like this:
func handleConnection(conn net.Con) {}

Describe alternatives you've considered

Additional context
Screenshot from 2021-08-05 14-04-49

@enkeyz enkeyz changed the title Add "implement missing function" to the quick fixes context menu Add "implement missing function definition" to the quick fixes context menu Aug 5, 2021
@findleyr findleyr transferred this issue from golang/vscode-go Aug 5, 2021
@findleyr findleyr changed the title Add "implement missing function definition" to the quick fixes context menu x/tools/gopls: add "implement missing function definition" quick fix Aug 5, 2021
@gopherbot gopherbot added this to the Unreleased milestone Aug 5, 2021
@findleyr findleyr removed this from the Unreleased milestone Aug 5, 2021
@findleyr findleyr added this to the gopls/v1.0.0 milestone Aug 5, 2021
@findleyr findleyr removed this from the gopls/v1.0.0 milestone Aug 5, 2021
@findleyr findleyr added this to the gopls/unplanned milestone Aug 5, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 5, 2021

Thanks for the suggestion. I agree that this would be helpful, and should theoretically not be that difficult to add.

Transferred to the Go issue tracker, as this would be implemented by Gopls.

@rentziass
Copy link
Contributor

@rentziass rentziass commented Aug 7, 2021

This sounds fun, do you mind if I give it a try?
If I understood this correctly, when asking for code actions while on an undefined symbol that is a function call we should give the option to create an empty function with a signature that satisfies what that symbol is trying to do? Arguments would be what's being passed to the function, do we want to look at the "left" side too for return values?

@rentziass
Copy link
Contributor

@rentziass rentziass commented Aug 7, 2021

And re: argument names: if variables are passed to the function call like in @enkeyz example we could use those in the signature, what if instead this was the case?

sum(1, 2)
// or
reverse("string to reverse")

I guess for non primitive types we could use a camel case version of the type name? (e.g. net.Conn => conn)
And for primitive types maybe the initial char? (e.g. string => s)
And for multiple arguments with the same type maybe append a number? For the sum(1, 2) example that would be

func sum(i1, i2 int) {}

Sorry jumping ahead a bit maybe, just trying to think of edge cases to keep that generated function compilable.

@enkeyz
Copy link
Author

@enkeyz enkeyz commented Aug 7, 2021

sum(1, 2) or sum([1, 2]): this also can be a variadic function: func sum(nums ...int) int {}
and func sum(nums ...{}interface) int {}

There are so many edge cases.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 9, 2021

@rentziass: Yes, you can definitely give this a try! You should be able to match on the error code that says that the interface isn't implemented, and then you will need to extract the expected function signature from the error message. I wouldn't focus on edge cases to start--the implementation doesn't need to be perfect, since it's a user-initiated action. You're right to say that this should be a code action for a given diagnostic.

@rentziass
Copy link
Contributor

@rentziass rentziass commented Aug 10, 2021

Hey @stamblerre, it's been a while! Cool, I'll give this shot (although I'll probably come back with more questions). As per the signature for the time being I'd focus on arguments rather than returned values, it sounds easier although I might be wrong again.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 10, 2021

@rentziass, I'd recommend starting this outside of the gopls codebase, using just go/ast and go/types. Given a package with a single undeclared name error, can you generate a stub that makes the error go away? This is the crux of the problem, but integrating with gopls' APIs for type information and diagnostics can be distracting. If you can solve the problem outside of gopls, we can help you integrate it as a quickfix.

@rentziass
Copy link
Contributor

@rentziass rentziass commented Aug 12, 2021

@findleyr thanks a lot, that's very useful!

@rentziass
Copy link
Contributor

@rentziass rentziass commented Aug 14, 2021

So I've put together something that works (or seems to work). I'm probably gathering the data I need to generate those stubs in a poor manner, so please let me know how it can be improved.

I've added a bunch of test cases I used to validate what I was doing (still staying well away from any possible values an undeclared func might be intended to return).

For arguments names, if an identifier was used the name of that identifier is reused, otherwise names are inferred by the type of the argument, e.g.:

  • int -> i
  • error -> err
  • net.Conn -> conn
  • []int -> i

Also, if two arguments share the same identifier and/or type, argument name uniqueness is ensured, for example:

c("some", "strings")

// results in
func c(s1 string, s2 string) {}

For the sake of this conversation, I'm reporting below what code fixed by generating function stubs for each case looks like right now, so whoever is interested in checking the outputs doesn't have to dive into code:

Test Cases

u is always the function that was undeclared to begin with.

Primitives with an identifier

package missingfunction

func a(s string) {
        u(s)
}

func u(s string) {}

Imported types

package missingfunction

import "io"

func a(r io.Reader) {
        u(r)
}

func u(r io.Reader) {}

Non-primitive types, values and pointers

package missingfunction

type T struct {}

func a() {
        pointer := &T{}
        var value T
        u(pointer, value)
}

func u(pointer *T, value T) {}

Non-unique identifiers

package missingfunction

func a(s string, i int) {
        u(s, i, s)
}

func u(s1 string, i int, s2 string) {}

Literals / No identifiers

package missingfunction

type T struct {}

func a() {
        u("hey compiler", T{}, &T{})
}

func u(s string, t1 T, t2 *T) {}

Returned values from another function

package missingfunction

import "io"

func a() {
        u(io.MultiReader())
}

func u(reader io.Reader) {}

Passing an error

package missingfunction

func a() {
        var err error
        u(err)
}

func u(err error) {}

Using a function with multiple returned values as an argument

package missingfunction

func a() {
        u(b())
}

func b() (string, error) {
        return "", nil
}

func u(s string, err error) {}

Using operations as arguments

package missingfunction

import "time"

func a() {
        u(10 * time.Second)
}

func u(duration time.Duration) {}

Using a slice as argument

package missingfunction

func a() {
        u([]int{1, 2})
}

func u(i []int) {}

If what's there is actually usable/useful, what's the plan now? :) also thanks for having me doing this, I'm having a blast!

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 16, 2021

This looks awesome, thanks for sharing this update @rentziass! I think the next step here would be to integrate this logic into the gopls codebase. This should be a "type error analyzer", which is an analysis that provides quick fixes for type errors. You can find some examples here: https://github.com/golang/tools/tree/master/internal/lsp/analysis. Fillreturns (https://github.com/golang/tools/blob/master/internal/lsp/analysis/fillreturns/fillreturns.go) may be a good one to look at. Basically, you will want to create another folder in this directory named something like "implementmissing" and put the logic in an analyzer. You can add your test cases there as well, and then you can add your type error analyzer here: https://github.com/golang/tools/blob/a55d5155d907b5d1114e99daae56070cc0253114/internal/lsp/source/options.go#L1165.

With regards to naming parameters, you can also look at the code that does this in autocompletion here.

@enkeyz
Copy link
Author

@enkeyz enkeyz commented Aug 18, 2021

Would it be too big to ask, if you could implement this for methods too, not just functions?

go s.scannerWorker(jobs, result)

to

func (s *domainScanner) scannerWorker(jobs <-chan string, result chan<- *scanReport) {}

for example.

Anyway, thank you for your work @rentziass

@rentziass
Copy link
Contributor

@rentziass rentziass commented Aug 18, 2021

@enkeyz that's something I personally would have done after this piece, to keep this relatively small and reasonable. Another thing I'd like to look at as well is returned values expected for the function, i.e.

var err error
err = myFunc() # undeclared myFunc

should generate

func myFunc() error {}

But again, as iterations on top of this.

@rentziass
Copy link
Contributor

@rentziass rentziass commented Sep 7, 2021

@stamblerre I'm finally getting to integrate this, although I noticed a todo in analysis/undeclaredname to handle the calling of undeclared functions. Given that undeclared name is the same type error I'd be handling here should I just add the ability to handle undeclared functions in that package to keep all of that in the same place? I'll start doing that, if you come back with a denial extracting to its own separate package shouldn't be that big of a deal :)

@rentziass
Copy link
Contributor

@rentziass rentziass commented Sep 7, 2021

Nope, I get it now, suggested fixes don't come out paired with undeclaredname diagnostics, separate package it is :D

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 9, 2021

Change https://golang.org/cl/348829 mentions this issue: internal/lsp/analysis/implementmissing: add analyzer

gopherbot pushed a commit to golang/tools that referenced this issue Sep 21, 2021
This adds an analyzer that provides suggested fixes for undeclared name
errors on function calls, implementing a stub of the fuction (with an
empty body). As of now this doesn't try to guess returned types but
only parameters.
Generated functions are appended at the end of the file where these type
errors occur.

Updates golang/go#47558

Change-Id: Iaef45ada6b7b73de1fbe42e5f7e334512b65e6c7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/348829
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Peter Weinberger <pjw@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@rentziass
Copy link
Contributor

@rentziass rentziass commented Oct 22, 2021

This went in gopls/v0.7.3, should this issue remain open until the generated function considers returned values too?

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