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: no completion for function literal in parameter #38416

Open
stamblerre opened this issue Apr 13, 2020 · 21 comments
Open

x/tools/gopls: no completion for function literal in parameter #38416

stamblerre opened this issue Apr 13, 2020 · 21 comments

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 13, 2020

Triggering completion here should offer a suggestion for a function literal.
The function call pictured below is https://pkg.go.dev/golang.org/x/build/maintner?tab=doc#Gerrit.ForeachProjectUnsorted, which takes a function as a parameter.

Screen Shot 2020-04-11 at 12 16 46 PM

/cc @muirdm

@stamblerre stamblerre added this to the gopls/v0.5.0 milestone Apr 13, 2020
@muirdm
Copy link

@muirdm muirdm commented Apr 13, 2020

That func literal requires placeholders to be enabled since the function signature doesn't give a name for the parameter. gopls gives a placeholder for func(<g> *GerritProject) since it is just guessing on variable name.

If we change the placeholder option to only apply to function invocations, then this problem would go away.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 13, 2020

I've hit this a lot too, and didn't realize I'd need to have placeholders to do it (I don't like them, so have them disabled) so had been assuming it was a weird bug that only affected some code.

Would it be reasonable to insert with the parameter names as _ in the no-placeholder case? (Enabling placeholders outside of calls would work too, of course.)

@muirdm
Copy link

@muirdm muirdm commented Apr 13, 2020

Would you dislike placeholders in this case as well, or do you mainly not want the function call placeholders?

The reason not to include "_" was because it is inconvenient/annoying to go back and update them. In general I feel like having to go "backwards" to change something is quite annoying, even vs having to type out the entire signature.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 13, 2020

I gave it a shot, re-enabling placeholders and checking this. Given it's a declaration, I don't mind the placeholders so much.

A small part (but definitely not all) of my issue with placeholders for normal calls is that it inserts "incorrect" code by inserting the type names (which then need to be replaced with values of that type). For decls, that's not the case, as it inserts say c context.Context as a parameter, which is valid code when declaring a function parameter.

This doesn't solve my other personal usability issues with placeholders, but I think it's fine in this case and better than inserting _ (since you get generated parameter names).

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2020

Change https://golang.org/cl/228104 mentions this issue: internal/lsp/source: limit effect of usePlaceholders option

@muirdm
Copy link

@muirdm muirdm commented Apr 14, 2020

(but definitely not all) of my issue with placeholders

What else don't you like about placeholders?

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 14, 2020

I'll try to explain it, but this is of course personal preference and years of workflow gained from previous Go tooling.

  1. I'm just not used to it (this is the first experience I have with it, in any editor or language outside of pre-defined "snippets", which I also dislike and disable). A big blob of text appears that doesn't look like anything I'm going to write and my brain goes "okay time to delete this". Without placeholders, it just adds the () to the function -- which itself took a good while to get used to (and isn't 100% right in all cases, especially when I'm trying to work with functions as values and have to undo the parens).
  2. In VS Code, the moment you move the cursor, the placeholder is "committed" and stays there, broken code and all. If I'm writing something and leave to go refer to what I'm calling, say go-to-def via moving the cursor and F12, accidentally click, etc, then I've got to do cleanup afterward. It's unlikely that I'm going to know exactly what I want to write in order to get it all right the fist time when tabbing through. And at the end of the day, I'm still going to have to type out the parameters. Placeholders are only saving me the commas, and costing me time when I make a mistake and break the workflow.
  3. I find much more value in the signature help while I'm typing than I do any placeholder; it's the same info except it doesn't get in my way (with the above items) and is re-triggerable if it happens to disappear.
  4. The "lame" reason that from the correctness POV, placeholders still put "invalid" code down (copy the signature, including types), so if I leave it then I'll end up with more red squiggles. Note that this is the case with almost every other placeholder, particularly struct literals where the placeholder is the type name (as the name has already been written to trigger the compltion).

So in comparison to your (100% reasonable) statement of "having to go backwards to change something is quite annoying", I find it annoying to have to undo what completion's done that I didn't like or "ask for", and the only benefit is not having to type a few commas.

Now, I know a lot of people like placeholders, and wouldn't want my workflow to break someone else's. I appreciate having the toggle so I can work happily.

I see that the above CL disables placeholders in other contexts as well. I'm not sure if I'm into that, for similar reasons as 1/2 above. I get things like:

image

Which end up distracting me in a similar fashion. The code isn't real, I have to clean it up, I'm just going to re-type it anyway, etc.

For function literals I think "forcing placeholders" (or a least filling things out automatically) makes sense as there is no way to end up with a usable function when the signature doesn't have names. It's certainly much better than not giving literal completion at all and leaving people confused as to why the (very cool) feature didn't work this time around.

But I'd be sad if the placeholders option stopped working for the other placeholders. It's much more likely that I'd just live with some function literals being mysteriously "broken" so I can save myself the trouble in the other contexts where I know it's going to slow me down. (EDIT: though now that I think about it, there just wouldn't be any way at all to disable those placeholders now, so I'd just be frustrated and leave my gopls old, or patch it myself... 🙁)

Can this change only be made for the specific "function literal without parameter names" case? Or, with placeholders disabled, just leave the auto-generated names?

@myitcv
Copy link
Member

@myitcv myitcv commented Apr 14, 2020

@zikaeroh very much agree.

We've covered this topic a few times, most recently in #37656 (comment). This issue seems to add weight to the idea that there needs to be different configuration options for the various types of placeholders that people might want:

  1. no placeholders
  2. a single placeholder that represents the cursor location
  3. ...
  4. what this issue is asking for
  5. the full monty; all placeholders in all places

(In #37656 (comment) and elsewhere I've argued that option 2 could be implemented using multiple text edits as opposed to snippets, but that's an orthogonal concern).

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 14, 2020

One extra thing I thought up after my comment (and numerous edits, sorry) is that I also view the "label" of the completion to signify the intent. If the completion says func(...) { }, as function literal completions do, it's clear to me that I'm about to generate something interesting. Same with the new if err != nil { ... } option (which I didn't realize had been locked behind placeholders until I read the aforementioned CL).

However, when I'm in a struct literal and the completion is Foo, I don't expect to end up with Foo: string,. The label said one thing, but I ended up with more than I agreed to.

I'd even go so far as to say that completions for function calls where the label is SomeFunc, I'd expect to end up with SomeFunc, but end up with SomeFunc() or with placeholders SomeFunc(huh string, foo int, vars ...interface{}). I'd advocate for the one with placeholders to have a label like SomeFunc() or SomeFunc(...) to indicate that it's about to create a call, and potentially even say that there should be completion items for both SomeFunc and SomeFunc(). But, those sorts of changes are out of scope for this function literal issue. 🙂

@muirdm
Copy link

@muirdm muirdm commented Apr 14, 2020

Thanks! It's really helpful to know other people's thoughts.

I sympathize with the placeholder text being invalid Go - that bugs me too.

For function call placeholders, my editor does not present signature help very well, so I rely more on the placeholders. Other than that, I don't like them either.

The struct field placeholder in particular may not be that useful. Perhaps the solution there is to just remove the placeholder altogether. I agree it is strange to just jam in some informational message as source code.

That being said, I don't understand the "have to clean it up" or "have to undo" sentiment (for struct field placeholders at least). Normally I just start typing what I want to complete and it overwrites the placeholder, so there are no wasted keystrokes. Your scenario in 2) of course causes extra work, and that has bugged me for function call placeholders.

I'd advocate for the one with placeholders to have a label like SomeFunc()

I agree. The label should be clearer.

there should be completion items for both SomeFunc and SomeFunc()

Are you still running into issues using functions as values? Maybe it is due to an expected type of interface{}?

I don't have any good ideas for how to proceed with this change. Long term I would love it if we could eliminate all the controversial use of placeholders and just get rid of the usePlaceholders option, but that might not be possible.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 14, 2020

That being said, I don't understand the "have to clean it up" or "have to undo" sentiment (for struct field placeholders at least). Normally I just start typing what I want to complete and it overwrites the placeholder, so there are no wasted keystrokes.

A lot of the time, I probably don't know what I'm about to type as parameters, or I get distracted. As stated in my issue, if I "leave", that stuff gets leftover and I have to fix it. The only keystrokes I'm saving are the commas, and I have no trouble typing those myself. I get that your editor doesn't display signature help too well, but mine does and I don't like the rest of the implications of the placeholders.

Also note that at least in VS Code (I think you use emacs?), typing a comma does not advance the placeholders, only tab does. This is frustrating as in every other language (and in Go previously), I have to type the commas. For example:

image

I'd rather just type it out.

Are you still running into issues using functions as values? Maybe it is due to an expected type of interface{}?

Simple case, x := someFunction. There's no way to know if I intend to call this function or not. Most of the time, I do, but sometimes I don't. I'm not really expecting gopls to know what I want. The extra completion items with distinct labels may help here, but deleting () is way easier than deleting an entire signature's worth of text.

I've also seen gopls add () and such when I use functions as literals:

image

It's extremely irritating. With placeholders off, it's less frustrating (just deleting ()).

Long term I would love it if we could eliminate all the controversial use of placeholders and just get rid of the usePlaceholders option, but that might not be possible.

I don't see this ever being possible. I'd much rather things continue to be configurable. For this specific issue, I think leaving the auto-generated names is the most "correct" thing to do for "usePlaceholders": false given it leaves valid code without placeholders.

I think it's possible to come up with a range of options that would make this more customizable as @myitcv said above, though it may take a little defining. I haven't personally found a case (besides this bug) where I'd actually want the placeholders to warrant extra levels, however. @myitcv has a technical motivation from the standpoint of not wanting to handle snippets at all (I think), which is more than I need given I'm a VS Code user.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 14, 2020

I entirely skipped over your "for struct field placeholders at least" when writing my first paragraph, my mistake. I thought I hadn't expressed my original thought well enough but you were just narrowing it to the struct stuff. Oops...

Generally I view all of the placeholders as "the same", at least from the "annoys me" POV where I have to undo. This is probably due to my first bullet thing before where I'm not used to it, and I feel weirded out by the extra text I didn't request (my follow-up label point).

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 14, 2020

I agree with most of @zikaeroh's comments above. I like the simplicity of usePlaceholders, so if we're going to offer more configurability, I'd like for us to have a concrete plan and all of the cases enumerated.

Regarding structs, I think the struct field placeholder could just be empty - I've seen that done before, and I find it helpful even though I don't like parameter placeholders.

@muirdm
Copy link

@muirdm muirdm commented Apr 14, 2020

It sounds like the options are:

  1. Do nothing, or
  2. Change my CL to keep struct field placeholders behind the "usePlaceholders" option, or
  3. Change my CL to get rid of the struct field placeholder altogether.
  1. and 3) would leave always-on placeholders for some literal completions ("func(..) {}" param names and "make([]foo, <0>)") and the "if err != nil { return 0, }" completion. Perhaps the "make()" placeholder for "0" is also controversial?
@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 14, 2020

I wouldn't pick 1, as it leaves this bug unresolved (literal completion sometimes not working). For me, 2/3 are equivalent as I would plan to keep placeholders disabled anyway; I have no opinion between them.

As for make, I'd prefer it without the <0> as I get now without placeholders, though a single character is less distracting than the other uses of placeholders.

If I can offer a fourth option: If placeholders are disabled and the completion item requires generated parameter names (i.e., this bug), offer the function literal without placeholders and place the cursor inside the function body, leaving the generated names.

  • For users with placeholders enabled, nothing changes.
  • For users with placeholders disabled, the function literal completion works consistently in all contexts (shows up in the completion list, cursor goes to the body).

Then, leave everything else as-is (or proceed with removing struct field placeholders in a different CL?).

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 16, 2020

If I can offer a fourth option: If placeholders are disabled and the completion item requires generated parameter names (i.e., this bug), offer the function literal without placeholders and place the cursor inside the function body, leaving the generated names.

  • For users with placeholders enabled, nothing changes.
  • For users with placeholders disabled, the function literal completion works consistently in all contexts (shows up in the completion list, cursor goes to the body).

Then, leave everything else as-is (or proceed with removing struct field placeholders in a different CL?).

This options sounds good to me, if it's ok with you, @muirdm.

@muirdm
Copy link

@muirdm muirdm commented Apr 16, 2020

It's fine with me. You will end up with unusable "_" parameters in some cases, but probably less annoying than having to type out the entire signature.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 16, 2020

You will end up with unusable "_" parameters in some cases

Under what conditions would you not be able to make a "fresh" name? Is it due to the current code picking out the first letter of the typename?

@muirdm
Copy link

@muirdm muirdm commented Apr 16, 2020

You will get "_" for unnamed types or if the abbreviation has been used by a previous param.

func(int32, bigCat, bigCar) currently gives func(_ int32, bc bigCat, _ bigCar)

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 16, 2020

Ah, I see, the camelCase conversion is clever. I would have been lazy and just made them p0, p1, p2, p3, etc... 🙂

If possible it'd be nice to have names for those even if they're "bad" so I can have gopls rename them after the fact; I can't trigger a rename on _. (Maybe pick off the first character for builtins like i, and start adding numbers for duplicates like i2/bc2? Spitballing.)

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Apr 16, 2020

In any case, any function literal will be better than none and I can hand-name the parameters in lieu of any changes there. I don't mean to make this more complicated than it needs to be.

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
You can’t perform that action at this time.