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: change the notation of postfix completion items to use a common prefix rather than a suffix #45718

Closed
urandom opened this issue Apr 23, 2021 · 13 comments

Comments

@urandom
Copy link

@urandom urandom commented Apr 23, 2021

gopls 0.6.10 added experimental support for postfix completions. Every such item has ! as a suffix (print!, range!), making it clear that it is a postfix completion item. However, that only helps with visibility when such an item is already in the list. Perhaps if the notation was changed so that they start with a common prefix instead (!print, !range), this would help with discoverability as well, since it should allow the user to just write someExpr.! and get all relevant postfix completion items.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 23, 2021

CC @muirdm @stamblerre

This seems reasonable, but I have only limited experience using the new postfix completions. Muir has thought about this much more, so I'm curious what he thinks.

@muirdm
Copy link

@muirdm muirdm commented Apr 23, 2021

Can you give a concrete example of a situation where the postfix completions weren't discoverable? I find that often I use postfix completions on objects that have no fields/methods, so foo. naturally lists only the postfix completions.

As for the particular "!" prefix suggestion, I have two concerns:

  1. It looks like unary negation, so it is somewhat confusing.
  2. It probably won't work in practice since "!" is not a valid identifier character, so typing it to filter completion results will probably mess up the AST for the file.

One other idea is we could make the postfix completions "secretly" be matched by e.g "postfix.print" within gopls, so you could optionally type foo.postfix to see all the postfix completions. Of course, that secret would not be discoverable either.

@urandom
Copy link
Author

@urandom urandom commented Apr 23, 2021

Perhaps it is still too buggy, but currently you can't even see all available completions, even for a type that doesn't have any methods:

Screenshot from 2021-04-23 19-36-51

I had to look at the code to see that there was also range! for example. If you typed the first two letters of the missing completion item it would show up, but not before.

I don't really know how the completion is implemented though. I just assumed that the current user input was passed outside of the current document AST, so I assumed having a ! as the initial character would not be a problem.

@muirdm
Copy link

@muirdm muirdm commented Apr 23, 2021

Ah, thanks for the example. It does appear to be a bug.

In this case we aren't getting all the completions because gopls isn't detecting that fns. begins a statement. The completion options are limited to expressions (e.g. gopls doesn't think it can insert an entire range statement).

I will see if we can improve the statement detection to work in this case (i.e. dangling selector). @findleyr would your planned parsing improvements improve this case to parse fns. as an empty selector rather than slurping up stuff on following lines? Below is what I used to reproduce the issue:

	f := [][]byte{}
	f.

	foo := []int{}
@muirdm
Copy link

@muirdm muirdm commented Apr 23, 2021

In particular, the above example is parsed as:

f  := [][]byte{}
f.foo := []int{}

The interpretation is close enough to being valid that I doubt go/parser can parse it any other way.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 23, 2021

Change https://golang.org/cl/313269 mentions this issue: lsp/completion: fix postfix completions preceding assignments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 24, 2021

The interpretation is close enough to being valid that I doubt go/parser can parse it any other way.

Yeah, I agree with that assessment. We might be able to have special handling for short var decls, but certainly not for assignment.

gopherbot pushed a commit to golang/tools that referenced this issue Apr 26, 2021
In cases like:

  foo.<>
  bar = 123

We weren't detecting that the selector preceding <> was a statement.
The above is parsed as "foo.<>bar = 123", so it looks like <> is
contained in an AssignStmt. This matters because we give different
postfix completions depending on whether it is valid to replace the
surrounding selector with a statement or not.

Updates golang/go#45718.

Change-Id: I8f74505b2c8c7060f1e94433904ff0a987d0cc57
Reviewed-on: https://go-review.googlesource.com/c/tools/+/313269
Run-TryBot: Muir Manders <muir@mnd.rs>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Rebecca Stambler <rstambler@golang.org>
@muirdm
Copy link

@muirdm muirdm commented Apr 26, 2021

@urandom A fix for the particular scenario above was just merged to master. Can you try it out and see if you think we need more changes regarding postfix snippet discoverability?

@urandom
Copy link
Author

@urandom urandom commented Apr 27, 2021

@muirdm
I'm afraid that doesn't seem possible at the moment:

$ go get golang.org/x/tools/gopls@master
go: downloading golang.org/x/tools/gopls v0.0.0-20210427040217-735ed62f40b9
go: downloading golang.org/x/tools v0.1.1-0.20210427040217-735ed62f40b9
# golang.org/x/tools/gopls/internal/hooks
/home/urandom/.go/pkg/mod/golang.org/x/tools/gopls@v0.0.0-20210427040217-735ed62f40b9/internal/hooks/analysis.go:27:34: too many arguments in call to options.AddStaticcheckAnalyzer
	have (*analysis.Analyzer, bool)
	want (*analysis.Analyzer)
@muirdm
Copy link

@muirdm muirdm commented Apr 27, 2021

I'm afraid that doesn't seem possible at the moment:

Try go get golang.org/x/tools/gopls@master golang.org/x/tools@master (see https://github.com/golang/tools/blob/master/gopls/doc/advanced.md#unstable-versions).

@stamblerre stamblerre modified the milestones: Backlog, gopls/unplanned Apr 27, 2021
@urandom
Copy link
Author

@urandom urandom commented Apr 28, 2021

@muirdm

a lot more entries are now available. perhaps all for the particular context.

@muirdm
Copy link

@muirdm muirdm commented May 5, 2021

@urandom Can this be closed now or are you still experiencing related issues?

@urandom
Copy link
Author

@urandom urandom commented May 6, 2021

I think this can be closed now.

@urandom urandom closed this May 6, 2021
@stamblerre stamblerre removed this from the gopls/unplanned milestone May 6, 2021
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