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

Completion #9

Closed
felixfbecker opened this issue Sep 3, 2016 · 17 comments
Closed

Completion #9

felixfbecker opened this issue Sep 3, 2016 · 17 comments
Assignees

Comments

@felixfbecker
Copy link
Owner

felixfbecker commented Sep 3, 2016

Should suggest:

  • global symbols (classes, interfaces, traits, functions)
  • local variables
  • local parameters
  • variables imported with use
  • not the variables outside of the function scope
  • properties and methods of classes if the cursor is behind a ->
    • it should resolve the variable to a class type and get the members
  • namespaces and classes when the cursor is inside a use statement
  • built-in PHP functions, classes and globals
    • how to get them? Is there a machine-parseble version of the docs?

It should:

  • follow PSR autoloading rules defined in composer.json
  • parse docblocks for type annotations of parameters and properties
@lgabeskiria
Copy link
Contributor

@felixfbecker you are doing great job!

for built-in php classes, functions you can use https://github.com/JetBrains/phpstorm-stubs

@felixfbecker
Copy link
Owner Author

Thanks for the hint!

@mniewrzal
Copy link
Contributor

For more advanced completion features language server needs to index files from project. For now it looks that whole process of indexing is not standardized (microsoft/language-server-protocol#54)

@felixfbecker felixfbecker changed the title provideCompletion Completion Sep 9, 2016
@felixfbecker felixfbecker mentioned this issue Sep 9, 2016
@kaloyan-raev
Copy link
Contributor

Another repo with PHP Stubs: https://github.com/HvyIndustries/crane-php-stubs

Used by the Crane PHP language server.

@felixfbecker
Copy link
Owner Author

felixfbecker commented Sep 13, 2016

I think relying on the PHPStorm stubs is the better way to go.

As for the indexing, here is what I have in my head:

  • Upon initialization, parse the stub files
  • Upon a completion request
    • at the start of a line (no node in front of it)
      • check if it has been used in the file, try to load it using the autoloader if yes
      • read the directory (which is equivalent to the current namespace). we can quickly provide the list of files as classes, then resolve additional details with the completionItem/resolve request by parsing it. folders can be provided as namespaces the same way.
      • check if it is in the globally parsed files (composer.json files property and stubs)
    • with a namespace name in front it, do the above steps in the typed namespace
    • with a class in front of it, find the file with the autoloader and parse it, then provide completion
    • with a variable in front of it, find the definiton (basically what definitionRequest will do), then find out its type (assignment or parameter type hint / docblock. Need some recursive logic here to resolve assignments). If the resolved type is a class, find the file with the autoloader and parse it, then provide completion.

@felixfbecker
Copy link
Owner Author

https://github.com/JetBrains/phpstorm-stubs is licensed under Apache 2, while the language server is ISC. Does anyone know if we can use it? Are they compatible? Afaik Apache 2 is more restrictive

@kaloyan-raev
Copy link
Contributor

Do you plan redistributing the stub files with the language server? If yes, then looking at the license compatibility would make sense.

Another approach is to download the stub files on demand, i.e. before parsing them during the initialize phase. This way the language server is not tightly coupled to the stub files. Users may even configure the language server to download stub files from a different location. This approach should not qualify as "redistribution" and I don't see any license issue.

@felixfbecker
Copy link
Owner Author

I was planning to add them as dependency with composer:

    "require": {
        "JetBrains/phpstorm-stubs": "dev-master"
    },
    "repositories": [
        {
            "type": "package",
            "package": {
                "name": "JetBrains/phpstorm-stubs",
                "version": "dev-master",
                "dist": {
                    "url": "https://github.com/JetBrains/phpstorm-stubs/archive/master.zip",
                    "type": "zip"
                },
                "source": {
                    "url": "https://github.com/JetBrains/phpstorm-stubs",
                    "type": "git",
                    "reference": "master"
                }
            }
        }
    ],

downloading on demand would be an option of course, that would put the burden on the user. The extension would have to make it easy for the user to download them and then send the location to the language server. Why can't they just release as ISC/MIT 🙄

@kaloyan-raev
Copy link
Contributor

I guess you can open an issue to the phpstorm-stubs project and ask to change the license to MIT/ISC.

However, I have read the 4. Redistribution section of the Apache 2.0 license and I don't see any license compatibility issue to distribute the stubs as you have described above.

Is there any particular clause that worries you?

@felixfbecker
Copy link
Owner Author

felixfbecker commented Sep 13, 2016

No, I just wanted to be sure. When we add it as dependency, the license notice will be included in the subfolder, so I guess it should be fine?

By the way: I just did a diff between HvyIndustries/crane-php-stubs and JetBrains/phpstorm-stubs and they are basically the same. Besides changes that were not merged from the upstream and some renames, the files are almost identical and it looks like crane's stubs are based on JetBrain's. Which would mean he copied the JetBrains stubs, removed the README and LICENSE, removed the PHPStorm specific files, and published it in a fresh commit as "crane php stubs". That would be an illegal copyright infringement and a violation of the license terms, which states that the license must be included in all copies.

That is exactly what I do not want to do. Give credit where credit is due.

@kaloyan-raev
Copy link
Contributor

I did not notice that. I agree with you.

@felixfbecker
Copy link
Owner Author

Maybe I'm wrong and the diff just happens to be similar because it includes the same documentation and signatures. I don't want to falsely accuse anyone here, and it is none of my business, but for this language server I think we shouldn't maintain our own stubs and it's best to include some open source option (without breaking any license).

@lgabeskiria
Copy link
Contributor

how about including completion for [global] keyword?

@mniewrzal
Copy link
Contributor

I'm experimenting with keywords completion. I prepare simplest possible implementation https://github.com/mniewrzal/php-language-server/tree/keyword_completion. There are two problems. First is that internal PHP support in VS Code also has now keywords completion (results are doubled). But its not very important from language server point of view. Second is microsoft/vscode#11876 and I'm wating for response from Code team.

@ghost
Copy link

ghost commented Oct 22, 2016

Hi, @felixfbecker, great to hear that someone else is also looking to develop FOSS indexing and IDE-like functionalities for PHP. It seems we are both developing a similar project but for different editors; I maintain the PHP integrator packages for Atom(.io), which also provide things such as autocompletion and code navigation, much like your package does for Visual Studio Code.

I've already went fairly far in implementing autocompletion and code navigation in my packages and already have most of the functionality you're looking for in this ticket in place (local variable scanning, built-in class fetching, docblock analysis and inheritance, ...). My work can be found here [1]. (I'm explicitly linking to the development version as it went through some major refactoring.)

I certainly don't want to keep you from writing your own indexer if that's what you desire, but just wanted to let you know that there is a similar project out there. I've only recently split off my PHP indexer (which is also based on php-parser, so I'm very glad to see you got nikic to improve the lexer's robustness when it encounters syntax errors!) from my atom-base package into a separate project and repository, hence the apparant low number of commits and activity.

[1] https://github.com/php-integrator/core/tree/development

@felixfbecker
Copy link
Owner Author

felixfbecker commented Oct 23, 2016

@Gert-dev thanks for reaching out! This language server is intended to be usable by any IDE as it uses the open language server protocol, which is being implemented by many IDEs. That means if Atom had a LSP client, it could consume it too. The server already has a simple indexer, it just isn't used to provide completion yet, only "go to definition", symbol search, "find all references", hover and parse errors. I am very open to joining efforts, and am particulary interested in how you leveraged the PHP parser to provide completion. Since your codebase is quite large and to not clutter this issue, would you jump on Gitter and explain a bit of your work?

@ghost
Copy link

ghost commented Oct 23, 2016

@felixfbecker After reading your post I also realized that documentation for my indexer/core is currently in a rather sorry state. It is also not on packagist yet. This is because, as I mentioned, I've only very recently split off the core from the base Atom package and didn't think I'd reach a point where it'd be particularly useful to other developers and only realized this after finding out about this project and reading this ticket (hence my post).

Because of this, I think it's perhaps a good idea if I look into further refactoring some of the remaining warts, start writing documentation on GitHub about its usage and finally publish the core on packagist. This way my explanations can be read be everyone instead of lost in a Gitter instance somewhere ;-). I had been thinking about writing this documentation before as well as it is currently not entirely clear to users of my package what features are exactly supported. I've already created tickets about it here [1] [2].

[1] https://github.com/php-integrator/core/issues/25
[1] https://github.com/php-integrator/core/issues/26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants