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

RFC: Argument hints, a.k.a. parameter completions! #1300

Closed
wants to merge 7 commits into from
Closed

RFC: Argument hints, a.k.a. parameter completions! #1300

wants to merge 7 commits into from

Conversation

oblitum
Copy link
Contributor

@oblitum oblitum commented Dec 29, 2014

This week I’ve setup myself to accomplish some nice "parameter completion" that’s in line with YouCompleteMe’s spirit.

I’d like to talk about the reasoning behind the design first:

  • There can’t be any argument completion per se, only type hints for the current argument to be passed. So I’m gonna refer to this not as parameter completion, but as argument hints.
  • We should stay away from a mechanism like the one employed by jedi-vim. It’s pure hackery and very prone to instability. I’ve tried it and it breaks just because one is using one colorscheme or another. We should try to stick with the native popup mechanisms offered by Vim to be stable.
  • We can’t start to try parsing function prototypes ourselves, on YCM/ycmd's side, to provide the hints. Such semantic analysis power should come from Clang.

From these three assumptions I’ve devised the following design that can live in harmony with the rest of YCM:

  • There should be a new kind of trigger for argument hints.
  • Once the user starts typing text after the argument hint trigger, the normal YCM completion should be able to take place. Argument hints should popup solely at the spot of an argument hint trigger, and nowhere else.
  • Argument hints should not provide text for completion, just hints in the popup, but no text for completion for them.
  • Example candidates of argument hint triggers are ( and ,.
  • libclang should be smart enough to complete after ( and , to provide argument hints. If not, we should try to improve it for that and not try to hammer it on YCM/ycmd’s side.

I say this’s in line with YouCompleteMe’s spirit because it doesn’t requires pressing Enter, etc, for some kind of selection of arguments.

Check The Video

This pull is actually working for free function's argument hints with the current libclang. Hints for C++ require improvements on Clang’s codebase. Argument hints are a weak point of libclang at this moment, I’ve created a fork that makes it work for C++, but it doesn’t work for providing hints for template arguments, for functors, or constructors, which can all work with some bit of effort probably. Nonetheless, having it working for functions, function template instances and member variations of both is quite OK. I’ll have to discuss this on the cfe-dev mailing list later.

This pull is also targetting issue #1287. I’ve removed full prototypes from the default completion popup since they can get annoying, and are less useful with this enabled.

I’m providing this pull as an RFC at this moment. I’ve not provided any tests yet and I’d like to hear comments/suggestions/critics and the feasibility of it being merged. I’ll sign the CLA after that, or not.

On issue #234 I mention another method that resembles clang_complete, which also works and currently serves for template arguments. I also provide it for YCM in my fork.

Happy holidays!

Review on Reviewable

@vheon
Copy link
Contributor

vheon commented Dec 29, 2014

Given that YCM and ycmd are really powerful for C/C++ but not specific about that, how do you think this would work on language like python, javascript and other dynamic typed languages? What would you display then? Just the name of the parameter? I'm not saying that is not good but I wanted to make sure we check everything.

@oblitum
Copy link
Contributor Author

oblitum commented Dec 29, 2014

@vheon This would be a task of improving each semantic completion, it can't be done in a general way, except for YCM's side, the client, where we can have it setup for supporting adding custom argument hint triggers for any language at user's choice, which doesn't mean there's any support for that on ycmd's side. The part of just identifying an argument hint request on ycmd's side can be general too... I guess.

I can't define what the author of a semantic completer should put in the popup, it'll be the author's choice. For dynamic languages I suppose parameter names are OK, but I can't state whether it'll be feasible or not in any given language, except for the ones I'm implementing =)

Besides displaying parameter names and types, one can take advantage of descriptive doxygen-like comments for example, if possible.

@vheon
Copy link
Contributor

vheon commented Dec 29, 2014

This would be a task of improving each semantic completion

I absolute agree with this. Often people come here with request that should not be addressed to YCM itself but to the semantic engines.

For dynamic languages I suppose parameter names are OK

I proposed that because is actually what vim-jedi does.

@vheon
Copy link
Contributor

vheon commented Dec 30, 2014

@oblitum This however would not solve #1221. Am I right?

@oblitum
Copy link
Contributor Author

oblitum commented Dec 30, 2014

@vheon This commit no, I've looked into completing C and C++ call style solely. I dunno what YCM is doing for completing Obj-C arguments, it looks like just inserting the prototype to let the user edit/patch it right?

I suspect the solution here may not match for Obj-C call style because it's hard to have identification for the spot for poping up an argument hint. It shouldn't have support on libclang yet, and from YCM/ycmd's side it would be tricky too.

The solution based on clang_complete that I mention in #234 (preview) may work with Obj-C, if not already working. It just improves the aspect of not having to edit a prototype, but just filling parameter spots with the arguments.

Besides this, I know a guy that was using YCM for generic identifier completion with clang_complete for semantic completion, which should also provide a solution. I have never tried this.

@Valloric
Copy link
Member

First off, I love the crap out of your demo! Really nice!

This will be quite a bit of work though with several steps. The first is getting the required libclang changes merged in. While I can't speak for @r4nt, I'd be surprised if he'd be unwilling to help out with any political buy-in you might need from clang devs. He'll probably have some advice for you as well. (@r4nt Hopefully I'm not overstepping when I say all that.)

The second step is the ycmd work. I haven't looked into details of how you've implemented this, but the API I'd like to see is when ycmd returns a set of completions, it also sends back a list of param names and types for the function (if the completion entry is a function/method) in a structured way. So you'd have [{'param_name': 'foo', 'type': 'std::string&'}, ...] in the JSON for the completion entry that ycmd returns. You'd have this for every function that's available after a completion trigger like ->. Then clients like YCM and emacs-ycmd can use this.

It's important to not be limited by Vim constraints in how we approach this feature.

The third step would be changes to YCM to use this new information. In the case of YCM, we may want to just ignore the param data in the initial completion response and just re-issue another request, but this is pretty costly. I work in a codebase where calls to libclang for semantic completions are pretty damn expensive (in terms of latency) so if we could use data from one response that has all the info instead of sending 5 requests for a function with 5 args, that would be awesome.

The other stuff about showing this data to the user in a completion menu in Vim is an entirely secondary discussion. I like the approach that you're using, but again, displaying the data is orthogonal to first getting the data.

@r4nt
Copy link

r4nt commented Dec 31, 2014

@Valloric - happy to advise and to jump in on the discussion (I love love love the idea).
The first step is to outline this on cfe-dev. Feel free to cc klimek@google.com so I will get it right into my inbox. The important part will be to keep the interfaces backwards compatible; but if I understand the plan correctly, this shouldn't be an issue, we should be able to just slim down the number of options.

@hermeszr
Copy link

hermeszr commented Jan 4, 2015

Following the reference @oblitum posted on my issue on Disabling function signature display in C, the above pull request is a bliss: not only YCM pops up the semantic completion faster, but the argument hints work excellent with untouched libclang. Cheers!

@zeayes
Copy link

zeayes commented Jan 7, 2015

Good idea!

Francisco Lopes added 7 commits January 24, 2015 13:47
Will adopt TextChangedI if Vim version is at least 7.3.867 (the version
where this autocommand became available).

 - This also fixes the way YCM is doing Vim version checking.
To avoid confusion with new support for whitespace after triggers. Tabs
by default are better to produce whitespace.
@Valloric
Copy link
Member

What's the state of this PR now?

@oblitum
Copy link
Contributor Author

oblitum commented Feb 28, 2015

Hi @Valloric, at this moment I'm unable to concentrate any efforts into this since I have some other priorities but, let's take a recap:

  • With the awesome review help by @r4nt, I've improved this area in the clang codebase, which is already available upstream and should land in clang 3.7. I've added more stuff than I expected, there's completion for a lot of things and I provide a brief description here.
  • I'm mantaining the master branch of my fork updated with the official one by merging changes as soon as I can instead of doing rebases as I was doing in feature branches. It contains the current implementation that's fine for my taste at last. I've tested it on OS X, Ubuntu and Arch, as well as other people too, so it can be tried out without much hassle, except for the clang version requirements. I'm not supporting working with older clang releases as is as I was doing before for it to work with free functions, the current YCM code has been simplified by adopting the new changes in the API.

So, first thing to note is that this won't be present in any release until clang 3.7. I guess there's much room for discussion until there.

Reviewing your previous input from 2nd point onwards, since the first step has been completed:

The second step is the ycmd work. I haven't looked into details of how you've implemented this, but the API I'd like to see is when ycmd returns a set of completions, it also sends back a list of param names and types for the function (if the completion entry is a function/method) in a structured way. So you'd have [{'param_name': 'foo', 'type': 'std::string&'}, ...] in the JSON for the completion entry that ycmd returns. You'd have this for every function that's available after a completion trigger like ->. Then clients like YCM and emacs-ycmd can use this.

It's important to not be limited by Vim constraints in how we approach this feature.

The third step would be changes to YCM to use this new information. In the case of YCM, we may want to just ignore the param data in the initial completion response and just re-issue another request, but this is pretty costly. I work in a codebase where calls to libclang for semantic completions are pretty damn expensive (in terms of latency) so if we could use data from one response that has all the info instead of sending 5 requests for a function with 5 args, that would be awesome.

To tell the truth I'm not so into this approach, this would be limiting regarding the current capabilities that are available. For example, as can be checked in the following GIF, as the second argument is identified as an int, an int is suggested for the third one, not a generic type:

YouCompleteMe GIF

Another problem with the approach you mention is to require that the client should handle and/or be aware of argument positioning. The compiler can do so, not the editor, so, any client-side solution for that would use some kind of trick like tab-selection coupled with matching groups for the parameters, or some kind of UltiSnip trick, all of this working in constrained situations. For example, in my implementation in any call-site I can do <C-Space> at the beginning of any argument to see what's being expected for that parameter, I can format the code anyway I like, go back and forth, erase stuff, changing modes, and still, I can get hints for the next argument. With the approach of getting all at once expecting to reuse afterwards, what should happen if I do this kind of editing, I guess nothing can be done since context is lost and the client side don't have intelligence to follow completion.

Regarding the latency issue, my humble opinion is this: it's all or nothing. I really dislike semantic completion when it's too slow, even if it's just the standard one as offered by official YCM, so I just prefer to have it disabled. Now, most of the time (I mean in non-giant and/or template-freak codebases) it's quite fast, so, it's much better to have it working with full capabilities, even if it needs more requests, than have it half-working.

Resolution from my side: If there's interest in getting it to work by using per argument trigger, somewhat like it's working now for me, I'm in for polishing stuff to get it upstream to be available when clang 3.7 arrives. If it's of no interesting in adopting something along these lines, I myself don't have interest in implementing it.

In any case, the libclang as it's now in SVN and when 3.7 arrives provides the necessary tooling for completing parameters for constructors, function objects, free functions, template variations of these, etc, which is candidate for solving a lot of recurrent issues in YCM.

The other stuff about showing this data to the user in a completion menu in Vim is an entirely secondary discussion. I like the approach that you're using, but again, displaying the data is orthogonal to first getting the data.

Indeed, but I thought it would be important to mention this design because at last it's a way out for getting argument hints to work in bare VIM completion in some non-intrusive (matching groups with conceal, etc) way, while still having the normal completions. It was a strange way of working stuff out, but it was from this initial possibility that all the other things came to be. Still, it's relevant as for the VIM side of things.

@Valloric
Copy link
Member

@oblitum Sorry for the delayed response, work life has been very busy lately.

For example, as can be checked in the following GIF, as the second argument is identified as an int, an int is suggested for the third one, not a generic type

That's certainly an advantage; thanks for pointing it out.

Your argument that we should go for the multiple-requests model is pretty convincing. I would though still prefer to have the param info in the original completion request (like I outlined earlier) in addition to this new model where ycmd can be queried for each param independently. Having the names & types of all the params would be useful in other situations.

So we're on the same page here. I'm fine with the multiple requests thing, but we'll have to put it behind a flag (probably on by default) so that the user can turn it off if it becomes too slow.

@oblitum
Copy link
Contributor Author

oblitum commented Mar 10, 2015

@Valloric That's a reasonable approach. I may try to work on that providing support for ycmd to respond requests about whole signatures too (or extending the original request with added information), the YCM part about handling that to the user is what I don't want to think much about myself. I may implement the multiple-request treatment in YCM side somewhat similar to the way it's working on my fork and make it optional, maybe ON by default, but the way it will work will probably be quite different from the way YCM would handle whole signatures, hence I'm not inclined at designing and implementing how them should be handled and presented (I see lots for points for discussion for how it could be implemented).

@lithammer
Copy link

Would this be able to support argument hinting for jedi-vim as you can see from this screenshot?

screenshot_function

@oblitum
Copy link
Contributor Author

oblitum commented Mar 10, 2015

@Renstrom The high level design probably, I believe that's the intent, for it not to be c++ specific, but implementation wise, this woldn't improve python side b/c it requires the python side of the completion engine to make use of any new capability in the communication between YCM and ycmd. Now, besides this, that Jedi functionality already works for me while coupled with YCM.

@Valloric
Copy link
Member

What's the state of this PR now? Is it blocked on something?

@BatmanAoD
Copy link

(If it matters: I build using CMake and EXTERNAL_LIBCLANG_PATH rather than using the install script.)

@oblitum
Copy link
Contributor Author

oblitum commented Jun 7, 2015

@BatmanAoD I've never experienced this issue, in my fork I've not changed the build system except about removing parts that offered non-system libclang build, so, the system libclang support when building should work exactly the same way.

@BatmanAoD
Copy link

@oblitum I saw that on your fork's README, but I'm not sure what you mean by it. The fork requires Libclang 3.7, but since that hasn't been released yet, I don't understand how it could be the "system libclang" on any system, unless that system is pretty heavily customized. And EXTERNAL_LIBCLANG_PATH is, if I understand correctly, precisely the "non-system libclang build" mechanism you mention, but it seemed to work fine except for the naming issue.

@BatmanAoD
Copy link

@oblitum Also, I've just noticed that it looks like you've overridden the g:ycm_key_list_select_completion and g:ycm_key_list_previous_completion options to remove <tab> and <S-tab>; was this intentional?

@oblitum
Copy link
Contributor Author

oblitum commented Jun 8, 2015

@BatmanAoD by "system's libclang", afaik, we mean any libclang that's not the one downloaded by YCM. You can break things further by stating that there's a default system's libclang and a custom built one, and not just "system's libclang" to mean both things, but until now I've always seen and used "system's libclang" to just mean, non-YCM's libclang.

@oblitum
Copy link
Contributor Author

oblitum commented Jun 8, 2015

@oblitum Also, I've just noticed that it looks like you've overridden the g:ycm_key_list_select_completion and g:ycm_key_list_previous_completion options to remove <tab> and <S-tab>; was this intentional?

@BatmanAoD check the README docs.

@oblitum
Copy link
Contributor Author

oblitum commented Jun 8, 2015

It's better to put questioning about the fork there than here.

@agauniyal
Copy link

When can we expect this feature to get merged into official YCM?

@oblitum
Copy link
Contributor Author

oblitum commented Oct 4, 2015

when I get some time to clean up my changes, I've started this some weekend
ago, but stopped progress because priorities, will get back to it anytime.
Em 04/10/2015 04:59, "Abhinav Gauniyal" notifications@github.com escreveu:

When can we expect this feature to get merged into official YCM?


Reply to this email directly or view it on GitHub
#1300 (comment)
.

@tu-nv
Copy link

tu-nv commented Oct 26, 2015

@oblitum if you have time, please consider working on this patch. It seems that not much work remains. I am sure that many people like me are waiting for this amazing feature in official YCM.

@m42e
Copy link
Contributor

m42e commented Dec 3, 2015

👍

@balta2ar
Copy link

balta2ar commented Dec 7, 2015

Great feature! I'm eagerly waiting for this patch to be ready!

@phcerdan
Copy link

Amazing feature @oblitum, I am using your branch, but having it upstream would be great.

@BigfootN BigfootN mentioned this pull request Mar 7, 2016
11 tasks
@oblitum oblitum closed this Apr 23, 2016
@oblitum oblitum deleted the argument_hints branch April 23, 2016 21:19
@yura-pakhuchiy
Copy link

any chances this feature will get merged?

@oblitum
Copy link
Contributor Author

oblitum commented Apr 23, 2016

@yura-pakhuchiy Probably. I've been adding small improvements to upstream YCM and ycmd over the time (my available time) aiming to reach what this proposes. The problem is that there are many changes, plus add to that that contributions to YCM/ycmd currently require effort beyond just coding, there's the added review process, plus convincing something is acceptable, plus many boring tests which sometimes are a bit tiring to provide.

This pull has just been closed because I just made a clean up at my fork, deleting some dead branches and refactoring it. I've just updated the current master on top of upstream's master, it adds the changes I'm looking forward to get merged, you may just install it just like installing the upstream YCM and have what's proposed here working.

bijancn pushed a commit to bijancn/YouCompleteMe that referenced this pull request Jul 26, 2016
… r=Valloric

Improves semantic trigger match.

Improves semantic trigger match.

Match triggers until user's caret column solely. Trying to match
triggers with characters after user's caret column doesn't seem
to have any utility.

---

This is the first in a series of pull-requests that will transparently enable
ycm-core#1300 for any completer that cares for. After this minimal foundation is
stablished, an initial Swift completer will be available, supporting diagnostics,
fix-it's, semantic completion and argument hints. Changes will also be
proposed to enable hints for C and C++.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/292)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.