Sarcasm / company-irony

company-mode completion back-end for irony-mode
118 stars 11 forks source link

irony backend severely cripples company features #1

Closed elemakil closed 10 years ago

elemakil commented 10 years ago

First of, let me say that I love that you chose to start this project fresh with company as completion framework. I'm looking forward to properous and fun coding sessions!

However, I have stumbled upon some bugs:

  1. Say there's a class foo with several member functions, among them foo(), foobar() and func(). If I type

    foo f;
    f.

    the completion menu shows and I can select a candidate using the arrow keys (or M-p, M-n). Typing f on the other hand does not narrow the list of completions to all candidates starting with f, instead the menu vanishes.

    Note that if I comment out the company-irony configuration and use the pure company-mode for completion [calling company-complete just after typing f.], entering the first char of a member narrows the list of candidates.

    Edit: Some further investigation revealed that this is related to the company-irony-setup-begin-commands setup. If I move point around a bit and return to the position just after the member access operator (the dot) and then execute company-complete, the menu shows and I can filter the candidates by entering characters.

  2. Assume that we have an #include <vector> statement. Entering std:: allows to select vector from the completion menu [albeit due to 1. this is rather cumbersome since one must scroll quite a bit ;-) ], this will then (yasnippet is enabled) insert vector<|> with point at |. Entering int and hitting tab yields std::vector<int>|.

    Somehow I'm lacking the option to get completion for the object's constructor. [This point might be a feature request rather than a bug report.]

  3. Say we have a class named name_longer_than_three_chars, without the irony backed entering nam will popup the completion menu (or show inline completion if there is no other candidate starting with nam) suggesting possible completions.

    company-irony seems to disable company's idle-completion feature. [Maybe this is related to point 4. and idle completion is not disabled but rather the backend parsing the file is inactive?]

    Edit: After closing and re-opening the file, suddenly entering nam will suggest the above class. It appears that re-parsing does not happen.

  4. Say I have added both company-irony and company-yasnippet to the company-backends list and I also have a snippet with the key for. Without the irony backend entering for will result in a popup allowing to expand the snippet. With the irony backend` no such thing will happen.

    Either company-mode does not support multiple active backends at the same time (couldn't find anything on this) or company-irony introduces some breaking change. Please clarify.

Sarcasm commented 10 years ago

First of, let me say that I love that you chose to start this project fresh with company as completion framework. I'm looking forward to properous and fun coding sessions!

Thanks! :)

  1. the completion menu shows and I can select a candidate using the arrow keys (or M-p, M-n). Typing f on the other hand does not narrow the list of completions to all candidates starting with f, instead the menu vanishes.

Thanks, I was annoyed by this but I'm not totally familiar with company and thought it was maybe normal. But testing your example with company-clang revealed the intended behavior and you are right, looks like a company-irony bug. I'm looking into it but I can't find it easily, will comment when I do.

  1. <...>
  2. Assume that we have an #include <vector> statement. Entering std:: allows to select vector from the completion menu [albeit due to 1. this is rather cumbersome since one must scroll quite a bit ;-) ], this will then (yasnippet is enabled) insert vector<|> with point at |. Entering int and hitting tab yields std::vector<int>|.

Somehow I'm lacking the option to get completion for the object's constructor. [This point might be a feature request rather than a bug report.]

Yep, that's how libclang returns the result and I think it makes sense. What if you want to do: std::vector<int>().swap(anotherVec); or std::vector<int>::iterator. But maybe it would be nice to have a way to list the constructors arguments. But when, seems difficult, at first I was think when you have a symbol maybe like: std::vector<int> foo| or after a parenthesis std::vector<int> foo(| but it could be just a function declaration:

#include <vector>

void f() {
  std::vector<int> foo();
}

So I'm open to ideas here but that will probably stay that way for a while.

  1. <...>
  2. <...>
  3. Say we have a class named name_longer_than_three_chars, without the irony backed entering nam will popup the completion menu (or show inline completion if there is no other candidate starting with nam) suggesting possible completions.

company-irony seems to disable company's idle-completion feature. [Maybe this is related to point 4. and idle completion is not disabled but rather the backend parsing the file is inactive?]

Thanks, like for 1. I can confirm but I don't see the fix just yet, will update you when I do.

For point 4., can you take a look at company's grouped backend and tell me if that's what you want?

C-h v company-backends RET:

Grouped back-ends:

An element of `company-backends' can also itself be a list of back-ends, then it's considered to be a "grouped" back-end.

When possible, commands taking a candidate as an argument are dispatched to the back-end it came from. In other cases, the first non-nil value among all the back-ends is returned.

The latter is the case for the prefix' command. But if the group contains the keyword:with', the back-ends after it are ignored for this command.

The completions from back-ends in a group are merged (but only from those that return the same `prefix').

Thanks for the issue(s), I'm investigating company-irony.

elemakil commented 10 years ago

Thanks for the swift reply. In case you missed my edits to the original post (I published them seconds after your reply) I'll repost them here:

Regarding 1.: Some further investigation revealed that this is related to the company-irony-setup-begin-commands setup. If I move point around a bit and return to the position just after the member access operator (the dot) and then execute company-complete, the menu shows and I can filter the candidates by entering characters.

Regarding 3.: After closing and re-opening the file, suddenly entering nam will suggest the above class. It appears that re-parsing does not happen.

It's good to know you're investigating this!

As for your remark on 2., you're right it is indeed not unambiguous what the user wants. I think the best course of action would be to follow these rules [assuming libclang yields data which can be used to generate the following case. I've never used libclang, so I don't know]:

  1. For TYPE|: The completions list should contain all of TYPE's constructors and otherwise publicly accessible entities (static member vars, typedefs, etc.). The completions menu should pop-up but close automatically if the first subsequently entered char(s) is/are not ( [for constructors] or :: [for typedefs, member vars]. This allows to use the completion engine but does not limit the user in case that he/she does not want to call a constructor/access a class scope entity.
  2. For TYPE NAME|: The completions list should contain all of TYPE's constructors. Again the completions popup should show automatically but close immediately if the next char is not (.

Following these rules one could easily construct std::vector<int>().swap(anotherVec); using the completion engine:

  1. std::| Now select vector from the completions list, then enter int into the angle braces and jump out of the inserted snippet.
  2. std::vector<int>| The completions list shows (among other things) () and ::iterator, select the former and hit enter.
  3. std::vector<int>()| Type ..
  4. std::vector<int>(). The completion menu shows all member functions, select swap() and expand the snippet with anotherVec.
  5. std::vector().swap(anotherVec)|Type;`

Assuming I have not forgotten any possible other completions options [and libclang actually provides the information required to implement this], this should yield seamless and fast completion.

Regarding 4.: Inserting

(add-to-list 'company-backends '(company-irony company-yasnippet))

instead of the advised variant works nearly as I desire. Typing for will show a completion menu with the correct snippet listed. However, typing f. for some variable f will show all public members and all yasnippet snippets (including for). I don't fully understand how company handles multiple backends but I think there is a way to limit completion to a subset of backends by calling company-BACKEND (i.e. company-irony) instead of company-complete.

Hence the irony trigger (the completion is executed after member/scope access trigger) should not advise a general completion but rather a restricted completion using only the irony backend.

Sarcasm commented 10 years ago

Thanks for the swift reply. In case you missed my edits to the original post (I published them seconds after your reply) I'll repost them here:

I saw them :)

Regarding 1.: Some further investigation revealed that this is related to the company-irony-setup-begin-commands setup. If I move point around a bit and return to the position just after the member access operator (the dot) and then execute company-complete, the menu shows and I can filter the candidates by entering characters.

I'm not sure how you identify company-irony-setup-begin-commands to be the culprit but I don't think it is. I tried company-clang with the same settings and the issue is not here.

Regarding 3.: After closing and re-opening the file, suddenly entering nam will suggest the above class. It appears that re-parsing does not happen.

So about this one, you are right, it looks like libclang doesn't take into account the change, so I'm not sure how to work around this just yet. If syntax-checking is added to irony-mode, it is likely that the buffer will be reparsed in between and the issue won't be much of a problem but I think it's worth asking on the Clang mailing list about this behavior. Will probably do when I can confirm I'm not saying rubbish.

About remark 2.. You may want to take a look at https://github.com/abo-abo/function-args/ (I think you said you are using semantic in another issue). This feature would indeed be nice to have but I don't think libclang provides this information easily and there are other features that I have in mind in the short term. So contribution welcomed... :)

Regarding point 4., I think the behavior is "normal" and some people may want a snippet after :: or ->, .... I think it would be totally appropriate to create a simple company backend that merge the features of both backend to match the specific behavior you want though. This is actually similar in my opinion to an issue on company-mode that I saw today, see the final answer here: https://github.com/company-mode/company-mode/issues/142#issuecomment-46589509

dgutov commented 10 years ago

testing your example with company-clang revealed the intended behavior and you are right, looks like a company-irony bug.

There is a certain difference between your prefix implementation and of company-clang's: you only return a cons right at trigger point, but company-clang--prefix calls company-grab-symbol-cons, which looks at the beginning of the current prefix, and returns a cons if preceding characters match the trigger regexp.

It's better in the scenario when the user managed to type the next character after . or -> before the popup had the time to appear, but should work the same if the next character is typed after a considerable delay. So it could be a bug in company, too, I haven't tested that yet.

Sarcasm commented 10 years ago

Thanks @dgutov, it was the problem indeed! Fixed in the last commit.

elemakil commented 10 years ago

I can confirm, the newest change fixes the filtering problem (item 1).

Sarcasm commented 10 years ago

Hey,

Can you take a look at the last version of irony mode? The commit https://github.com/Sarcasm/irony-mode/commit/a1b045c7f5f0377748fe2deaa31b70cd3e13379d should fix point 3..

dgutov commented 10 years ago

but should work the same if the next character is typed after a considerable delay. So it could be a bug in company, too, I haven't tested that yet.

Actually, no. The logic there seems sound: the code was treating the problem scenario as if the user had a long enough prefix to satisfy the min-length requirement, and then backspaced to a much shorter prefix. Aborting the idle completion seems appropriate.

Sarcasm commented 10 years ago

Thanks for the heads-up, the current behavior is sounds reasonable to me too.

elemakil commented 10 years ago

I can indeed confirm that the most recent version of irony-mode fixes point 3..

I also noticed that just typing foo on an empty line will complete to foo's constructors:

struct foo {
    foo( int i );
    foo( float f );
    foo( int i, float f );
};

int main(){
    foo| -> foo
            foo( int i )
            foo( float f )
            foo( int i, float f )
}

This got me thinking. The way I see it, internally company-irony uses yasnippet, therefore it should be possible to add an additional "jump point" just before the opening paren to the snippet used for constructors. If one want's to create a local variable, just hit space and enter the name of the variable, then jump to the next field (being the first variable). Otherwise just skip the first field.

Another remark: The new functionality does not work (yet) if there is something else before the current code in the line. Example:

int it, foo|                  -> no completion
foo * pf = new foo|           -> no completion
Sarcasm commented 10 years ago

I also noticed that just typing foo on an empty line will complete to foo's constructors:

I noticed that it doesn't always happen (not sure about the exact reason). Edit: apparently you noticed it as well in your last example.

The way I see it, internally company-irony uses yasnippet

Yasnippet is optional. Someday I would like to support the built-in snippet support of company-mode to use when yasnippet isn't available.

therefore it should be possible to add an additional "jump point" just before the opening paren to the snippet used for constructors

Maybe possible but the part "for constructors" isn't really possible ATM. Maybe libclang provides this information but it is not yet available in the candidate data. It is difficult to recognize constructors and functions, or cast operators, or anything with parentheses.

Can we agree on closing this issue since all except this point are fixed now I believe? This feature is not uninteresting but I think it will be better to create a specific issue/feature request in the irony-mode repository. It's not specific to company-irony but could be useful to any completion backend. There is actually another issue about the snippet expansion on completion (https://github.com/Sarcasm/irony-mode/issues/103) so I guess there is some work to be done in this area.

elemakil commented 10 years ago

You are correct, the constructor completion is the only point that is left unresolved with this issue. I guess closing this issue makes sense as well as moving the issue of constructor completion to the irony-mode repo.

On that note, I just realised that the completion menu does not show a function's return type. Is this a limitation/missing feature of libclang/irony-mode or a missing feature of company-irony?

Sarcasm commented 10 years ago

On that note, I just realised that the completion menu does not show a function's return type. Is this a limitation/missing feature of libclang/irony-mode or a missing feature of company-irony?

This is an oversight, the information is available and ready to be used, I'm just not sure yet where to put it. Maybe I will use the C++11 notation of trailing return type or something like that.

elemakil commented 10 years ago

I think the trailing return type notation would be a very good choice.

Usually the user selects the function first (by its name), then selects the correct overload (either from the overloads' arguments only or the combination of arguments and return type) and typically (unless the return type was already checked during overload selection) checks the return type only for consistency.

Since you seem to be already working on that, I don't think this is worth an "issue". I will close this issue since the main problems have been resolved. Thanks!