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.

Sign up for GitHub

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

Handle anonymous class attribute #152

Merged
merged 10 commits into from
Feb 6, 2024
Merged

Handle anonymous class attribute #152

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
27e8ab9
Add failing test for anonymous class attribute
romm Jan 27, 2024
c8a47ac
Add more cases in anonymous class attribute test
romm Feb 1, 2024
49f8fff
Fix handling of anonymous class with attribute
romm Feb 1, 2024
368f49b
Add failing test for redefinition of new anonymous class with attribute
romm Feb 1, 2024
28c1464
Fix handling redefinition of new for anonymous class with attribute
romm Feb 1, 2024
855dc3b
Look for T_STRING on the right side of T_CLASS
antecedent Feb 5, 2024
88c50ac
Look for LEFT_ROUND | LEFT_CURLY | T_EXTENDS | T_IMPLEMENTS instead
antecedent Feb 5, 2024
3615e89
When searching for `new class`, include `new readonly class` as well
antecedent Feb 5, 2024
2abd509
Add a version-proof T_READONLY
antecedent Feb 5, 2024
bc7d1c8
When searching for `new class` (take 2), search instead for `new (cla…
antecedent Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/CodeManipulation/Actions/Generic.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ function injectCodeAfterClassDefinitions($code)
{
return function(Source $s) use ($code) {
foreach ($s->all(T_CLASS) as $match) {
if ($s->is([T_DOUBLE_COLON, T_NEW], $s->skipBack(Source::junk(), $match))) {
# Not a proper class definition: either ::class syntax or anonymous class
if ($s->is([T_DOUBLE_COLON, T_NEW, RIGHT_SQUARE], $s->skipBack(Source::junk(), $match))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This incorrectly excludes injecting code after

patchwork/tests/includes/AnonymousClassAttribute.php

Lines 3 to 6 in 28c1464

#[\Attribute]
class SomeAttribute {
public function __construct($value) {}
}

A test for that might have to actually call Patchwork\CodeManipulation\transformString() and examine the output rather than just seeing that there's no fatal error raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well observed. To be honest I have no clue how to fix the issue, how to detect that a class with attribute is anonymous or not. Do you have any idea on how to do it?

Copy link
Owner

Choose a reason for hiding this comment

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

I think that all named classes have a T_STRING token (the name) following the class token. This, however, only comes from my intuition for the time being, and could be wrong. But if it is correct, then we could get around this issue by inspecting the token on the right side of the class token, not the left side.

Copy link
Collaborator

@jrfnl jrfnl Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that all named classes have a T_STRING token (the name) following the class token. This, however, only comes from my intuition for the time being, and could be wrong. But if it is correct, then we could get around this issue by inspecting the token on the right side of the class token, not the left side.

Depends on how the tokenizer is run. Looks like it is run with the TOKEN_PARSE $flag, in which case probably yes, though results from running the tokenizer with that flag vary depending on the PHP version (then again, that is the case for token streams anyway).

In PHPCS, to determine whether the class keyword is an anonymous class or a named class, we check if the next non-empty token is one of the following, if it is, the class keyword is marked as an anonymous class:

  • ( (open parenthesis)
  • { (open curly)
  • T_EXTENDS
  • T_IMPLEMENTS

Hope that helps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally what I was thinking of doing was to $pos = $s->skipBack(Source::junk(), $match), then if it's pointing at a RIGHT_SQUARE and $s->match() for that points to a General\ATTRIBUTE, skipBack from that General\ATTRIBUTE and repeat as necessary for more RIGHT_SQUAREs. Once it finds a non-bracket (or a RIGHT_SQUARE not matching with a General\ATTRIBUTE), then look for the T_DOUBLE_COLON or T_NEW.

Although I like @jrfnl 's idea too.

Copy link
Owner

@antecedent antecedent Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I took @jrfnl 's suggestion. Both the last one, and the previous one about readonly. T_READONLY really seems to be the only token that could intervene between new and class, beside attributes:
https://github.com/nikic/PHP-Parser/blob/master/grammar/php.y#L501-L515

# Not a proper class definition: either ::class syntax or anonymous class (with or without attribute)
continue;
}
$leftBracket = $s->next(LEFT_CURLY, $match);
Expand Down
2 changes: 1 addition & 1 deletion src/CodeManipulation/Actions/RedefinitionOfNew.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ function spliceAllInstantiations(Source $s)
}
foreach ($s->all(T_NEW) as $new) {
$begin = $s->skip(Source::junk(), $new);
if ($s->is(T_CLASS, $begin)) {
if ($s->is([T_CLASS, T_ATTRIBUTE], $begin)) {
# Anonymous class
continue;
}
Expand Down
20 changes: 20 additions & 0 deletions tests/anonymous-class-attribute.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
--TEST--
Attribute declared in anonymous class

--SKIPIF--
<?php version_compare(PHP_VERSION, "8.0", ">=")
or die("skip because attributes are only available since PHP 8.0") ?>

--FILE--
<?php

error_reporting(E_ALL | E_STRICT);

require __DIR__ . "/../Patchwork.php";
require __DIR__ . "/includes/AnonymousClassAttribute.php";

?>
===DONE===

--EXPECT--
===DONE===
18 changes: 18 additions & 0 deletions tests/includes/AnonymousClassAttribute.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

#[\Attribute]
class SomeAttribute {
public function __construct($value) {}
}

// Simple attribute
new #[SomeAttribute('foo')] class {};

// Several attributes
new #[SomeAttribute('foo')] #[SomeAttribute('bar')] class {};

// Attribute with argument on several lines
new #[SomeAttribute([
'foo',
'bar',
])] class {};
22 changes: 22 additions & 0 deletions tests/redefinition-new-anonymous-class-with-attribute.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
Redefinition of new in anonymous class with attribute

--SKIPIF--
<?php version_compare(PHP_VERSION, "8.0", ">=")
or die("skip because attributes are only available since PHP 8.0") ?>

--FILE--
<?php

error_reporting(E_ALL | E_STRICT);

$_SERVER['PHP_SELF'] = __FILE__;

require __DIR__ . "/../Patchwork.php";
require __DIR__ . "/includes/AnonymousClassAttribute.php";

?>
===DONE===

--EXPECT--
===DONE===