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

support comments in array shape #213

Open
wants to merge 2 commits into
base: 1.23.x
Choose a base branch
from
Open

support comments in array shape #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f23cd90
support comments in array shape
shmax Sep 17, 2023
4dd3d9a
Tests for comment-like descriptions
ondrejmirtes Oct 9, 2023
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
2 changes: 1 addition & 1 deletion .github/workflows/apiref.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Install PHP"
uses: "shivammathur/setup-php@v2"
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/backward-compatibility.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
fetch-depth: 0

Expand Down
10 changes: 5 additions & 5 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Install PHP"
uses: "shivammathur/setup-php@v2"
Expand Down Expand Up @@ -53,10 +53,10 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Checkout build-cs"
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
repository: "phpstan/build-cs"
path: "build-cs"
Expand Down Expand Up @@ -104,7 +104,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Install PHP"
uses: "shivammathur/setup-php@v2"
Expand Down Expand Up @@ -144,7 +144,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Install PHP"
uses: "shivammathur/setup-php@v2"
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/create-tag.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
runs-on: "ubuntu-latest"
steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
fetch-depth: 0
token: ${{ secrets.PHPSTAN_BOT_TOKEN }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/merge-maintained-branch.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4
- name: "Merge branch"
uses: everlytic/[email protected]
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Generate changelog
id: changelog
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/send-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:
php-version: "8.1"

- name: "Checkout phpstan-src"
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
repository: phpstan/phpstan-src
path: phpstan-src
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/test-slevomat-coding-standard.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ jobs:

steps:
- name: "Checkout"
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: "Checkout Slevomat Coding Standard"
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
repository: slevomat/coding-standard
path: slevomat-cs
Expand Down
5 changes: 0 additions & 5 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ parameters:
count: 1
path: src/Ast/NodeTraverser.php

-
message: "#^Strict comparison using \\=\\=\\= between 2 and 2 will always evaluate to true\\.$#"
count: 2
path: src/Ast/NodeTraverser.php

-
message: "#^Variable property access on PHPStan\\\\PhpDocParser\\\\Ast\\\\Node\\.$#"
count: 1
Expand Down
2 changes: 2 additions & 0 deletions src/Ast/Attribute.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ final class Attribute

public const ORIGINAL_NODE = 'originalNode';

public const COMMENTS = 'comments';

}
31 changes: 31 additions & 0 deletions src/Ast/Comment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php declare(strict_types = 1);

namespace PHPStan\PhpDocParser\Ast;

use function trim;

class Comment
{

/** @var string */
public $text;

/** @var int */
public $startLine;

/** @var int */
public $startIndex;

public function __construct(string $text, int $startLine = -1, int $startIndex = -1)
{
$this->text = $text;
$this->startLine = $startLine;
$this->startIndex = $startIndex;
}

public function getReformattedText(): ?string
{
return trim($this->text);
}

}
4 changes: 4 additions & 0 deletions src/Lexer/Lexer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class Lexer
public const TOKEN_NEGATED = 35;
public const TOKEN_ARROW = 36;

public const TOKEN_COMMENT = 37;

public const TOKEN_LABELS = [
self::TOKEN_REFERENCE => '\'&\'',
self::TOKEN_UNION => '\'|\'',
Expand All @@ -65,6 +67,7 @@ class Lexer
self::TOKEN_OPEN_CURLY_BRACKET => '\'{\'',
self::TOKEN_CLOSE_CURLY_BRACKET => '\'}\'',
self::TOKEN_COMMA => '\',\'',
self::TOKEN_COMMENT => '\'//\'',
self::TOKEN_COLON => '\':\'',
self::TOKEN_VARIADIC => '\'...\'',
self::TOKEN_DOUBLE_COLON => '\'::\'',
Expand Down Expand Up @@ -160,6 +163,7 @@ private function generateRegexp(): string
self::TOKEN_CLOSE_CURLY_BRACKET => '\\}',

self::TOKEN_COMMA => ',',
self::TOKEN_COMMENT => '((?<![:/])\/\/[^\n]*)',
self::TOKEN_VARIADIC => '\\.\\.\\.',
self::TOKEN_DOUBLE_COLON => '::',
self::TOKEN_DOUBLE_ARROW => '=>',
Expand Down
4 changes: 1 addition & 3 deletions src/Parser/PhpDocParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,15 +1127,13 @@ private function parseAssertParameter(TokenIterator $tokens): array
{
if ($tokens->isCurrentTokenType(Lexer::TOKEN_THIS_VARIABLE)) {
$parameter = '$this';
$requirePropertyOrMethod = true;
$tokens->next();
} else {
$parameter = $tokens->currentTokenValue();
$requirePropertyOrMethod = false;
$tokens->consumeTokenType(Lexer::TOKEN_VARIABLE);
}

if ($requirePropertyOrMethod || $tokens->isCurrentTokenType(Lexer::TOKEN_ARROW)) {
if ($tokens->isCurrentTokenType(Lexer::TOKEN_ARROW)) {
$tokens->consumeTokenType(Lexer::TOKEN_ARROW);

$propertyOrMethod = $tokens->currentTokenValue();
Expand Down
45 changes: 35 additions & 10 deletions src/Parser/TokenIterator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace PHPStan\PhpDocParser\Parser;

use LogicException;
use PHPStan\PhpDocParser\Ast\Comment;
use PHPStan\PhpDocParser\Lexer\Lexer;
use function array_pop;
use function assert;
Expand All @@ -17,14 +18,19 @@ class TokenIterator
/** @var list<array{string, int, int}> */
private $tokens;

/** @var array<Comment> */
private $comments = [];

/** @var int */
private $index;

/** @var int[] */
private $savePoints = [];

/** @var list<int> */
private $skippedTokenTypes = [Lexer::TOKEN_HORIZONTAL_WS];
private $skippedTokenTypes = [
Lexer::TOKEN_HORIZONTAL_WS,
Lexer::TOKEN_COMMENT];

/** @var string|null */
private $newline = null;
Expand Down Expand Up @@ -154,8 +160,7 @@ public function consumeTokenType(int $tokenType): void
}
}

$this->index++;
$this->skipIrrelevantTokens();
$this->next();
}


Expand All @@ -168,8 +173,7 @@ public function consumeTokenValue(int $tokenType, string $tokenValue): void
$this->throwError($tokenType, $tokenValue);
}

$this->index++;
$this->skipIrrelevantTokens();
$this->next();
}


Expand All @@ -180,12 +184,30 @@ public function tryConsumeTokenValue(string $tokenValue): bool
return false;
}

$this->index++;
$this->skipIrrelevantTokens();
$this->next();

return true;
}

/**
* @return Comment[]
*/
public function flushComments(): array
Copy link
Member

Choose a reason for hiding this comment

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

i think this PR is starting to be in a pretty good shape. I still have to go through the many test cases and think about it very hard (and try to break your logic :))

Every time there's a new stateful logic introduced like here with the comments array, I try to think how we can prevent it being used wrongly.

I think TokenIterator should throw an exception if the comments weren't flushed (the array is not empty):

  1. Definitely when consuming TOKEN_END
  2. Maybe even more often, basically every time we assume the array should be empty. Like when consuming some other token than EOL and maybe others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having a look through the code, too, but so far I don't actually see any calls to consumeTokenType with TOKEN_END as an argument.

My next thought was to do a check before returning from TypeParser::parse, but that one is recursive, so TypeParser doesn't really know when it's done (without some further refactoring).

PhpDocParser::parse has some potential, but while it is possible to have comments remaining when the parsing is done, it's not clear to me if it's an error. For example, this seems like it should be valid to me:

/**
 *  @return int|false
 *
 * // can return false on failure
 * /

Since we always attach each comment to the following node, and there are no more nodes after the comment, we technically have a leftover comment, but no cause to throw an exception.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, so this comment made me hear a loud sound of breaking tableware :D

We need to make sure that comments outside of types, outside of TypeParser, are still parsed as they are today!

So for example:

/** @param int $a // this is a description */

Must become a description of ParamTagValueNode. Please be aware that there are two differents implementations of consuming descriptions: parseOptionalDescription and parseOptionalDescriptionAfterDoctrineTag. So make sure to update and test both. Thank you.

In this case:

/**
 *  @return int|false
 *
 * // can return false on failure
 * /

This must become a standalone PhpDocTextNode as today.

Can you make sure they still do?

Copy link
Contributor Author

@shmax shmax Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you make sure they still do?

Did they ever? I didn't know about parseOptionalDescription or parseOptionalDescriptionAfterDoctrineTag, but I checked out the 1.23.x branch, stuck a breakpoint in each of them, and ran the tests. Neither breakpoint was hit.

edit: disregard. Somehow I wound up inside the build-cs folder.

Can you provide a breaking test for me? Feel free to push a commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/** @param int $a // this is a description */
Must become a description of ParamTagValueNode

By the way, I'm not sure about that one. None of the samples I see in the tests look like that. They all look like this:

/** 
@param int $a this is a description 
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise we're already inside a comment. There's no structural difference between @param int $foo the number of foos to count and @param int $foo // TODO: come up with description.

There is a functional difference. Let's rewrite it a different way:

// with description
$tag = [
  'name' => '@param',
  'type' => `int`,
  'parameterName' => '$foo,
 'description` => 'the number of foos to count'
];
// with comment
$tag = [
  'name' => '@param',
  'type' => `int`,
  'parameterName' => '$foo,
 'description` => '' // TODO: come up with a description
];

Is that more clear? The first one has a description, the second one doesn't.

// outside of PHPDoc types must mean the same thing as before. If not for nothing else then at least for backward compatibility.

Can you elaborate on this? I'm not clear on what you feel is broken. Comments are currently ignored, which is what is supposed to happen. How exactly have I broken backwards-compatibility? All the existing tests pass, and comments are neatly ignored.

If you want to add the kind of complexity you have in mind later that's your business, but for now do we really have a problem?!

Copy link
Member

Choose a reason for hiding this comment

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

About backward compatibility - I've described the current behaviour which we need to keep in these tests: 0bb2fe4

AFAIK they are failing in your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How exactly have I broken backwards-compatibility?

Ah, maybe I understand. You're saying that there is code currently floating around out in the wild where there are already comments assigned to descriptions and being interpreted as descriptions, and your concern is that after this change they will lose their description status, is that right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can't change the AST like that. It'd also make altering these comments more difficult and diferent for the users of format-preserving printer, most notably Slevomat CS.

Copy link
Contributor Author

@shmax shmax Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Okay. At this point I am going to abandon this PR. I tried. I feel that the behavior in this PR is correct. I understand it may "break" descriptions out in the wild, but I would argue that the code out in the wild is incorrect, and this fixes it. That happens sometimes in the software development cycle, and the solution is to do a major version release, not add tons of complexity trying to achieve backwards compatibility with existing behavior that is wrong.

I'm going to use my own fork, which I really hate to do, but I've done everything I can within reason to solve this problem rationally, and I'm not getting support. Good luck.

{
$res = $this->comments;
$this->comments = [];
return $res;
}

/** @phpstan-impure */
public function tryConsumeTokenTypeAll(int $tokenType): bool
{
$found = false;
while ($this->tryConsumeTokenType($tokenType)) {
$found = true;
}
return $found;
}

/** @phpstan-impure */
public function tryConsumeTokenType(int $tokenType): bool
Expand All @@ -200,8 +222,7 @@ public function tryConsumeTokenType(int $tokenType): bool
}
}

$this->index++;
$this->skipIrrelevantTokens();
$this->next();

return true;
}
Expand Down Expand Up @@ -256,6 +277,11 @@ private function skipIrrelevantTokens(): void
if (!isset($this->tokens[$this->index + 1])) {
break;
}

if ($this->currentTokenType() === Lexer::TOKEN_COMMENT) {
$this->comments[] = new Comment($this->currentTokenValue(), $this->currentTokenLine(), $this->currentTokenIndex());
}

$this->index++;
}
}
Expand Down Expand Up @@ -299,7 +325,6 @@ public function rollback(): void
$this->index = $index;
}


/**
* @throws ParserException
*/
Expand Down
Loading