Add scope for PHP primitive types

[bakerkretzmar]

PHP has 10 "primitive" types, 8 of which are relevant to this PR because they can be used as argument typehints: bool, int, float, string, array, object, iterable, and callable.

These primitive types are not classes, but they can be used in some of the same ways as class names, notably for typed properties, parameter types, and return types:

class Car
{
    public int $wheels = 4;

    public function addWheels(int $wheels): int
    {
        $this->wheels = $this->wheels + $wheels;

        return $this->wheels;
    }
}

Description of the Change

This PR adds the storage.primitive.php scope for the 8 primitive types listed above. This scope has been added everywhere these types might appear, including in function parameter typehints, object method parameter typehints, return types, typed properties, and typecasts.

I chose storage.primitive.php as the scope name because it's short, descriptive, and very similar to the existing storage.type.php, but I welcome suggestions if it makes more sense to call it something else.

Alternate Designs

I considered changing existing scopes to accomplish this rather than adding a new one, but decided not to so that this change won't break any existing scopes or colour themes—this PR is completely backwards-compatible.

Benefits

Among other things, this will allow users of Atom and VS Code to style typehints differently based on whether they are class names or primitive types.

Here's VS Code right now:

Note that Request is the name of a class imported at the top of the file. It's purple just like bool, and before this PR there was no way to change this or style primitive typehints and class names differently.

For comparison, here's Sublime:

Sublime's grammars and scopes are quite different, but the point is that the Request class typehint looks like a class name, and the bool primitive typehint looks like any other keyword.

Possible Drawbacks

None that I can see.

Applicable Issues

Closes #387. See also microsoft/vscode#95029.

It's worth noting that the styling thing is, confusingly, already possible for the array and callable types only, because they're tokenized different from any other typehints. I think this is because their default arguments could contain parentheses (e.g. array()) but I would consider this a bug—right now array parameter typehints get the scope meta.function.parameter.array.php, but not meta.function.parameter.typehinted.php, even though they are, of course, typehinted.

[KapitanOczywisty]

@bakerkretzmar storage.type.primitive.php would be more appropriate in this case.

Have you tried to use updated grammar in vscode with themes? You can use this repo and change two lines to make own grammar for vscode based on updated branch.

[bakerkretzmar]

I updated the scope to storage.type.primitive.php, thanks! Just tried setting it up locally in VS Code and it works great:

Request is scoped as it normally is and bool and int both have the additional storage.type.primitive.php scope, so they can now be styled separately!

[bakerkretzmar]

@KapitanOczywisty is there anything else you need from me here?

[KapitanOczywisty]

@bakerkretzmar Looks fine to me, but you need to wait for someone from atom itself to check and merge pr and this can take a while.

You can publish your version of vscode extension to github for anybody wanting to test changes.

[Ingramz]

I tried to review this, it's mostly nitpicky. Work on the styling of quotes and see if the suggestion of replacing \\b is viable. After that I don't think there is anything that stops it from getting merged.

[Ingramz]

I have not tried this (especially in VS Code), but since you are only using this for nested matching, you could try using ^ and $ instead of the two \\b. If it works exactly as before, you should consider using that form instead.

While \b is mostly "fine", it's somewhat tricky thing to get right and sets a poor example to others. I cannot think of specific counterexamples from top of my head, but historically we have had to fix poor usages of \b here, which is why I suggest avoiding it altogether whenever the replacement is trivial to understand.

[KapitanOczywisty]

@Ingramz I'm not sure how unicode is treated in vscode, but in atom I couldn't abuse this with stuff like stringʕ•ᴥ•ʔ. However, this might be a problem in vscode or some other editor.

Although it's rather edge case, replacing \b with ^...$ is a good idea.

[Ingramz]

One possible example is using emojis like int🐱.

[bakerkretzmar]

Thanks! I basically copy-pasted from other parts of the syntax to start with, so I appreciate this advice.

All the tests pass with ^(...)$ but interestingly, this doesn't work in VS Code. No clue why, it just doesn't tokenize properly. But if I remove the capturing group, so it really is just ^...$ (no parentheses) then it works as expected.

Is there any reason to keep those, or is removing them as I've done now fine?

[KapitanOczywisty]

Without parentheses you have equivalent of: (^array)|bool|callable|float|int|iterable|object|(string$) so only array and string are broken then.

Okay I dived into this and found cause microsoft/vscode-textmate#74 tl;dr only $ is working somewhat correctly and all because substring(line start, capture end) is used for match, thus any start anchors are useless there.

Even if ^..$ are better, implementation makes them useless in vscode.

[bakerkretzmar]

Interesting. So would something like (^array$)|(^bool$)|(^callable$)|... work here? Or would it still break because ^ doesn't work properly at all?

@Ingramz it looks like \\b might be simpler after all if that's okay with you! 😂

[KapitanOczywisty]

Yes, ^ is broken. And yes, in that case \b is better. I wasn't aware of that until now and this explains some weird behaviors which I had in the past.

[Ingramz]

Since it is broken in vscode then we have no other choice than to use \b. Thank you for trying it out and @KapitanOczywisty for finding the relevant issue. Rest of the pull request looks great now. I am however annoyed by this vscode's deviation from spec.

[bakerkretzmar]

Yeah that's frustrating. I've changed it back to \b.

[bakerkretzmar]

@KapitanOczywisty @Ingramz hope you're both well, thanks again for your help with this PR. Is there anything else you need from me, or anything I can do to help get it merged?

[KapitanOczywisty]

@bakerkretzmar So.. I'll break your PR. 😛
I'm working on updates for php 8.0 and after digging through whole code seems that storage.type.php should be exactly what you're doing with primitives and classes are wrongly tokenized in few places (e.g. in parameters) as storage.type.php instead of support.class.php. With my PR this will no longer be the case, and probably this will solve your issue. Sorry, not sorry 😉