-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
gopls/internal/analysis/undeclared: infer return types from call context when synthesizing a function #47558
Comments
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. |
This sounds fun, do you mind if I give it a try? |
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. func sum(i1, i2 int) {} Sorry jumping ahead a bit maybe, just trying to think of edge cases to keep that generated function compilable. |
There are so many edge cases. |
@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. |
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. |
@rentziass, I'd recommend starting this outside of the gopls codebase, using just |
@findleyr thanks a lot, that's very useful! |
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.:
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
|
This looks awesome, thanks for sharing this update @rentziass! I think the next step here would be to integrate this logic into the With regards to naming parameters, you can also look at the code that does this in autocompletion here. |
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 |
@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. |
@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 |
Nope, I get it now, suggested fixes don't come out paired with |
Change https://golang.org/cl/348829 mentions this issue: |
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 <[email protected]> Trust: Rebecca Stambler <[email protected]> Trust: Peter Weinberger <[email protected]> Run-TryBot: Rebecca Stambler <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Go Bot <[email protected]>
This went in gopls/v0.7.3, should this issue remain open until the generated function considers returned values too? |
Thanks for adding this feature, but there's one issue. When doing TDD, you write the tests first, so sometimes the function, or method doesn't exists. So when you use this auto implement feature, it creates the function or method in the test file, instead of the normal file, where you put the package code. Is this something you can solve? @rentziass Also, yeah, it should consider return values imo. Awesome job btw. |
@enkeyz just a year late, sorry 😅 not sure this is something you can solve, test code is still part of a package and depending on whether you're using a @findleyr is it ok if I start looking into adding expected returned values too? After sometime spent as a user of this analyzer it really is a bit of a missing feature. |
@rentziass sounds good, please let us know how we can help. |
I change the title to reflect the remaining missing functionality: the 'undeclared' analyzer currently implements the missing function, but does not yet infer its return types from the context of the call. |
Change https://go.dev/cl/560475 mentions this issue: |
This CL changes the name of the fix from "Create variable" to "Create function" when appropriate. Oddly the names of fixes appear to be systematically not covered by our tests. Also tidy up the code. Updates golang/go#47558 Change-Id: Ibbefeb90874d7ce481893cfa401c59a421aad5e4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/560475 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Robert Findley <[email protected]>
Change https://go.dev/cl/623156 mentions this issue: |
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
The text was updated successfully, but these errors were encountered: