golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
121.47k stars 17.4k forks source link

x/tools/gopls: add .if and .not snippets for bool types #47545

Open hhhapz opened 2 years ago

hhhapz commented 2 years ago

Is your feature request related to a problem? Please describe. The introduction of postfix snippets has brought very convenient functionality to gopls, especially when working outside of Goland. I wanted to contribute more ideas for postfix completions inspired from what's already available from their editors. In particular, I wanted to propose having a if! and not! prompts, which do this (respectively):

(complex bool expression).if|
if (complex bool expression) {
    |
}

(expr).not|
!(expr)

Describe the solution you'd like Implementing these in completion/postfix_snippets.go.

Describe alternatives you've considered As a vim developper personally, I use snippetsEmu. Other potential alternatives are available in VSCode and Goland.

findleyr commented 2 years ago

CC @muirdm.

findleyr commented 2 years ago

Thanks for the suggestion. Muir might have some thoughts on this.

hhhapz commented 2 years ago

Wanted to add, that we could totally over-engineer the not by handling stuff like ==, >= ^, etc specially but I personally think it's overkill. Most of the time (at least personally) that this is useful is just negating a boolean var or a complex boolean expression surrounded by parenthesis.

In my mind, for the edge cases:

if x == y.not

should expand to

if x == !y

This helps keep the completion snippets more light weight, and if the user so desires, they can get the functionality by surrounding the expression with parentheses, or alternatively manually changing it.

muirdm commented 2 years ago

The if! snippet sounds reasonable, but you will need to do some work to make a == b.if! be handled as (a == b).if! (i.e. to apply to the entire statement instead of the selector expression). You also may run into issues with the keyword if as a selector.

I'm less convinced about the not! snippet. As described it would only apply to bool expressions, so for example it wouldn't work in cases like if a == "foo".not. If it did work in cases like that, then it would be hard to predict what it would do in more complex cases like if (a == b) == (c == d).not. Does Goland offer a "not" snippet?

ianlancetaylor commented 2 years ago

I don't think this needs to go through the proposal process, so taking it out. Please comment if you disagree.

hhhapz commented 2 years ago

You also may run into issues with the keyword if as a selector.

Sorry, I am not entirely sure I understand what you mean here.

Goland's implementation for these snippets is actually a bit complex. When there is an abiguous case, like "a == b.not!", it shows a context menu to let the user pick whether to only do the first part, or the entire thing: image image

From my understanding (correct me if I am wrong) is not possible with gopls atm, so I think it would be reasonable to ditch the .not snippet unless you have a suggestion on how we can circumvent this?

muirdm commented 2 years ago

You also may run into issues with the keyword if as a selector.

Sorry, I am not entirely sure I understand what you mean here.

The Go parser has trouble with keywords showing up in unexpected/invalid places (i.e. the "if" in foo || bar.if). Gopls has a workaround that tries to make "foo.if" parse the "if" as a plain identifier rather than an if statement, but I'm not sure if it will work in the case of the new snippet.

From my understanding (correct me if I am wrong) is not possible with gopls

LSP snippets have choices which might allow for a similar experience. But simpler and better, probably, would be just to have gopls offer multiple completion candidates, one for each possibility.

Regardless, I would skip not and start with if first because it seems more useful and straightforward.

hhhapz commented 2 years ago

Sounds good. I will take a stab at it. Will report back if I come across any difficulty 😄

nchengyeeshen commented 11 months ago

@hhhapz Are you still working on this issue?