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

First pass of support for completion #22

Merged
merged 14 commits into from
Nov 10, 2018

Conversation

raph-amiard
Copy link
Member

@reznikmm
Copy link
Member

reznikmm commented Nov 7, 2018

It seems we need to bump stable branch of libadalang to make it working

@raph-amiard
Copy link
Member Author

@reznikmm Ah, didn't know this relied on the stable version of LAL. I wonder if for now we shouldn't make it sync with LAL, and maybe have a stable branch for it that is in sync with LAL stable ? It would make development of new features much easier ...

@raph-amiard raph-amiard force-pushed the topic/completion branch 2 times, most recently from 347eb37 to 839bc48 Compare November 8, 2018 15:09
@@ -33,9 +33,10 @@ package body LSP.Ada_Documents is
function To_LSP_String
(Value : Wide_Wide_String) return LSP.Types.LSP_String;

function Get_Symbol_Kind
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you rename this function? It returns LSP.Messages.SymbolKind; So Get_Symbol_Kind is good enough for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not only rename it, I change its mode of operation: Before it operated on an Ada_Node, now it operates on a Decl, hence the renaming. Also, as you can see in this next commit:

162c8f1

It's used as a basis to provide completion kind too. We can rename it Get_Decl_Symbol_Kind if you prefer, but ultimately it would be good to have all of those based on some function that operates on an abstract kind, in order to not duplicate the logic for every kind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep these two kinds independent, so we will be able to change mapping from nodes to symbol kind without affecting mapping to completion kind.

Copy link
Member Author

@raph-amiard raph-amiard Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree.

  1. I'm not going to duplicate code today because we might need that later. If we need different logic later, we'll take care of it later, it's simple enough.

  2. There is no practical use case where you want a different kind for symbols and completions. Also keep in mind that all those kinds determine are icons.

Also please notice that the two enum types share a lot of enum literals. If something is A_Function for completion, there is a very big chance that it will stay that way for symbols, and no reason to duplicate the mappings from LAL.


Search_GPR_File (Root, Result);
-- If not found, perform a comprehensive search everywhere below
-- root. TODO: I'm not sure about that logic. As an user I might
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think comments in the code is right place to discuss server behavior. If you are not sure, open an issue instead of add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, however a comment is better than nothing in the meantime ! I added a comment because I needed to understand what this function did.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue and a reference to the issue in the comment. I prefer to keep the comment so we don't forget about it later :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we have the issue and we won't forget it. Part after TODO: is not required any more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I disagree! It might not be apparent to somebody reading/modifying this code that there is something going on here that might need to be changed, and the information might be useful to her/him. Or, as @setton put it, there is very rarely too much comments. But whatever, will remove it, it's unimportant enough.

Server_Trace.Trace (Symbolic_Traceback (E));

when Ada.IO_Exceptions.End_Error =>
Server_Trace.Trace
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer Failure exit status here. One of the test in our tiny testsuite checks this.

Copy link
Member Author

@raph-amiard raph-amiard Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean rather than checking for End_Error ? This was the shortest path, I modified the function that raised a Constraint_Error before. We can further modify it to use error codes instead, but I would suggest doing that in a second pass, since this code is already an improvement over the status quo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I think I understood what you meant. You want the server to quit with an error code on this condition right ? In that case: OK, will take care of it !

FWIW: VS Code always quit like that on my system for some reason, which is why I didn't assume that it was an error case ...

The LSP shouldn't create project files implicitly for the user inside
his project directory. Leave that responsibility to the LSP clients, but
provide a sane default behavior using a default project file in /tmp/.
1. Custom unit provider is not needed

2. It didn't include defensive guard against empty files. Consequently,
   when a unit wouldn't be found, Unit_File_Name would return the empty
   string, and then Get_Unit would try to parse the root dir as a file.
   For an obscure reason, this is not rejected by Libadalang at the
   moment.

Replace the unit provider by Libadalang's Project_Unit_Provider.
@reznikmm reznikmm merged commit 701e379 into AdaCore:master Nov 10, 2018
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.

2 participants