bmewburn / vscode-intelephense

PHP intellisense for Visual Studio Code
https://intelephense.com
Other
1.62k stars 94 forks source link

Doc block indentation missing and formatting issues #1145

Open mikehaertl opened 4 years ago

mikehaertl commented 4 years ago

For me the documentation and hover content has some issues:

Here's an example:

docblock_markdown

Note the missing spaces in the example for options() and the excessive underscores and backslashes at __app\\tasks... or _@returns_

The related response from intelephense is:

[Trace - 9:22:52 AM] Received response 'textDocument/hover - (6)' in 16ms.
Result: {
    "contents": {
        "kind": "markdown",
        "value": "__app\\\\tasks\\\\Task::options__\n\n```php\n<?php\nprotected function options() { }\n```\n\n_@return_ `array`  \nof options and default values. Child classes can use this\nmethod to define their own options. Options are set at runtime via the\n'options' array in the `$config` parameter passed to the constructor.\nWhen overriding this method, make sure you merge the parent options like\nthe following:\n\n```php\n<?php\npublic function options()\n{\nreturn array_merge(parent::options(), [\n// ...more default options here...\n}\n}\n```"
    },
    "range": {
        "start": {
            "line": 509,
            "character": 23
        },
        "end": {
            "line": 509,
            "character": 30
        }
    }
}

versions

I'm using neovim + coc.nvim + coc-phpls but as I understand it the response from intelephense is already missing the indentation. So I hope it's the right place to report this issue.

vim version: NVIM v0.5.0-506-gda6f38ab3 node version: v10.20.1 coc.nvim version: 0.0.78-0033e4e624 term: screen-256color platform: linux coc-phpls 2.1.8 intelephense 1.3.11

I've also checked #563 which should be fixed since 1.1.5.

bmewburn commented 4 years ago

The server is sending back markdown which explains the underscores and backslashes (4 bs to escape 2 bs for json then 2 bs to escape 1 bs for markdown). Are you able to post the capabilities.textDocument.hover data that the client sends for the initialize request? The client can declare the MarkupKinds it supports here.

I think the code block formatting is failing here because the intelephense formatter is run over any code blocks that are found and if it is not valid code it fails. In the example there are parse errors in the method call and there is no class encapsulating the method declaration.

mikehaertl commented 4 years ago

The server is sending back markdown which explains the underscores and backslashes (4 bs to escape 2 bs for json then 2 bs to escape 1 bs for markdown).

Right, but then it seems like the vim-markdown plugin has some issues with escaping. For a test I've now decoded the JSON contents.value from above and put it into a test.md file like this:

__app\\tasks\\Task::options__

```php
<?php
protected function options() { }
```

_@return_ `array`  
of options and default values. [...]

Opened in neovim the result looks better but still has issues with the \\ and also renders the two spaces after _@return_ array:

test_markdown

I still don't get why the rendered result looks different in a floating window, though. I know that this is probably not related to intelephense but with all the parts involved it's a bit hard to find out what's going on. Maybe you have some idea? (I've also tried with disabled markdown plugin which gives yet another but still incorrect rendering result).

Are you able to post the capabilities.textDocument.hover

Sure, I've included all parts where markdown is supported.

    "completion": {
        "dynamicRegistration": true,
        "contextSupport": true,
        "completionItem": {
            "snippetSupport": true,
            "commitCharactersSupport": true,
            "documentationFormat": [
                "markdown",
                "plaintext"
            ],
            "deprecatedSupport": true,
            "preselectSupport": true
        },
        "completionItemKind": {
            "valueSet": [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25 ]
        }
    },
    "hover": {
        "dynamicRegistration": true,
        "contentFormat": [
            "markdown",
            "plaintext"
        ]
    },
    "signatureHelp": {
        "dynamicRegistration": true,
        "signatureInformation": {
            "documentationFormat": [
                "markdown",
                "plaintext"
            ],
            "parameterInformation": {
                "labelOffsetSupport": true
            }
        }
    },

... if it is not valid code it fails. In the example there are parse errors ...

Ups, right! I've fixed the errors and hat to remove the public to make it work. It makes the code example a bit less correct, though. And you don't always want to get too verbose with these examples and surround everything with class {...} just to get completely correct code. Do you see any other way to get partial example code snippets rendered correctly?

mikehaertl commented 4 years ago

Probably related: https://github.com/neoclide/coc.nvim/issues/1704. From what I understand they say, that the filetype is not reported correctly by the language server. But no idea where this should be fixed.

bmewburn commented 4 years ago

Do you see any other way to get partial example code snippets rendered correctly?

The better approach might be to just preserve all whitespace as is. I'll look into this further.

For the markdown issue I wonder after looking at that coc.nvim thread if the client is expecting the deprecated MarkedString or MarkedString[] type as shown below. This might explain the comment about the server not sending the filetype (ie language). Intelephense sends MarkupContent which arrived in LSP 3.3 . If this is the case then I think the client should not be sending markdown in capabilities.textDocument.hover, only plaintext.

If coc.nvim cant/wont fix it then this could be fixed in coc-phpls by intercepting the intitialize request and removing markdown from capabilities.textDocument.hover.contentFormat. If onlyplaintext is requested you shouldnt get the underscores and extra backslashes but the docs should still be sent as written (ie with fenced code blocks). Or intercepting the hover result in coc-phpls and converting to a MarkedString.

/**
 * The result of a hover request.
 */
interface Hover {
    /**
     * The hover's content
     */
    contents: MarkedString | MarkedString[] | MarkupContent;

    /**
     * An optional range is a range inside a text document
     * that is used to visualize a hover, e.g. by changing the background color.
     */
    range?: Range;
}

/**
 * MarkedString can be used to render human readable text. It is either a markdown string
 * or a code-block that provides a language and a code snippet. The language identifier
 * is semantically equal to the optional language identifier in fenced code blocks in GitHub
 * issues. See https://help.github.com/articles/creating-and-highlighting-code-blocks/#syntax-highlighting
 *
 * The pair of a language and a value is an equivalent to markdown:
 * ```${language}
 * ${value}
 * ```
 *
 * Note that markdown strings will be sanitized - that means html will be escaped.
* @deprecated use MarkupContent instead.
*/
type MarkedString = string | { language: string; value: string };

export interface MarkupContent {
    /**
     * The type of the Markup
     */
    kind: MarkupKind;

    /**
     * The content itself
     */
    value: string;
}
mikehaertl commented 4 years ago

The better approach might be to just preserve all whitespace as is. I'll look into this further.

Great, thanks.

Regarding markdown it's a bit over my head to be honest. As I understand it coc.nvim doesn't render the hover content as markdown except for embedded code examples. That of course explains why it looks weird: It's mostly "raw" markdown. I don't know why this is and why this can't be fixed and if that assumption is even correct (the rendered result doesn't look completely raw either).

So any hints welcome. Maybe you can even comment on the other thread if there's something to improve on the coc.nvim side.

bmewburn commented 4 years ago

So as I understand it coc.nvim doesn't fully support markdown but if it sees a fenced code block it will render this correctly. Intelephense doesn't have a concept of partial markdown support. It's either markdown or plaintext as per the LSP. In this case the client should really declare that it only supports plaintext. The hover client capability sent during initialize should be.

"hover": {
        "dynamicRegistration": true,
        "contentFormat": [
            "plaintext"
        ]
    },

This change would be pretty simple and can be done in coc-phpls middleware.

Theres a caveat though. Intelephense will no longer construct signatures in markdown format. It will still send any markdown that a user has added themselves, but will not add any markdown to the hover result.

mikehaertl commented 4 years ago

Ok, thanks for your helpful comments. So the formatting problem can only be fixed on the coc.nvim / coc-phpls side.

If you find time I'd still appreciate any improvements with indetation of fenced code blocks, though.