pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard

MEGA-ISSUE: Syntax highlighting in `language-php`

Open savetheclocktower opened this issue 2 years ago • 63 comments

IMPORTANT: Some issues have already been fixed!

If you’re still on the regular 1.113 release, you might be suffering from a problem that has already been fixed. Many fixes landed on master in #859. You are encouraged to download the latest rolling release — both (a) to see whether what you’re reporting has been fixed, and (b) so that you can enjoy the fixes that have been reported since the 1.113 release.


This will serve as a catch-all issue for any problems you may encounter with syntax highlighting in PHP files (language-php). If you have problems with the new syntax highlighting (or folds, or indents) that are specific to php files, keep reading.

Something isn't highlighting correctly!

If there are any highlighting regressions since 1.113:

First, please scroll down and see if someone else has reported the issue. If not, please comment with

  • A small amount of sample code to reproduce the issue
  • Screenshots (before and after would be ideal, but just the “after” is OK for obvious things

I want to go back to the old highlighting!

You can easily opt out of the new Tree-sitter highlighting for any language, but first please:

  • subscribe to this issue so you'll know when the problem is fixed
  • remove the config change when the next release comes out so that you can enjoy the new grammar once again

To revert to the TextMate-style grammar for this language only, add the following to your config.cson:

".text.html.php":
  core:
    useTreeSitterParsers: false

savetheclocktower avatar Jan 19 '24 00:01 savetheclocktower

@croaker000 reports occasional failures with injection of HTML into PHP files. We're working to establish reproduction steps. If you can get this issue to reproduce consistently, please add a comment and tell us how.

savetheclocktower avatar Jan 19 '24 00:01 savetheclocktower

A user on Discord reported that constructors and some builtin functions were no longer highlighted like functions. For instance, Bar and unset below were highlighted like plain text:

$foo = new Bar();
unset($baz);

This has been fixed in #859, specifically in this commit. At the latest it'll be released as part of 1.114, but we might be able to get it out sooner as part of a rolling release. If so, it'll be announced in this ticket.

savetheclocktower avatar Jan 19 '24 01:01 savetheclocktower

Legacy tree-sitter folding allowed for folding within commented sections. We can no longer do this image In the screenshot, hovering over line 30 toggles the folding twistie. Hovering over line 36 does not (it used to)

croaker000 avatar Jan 19 '24 12:01 croaker000

At some locations, not all, pressing [enter] at the end of a line erroneously indents the cursor on the new line image

croaker000 avatar Jan 19 '24 13:01 croaker000

At some locations, not all, pressing [enter] at the end of a line erroneously indents the cursor on the new line

I can confirm this on the latest #859. @croaker000 is there any chance that line 4 in your example using a ternary? I am seeing:

# indent is wrong
$id = (foo(1) ?: 1);
$id = (foo(1) ? 1 : 2);
$id = [(foo(1) ?: 1)];
$id = 1 ? 1 : 1;

# indent seems fine
$id = (foo(1));
$id = (foo(1) ?? 2);
$id = [];

Re the highlighting issue you saw w/ HTML+PHP templates, I have not seen that happen yet, personally. I spent all day yesterday in such templates w/ the new parser and they always behaved fine for me. I'll keep an eye out for it, too, though. Does it just happen "on it's own", or is it after you've altered the buffer or added any new code? Does removing any code seem to fix it? If I'm reading between the lines of @savetheclocktower comment, it could be that something in your file is triggering an uncommon code path in the bundled PHP parser, and that might be using something that wasn't accounted for when the parser was bundled in Pulsar. So if it only happens when you use certain syntax or something like that, that may be the case.

Also, @savetheclocktower could the issue be in the HTML parser and not in PHP? In the posted screenshot, the 2 chunks of PHP appear to be highlighted correctly. It's just the HTML in between that isn't. Perhaps the injection is working fine, but the HTML parser is falling over when trying to deal with that part of the document? @croaker000 is all of the HTML in the whole file losing hightlights, or just certain blocks of it between <?php ... ?> tags?

claytonrcarter avatar Jan 19 '24 14:01 claytonrcarter

Yes, I can duplicate the indenting problem with ternary lines and seems OK for other lines.

HTML/PHP mixed files have been working fine for me today. Yesterday (on a different set of similar files) was a nightmare until I switched back to the legacy tree-sitter. It seemed to happen randomly and become visible at the point of opening a file. After which, all other open files displayed the same problem. I wasn't in a debug mindset at the time so it's tricky to be definitive. I'm waiting for the problem to crop up again so I can try to duplicate it.

Yes, all the HTML in an affected document loses highlighting but snippets and code folding stop working at the same time for all code types.

croaker000 avatar Jan 19 '24 14:01 croaker000

In the screenshot, hovering over line 30 toggles the folding twistie. Hovering over line 36 does not (it used to)

That is ridiculous and I had no idea it was even present.

I imagine that we could probably eventually detect a span of consecutive line comments and convert them into a single fold, but folding a commented-out function is not a feature I can reasonably provide you in the near future.

Meanwhile, those ternary test cases are useful. I can have that indentation problem fixed shortly.

savetheclocktower avatar Jan 20 '24 02:01 savetheclocktower

Indentation fix now present in #859.

Also, @savetheclocktower could the issue be in the HTML parser and not in PHP? In the posted screenshot, the 2 chunks of PHP appear to be highlighted correctly. It's just the HTML in between that isn't.

It's possible. The root language layer is tree-sitter-php, which identifies and distinguishes the HTML parts from the PHP parts. The HTML grammar is then injected into the HTML parts.

So the PHP parser could be screwing up at marking the HTML nodes as HTML nodes, or the HTML grammar could be screwing up at parsing the HTML parts as HTML.

Or it could be a bug in the injection handling. For instance, if we fail to determine the correct range(s) for the injection and fall victim to an off-by-one error, it could throw off the HTML parser entirely.

savetheclocktower avatar Jan 20 '24 02:01 savetheclocktower

folding a commented-out function is not a feature I can reasonably provide you in the near future.

That's a huge shame. I've found that massively useful for my professional work every since I began using Atom.

croaker000 avatar Jan 20 '24 12:01 croaker000

Hello. I'd like to report the followings.

Sample code
<?php
/**/
/***/
/** */
/****/

/** @var string $str */
/** @var array $arr */
/** @var array<mixed> $arr */
/** @var array<string> $arrOfStr */
/** @var array<object> $arrOfObj */
/** @var array<array> $arrOfArr */
/** @var array<array<mixed>> $arrOfArr */
/** @var array<array<string>> $arrOfArrOfStr */
/** @var array<array<object>> $arrOfArrOfObj */
/** @var object $obj */
/** @var stdClass $obj */

intdiv(num1: $dvdnd, num2: $dvsr);
str_pad(string: $str, length: 10, pad_string: "0");
ksort(array: $arr, flags: SORT_REGULAR);

Screenshots

Modern:

image

Old:

image

Digitalone1 avatar Jan 20 '24 14:01 Digitalone1

That's a huge shame. I've found that massively useful for my professional work every since I began using Atom.

The good news is that this feature isn't going away; it's just not something we can do in a Tree-sitter grammar. The existing PHP TextMate-style grammar that you've used for years will still be able to do this. Keep the setting override in your config.cson indefinitely and you'll always get the TextMate-style grammar.

The older Tree-sitter grammars are the ones that will be going away, but PHP never even had one of those.

savetheclocktower avatar Jan 20 '24 18:01 savetheclocktower

Hello. I'd like to report the followings.

@claytonrcarter, these lines cause errors in the PHPDoc parsing:

/** @var array<array<mixed>> $arrOfArr */
/** @var array<array<string>> $arrOfArrOfStr */
/** @var array<array<object>> $arrOfArrOfObj */

Are these legal PHPDoc constructs?

Meanwhile, @Digitalone1, we'll make sure that the PHPDoc syntax is changed to look much more like what it used to be, and that comment fragments like /***/ are handled properly. Thanks for the report!

savetheclocktower avatar Jan 20 '24 18:01 savetheclocktower

@Digitalone1, this commit should fix most of what you're seeing in that code example. However, I wasn't able to reproduce the lack of highlighting of variables in the PHPDoc snippet:

Screenshot 2024-01-20 at 10 37 15 AM

Are you using One Dark? If so, can you put the cursor in the middle of one of those gray variables, then open the command palette (Ctrl-Shift-P or Cmd-Shift-P and run the Editor: Log Cursor Scope command? Please copy the text of the notification that appears, or else take a screenshot, and show it to us.

savetheclocktower avatar Jan 20 '24 18:01 savetheclocktower

@garrettboone reported in #887 that the PHP grammar is using HTML comment delimiters inside of PHP sections.

This happened because we weren't annotating any parts of the buffer with the root source.php scope of PHP regions. Since that grammar has no highlights query — it's a long story! — we incorrectly assumed we could skip scope annotation altogether. But all injection language layers need the opportunity to apply their root scope.

The fix is now present in #859.

savetheclocktower avatar Jan 20 '24 21:01 savetheclocktower

/** @var array<array<mixed>> $arrOfArr */
/** @var array<array<string>> $arrOfArrOfStr */
/** @var array<array<object>> $arrOfArrOfObj */

Are these legal PHPDoc constructs?

Short story: no. Long story: yes. As I recall, they are not valid phpdoc in the literal sense of phpdocumentor, but – since these are all just comments anyway – static analysis tools have "extended" phpdoc to allow for things like this. I just checked, and phpstan knows what @return array<array<array<int>>> means. There is a fix pending at https://github.com/claytonrcarter/tree-sitter-phpdoc/pull/29, but CI isn't cooperating at the moment.

claytonrcarter avatar Jan 20 '24 22:01 claytonrcarter

I pushed this commit about an hour ago. Now each <?php … ?> range should have the source.php scope and either a meta.embedded.line.php scope or a meta.embedded.block.php scope depending on whether it spans more than one line. This is pretty important if your syntax theme does something special with meta.embedded, and it's a feature I lean on a lot to be able to tell PHP, EJS, ERB, etc., from their surrounding HTML.

All the tests pass, but of course the language bundles need more test coverage, so let me know if you run into any situations where your PHP editing experience goes sideways. This would match some of the symptoms already reported: some part of the document, typically the HTML, would suddenly lose syntax highlighting. If it happens to you, please try to notice what you were doing when it happened so I can try to replicate it myself.

I'm pretty confident about what I've done here; I think it's no more likely to fail than the previous approach, and I couldn't have delivered the meta.embedded stuff without it. But we'll see!

savetheclocktower avatar Jan 21 '24 05:01 savetheclocktower

I had to push a couple of fixes to the new system I described above. I also noticed that tree-sitter-php has seen some major updates even since I last updated our copy about a month ago, so I brought us in sync with their master branch.

There's now a “php-only” version of the grammar which we could theoretically be using on the injected PHP layer instead of re-injecting the top-level PHP parser (the one that also identifies HTML), but I can't think of a reason to do this. At least it's there should we ever find a use for it.

savetheclocktower avatar Jan 21 '24 21:01 savetheclocktower

I'm trying out Windows.Pulsar-1.113.2024012105-win with Use Tree-sitter Parsers enabled and the php files are gray-text. In dev tools elements are appearing like this:

image

When I comment the line, it's behaving as html: image

When I disabled use of Tree-Sitter Parsers the file appears and behaves as expected. Element is showing:

image

garrettboone avatar Jan 22 '24 14:01 garrettboone

@garrettboone, give the latest build a shot — the one on this page. Apologies; PHP wasn't working on CI builds for most of Sunday because of an error I made.

savetheclocktower avatar Jan 22 '24 16:01 savetheclocktower

Using 1.113.2024012204 the behavior was the same (with packages running). I decided to investigate further with safe mode using a local file.

  • When starting in safe mode with the file previously opened in the project and TSP enabled it handles the php file ok.
  • If I then toggle TSP to disabled the php file flashes and again seems ok.
  • If I then toggle TSP to enabled the file turns gray-text with html commenting observed.
  • If I restart with file open in the project and TSP enable, then file is behaving properly.
  • However, if I close the php file, and close the app with TSP enabled, then open the app and open the file, the file is gray-text. But disabling TSP will return the file to proper processing.

garrettboone avatar Jan 22 '24 20:01 garrettboone

OK, I'll take a look in a few hours with the latest CI build, using my own WordPress project as sample code.

savetheclocktower avatar Jan 22 '24 21:01 savetheclocktower

I'm having no luck reproducing those symptoms on a CI build on my Mac, so I might try tomorrow on my seldom-used Windows machine.

savetheclocktower avatar Jan 23 '24 02:01 savetheclocktower

@savetheclocktower I installed on a fresh user profile with no packages installed except what comes with it, and could not reproduce the above behavior.

I am concluding it must either be something about my user profile and environment, or packages installed. But would an installed package affect safe mode?

From a user updating standpoint, I would like to help pinpoint what the issue is. Any suggested steps?

  • I will next try backing up my app data and settings and doing a fresh install on my user profile.
  • I will then try to reinstall packages on fresh install

BTW I'm using the folder as a standalone/portable vs installation using the executable.

garrettboone avatar Jan 23 '24 15:01 garrettboone

@savetheclocktower I installed on a fresh user profile with no packages installed except what comes with it, and could not reproduce the above behavior.

I am concluding it must either be something about my user profile and environment, or packages installed. But would an installed package affect safe mode?

From a user updating standpoint, I would like to help pinpoint what the issue is. Any suggested steps?

* I will next try backing up my app data and settings and doing a fresh install on my user profile.

* I will then try to reinstall packages on fresh install

BTW I'm using the folder as a standalone/portable vs installation using the executable.

In fairness, I have noticed issues with hot-swapping CI releases into place where a regular release used to be. I think it's partly related to window state. You can clear window state by launching from the command line (at least on macOS) with the --clear-window-state flag.

I freely admit that the startup process in Pulsar is a mystery to me, and I'd like to understand it better. But I'm pretty sure “window state” encompasses some things about text-buffer storage that feel magical when they work (like undo history persisting across window launches!) but which have personally (I think) been the cause of very strange performance issues when I upgrade versions.

I notice people deleting/moving their whole .pulsar directory pretty often, and I think that should be a last resort. If clearing window state doesn't work in the future, try just backing up/deleting the files inside blob-store, compile-cache, and storage before starting over with a fresh profile. I seriously doubt this has to do with any of your community packages.

savetheclocktower avatar Jan 23 '24 17:01 savetheclocktower

@savetheclocktower After moving folders and copying back config files and packages, it this particular issue seems to be working fine, thanks for the help!

garrettboone avatar Jan 24 '24 03:01 garrettboone

Minor nitpick I noticed today: I think that these lines in the PHP highlights are too aggressive.

((name) @constant.other.php
  (#match? @constant.other.php "^_?[A-Z][A-Z\\d_]+$"))

What I found was that FOO was scoped as constant.other.php in all of the following cases:

use FOO\Bar;
use Bar\FOO;

FOO::bar();
Bar::FOO();

class FOO extends Controller {}

I would expect it to only be scoped as a constant in the following cases:

const FOO = 1;
$a = FOO;
bar(FOO);
// etc

Granted, it's pretty uncommon to name classes and namespaces in all caps. I only noticed it when using SDK as a namespace.

There is a fix pending at https://github.com/claytonrcarter/tree-sitter-phpdoc/pull/29, but CI isn't cooperating at the moment.

This has been merged to master BTW, in case you feel motivated to fix /** @var array<array<mixed>> $arrOfArr */ et al.

claytonrcarter avatar Jan 24 '24 20:01 claytonrcarter

I'll comment out that query for now. I forgot I'd written that. User-defined constants honestly should go into variable, but with enough detail in the scope name that a theme can choose to color them like other constants if they want.

I didn't even realize the const statement existed; I was brought up in the define days. I'll make sure the FOO in const FOO = 1 is scoped as variable.constant.other.php or something like that.

This has been merged to master BTW, in case you feel motivated to fix /** @var array<array<mixed>> $arrOfArr */ et al.

Thanks! I'll put that on my to-do list also.

savetheclocktower avatar Jan 24 '24 21:01 savetheclocktower

@savetheclocktower Sorry for the late reply, I've been busy.

/** @var array<array<mixed>> $arrOfArr */
/** @var array<array<string>> $arrOfArrOfStr */
/** @var array<array<object>> $arrOfArrOfObj */

Are these legal PHPDoc constructs?

I don't know if they're fully legal, but Pulsar 1.112 it's handling them better anyway.

Are you using One Dark?

No, I'm using Dark One Dark. I also had to reinstall Pulsar 1.112 because I tried to disable the new syntax but I didn't manage to make it. Yes, I followed the open post and restarted Pulsar many times, but the only way to get the old syntax was to downgrade the package.

I also discovered another issue with PHP string heredoc syntax. This is occurring with Pulsar 1.112, but I think it's reproducible also in 1.113.

Take the following sample code:

<?php
class ClassName extends AnotherClass
{
    
    function __construct(argument)
    {
        // code...
    }

    public function FunctionName(): string
    {
        $str = <<<EOT
String in heredoc syntax.
EOT;
        
        return $str;
    }
}

And try to collapse the class. I get this:

image

Digitalone1 avatar Jan 25 '24 10:01 Digitalone1

Updated to 1.113.2024012906 today and PHP/HTML/JS mixed files are still losing their non-PHP syntax decorations at some random point through a session. Freshly opening a new instance starts out fine, once the decorations are lost, they stay lost until Pulsar is shutdown and restarted. This affects individual files only. eg. If I have x2 files open and one breaks, the other is fine. This state persists between opening and closing the file(s) within the session. View->Developer->Reload Window restores the correct syntax decorations. Can confirm that there are no new error messages in the console when the decorations stop working.

croaker000 avatar Jan 29 '24 15:01 croaker000

My only theory on that — that an unusual parse state causes a failure — must be incorrect, or else there would be a message in the console.

If you're willing to help debug this, I'd like you to install tree-sitter-tools. The next time this happens, you could run the Open Inspector For Editor command and verify:

  • That the initial view shows something that appears to be a tree of PHP without any ERROR nodes (assuming the buffer contents are valid PHP at that point)
  • That you can find a source.php layer in the drop-down menu near the top left
  • That, when you switch to that menu, you see another tree of PHP
  • That, when you check the “Show injection ranges” checkbox on that source.php layer, you see annotations in the editor that appear to correspond to the areas of your buffer that contain PHP

savetheclocktower avatar Jan 29 '24 17:01 savetheclocktower