Skip to content

Commit

Permalink
Use visitor to assign comments
Browse files Browse the repository at this point in the history 
This fixes the long-standing issue where a comment would get assigned
to all nodes with the same starting position, instead of only the
outer-most one.

Fixes #253.
  • Loading branch information
@nikic
nikic committed Sep 28, 2023
1 parent de84f76 commit 4e27a17
Show file tree
Hide file tree
Showing 23 changed files with 97 additions and 371 deletions.
82 changes: 82 additions & 0 deletions lib/PhpParser/NodeVisitor/CommentAnnotatingVisitor.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
<?php declare(strict_types=1);

namespace PhpParser\NodeVisitor;

use PhpParser\Comment;
use PhpParser\Node;
use PhpParser\NodeVisitorAbstract;
use PhpParser\Token;

class CommentAnnotatingVisitor extends NodeVisitorAbstract {
/** @var int Last seen token start position */
private int $pos = 0;
/** @var Token[] Token array */
private array $tokens;
/** @var list<int> Token positions of comments */
private array $commentPositions = [];

/**
* Create a comment annotation visitor.
*
* @param Token[] $tokens Token array
*/
public function __construct(array $tokens) {
$this->tokens = $tokens;

// Collect positions of comments. We use this to avoid traversing parts of the AST where
// there are no comments.
foreach ($tokens as $i => $token) {
if ($token->id === \T_COMMENT || $token->id === \T_DOC_COMMENT) {
$this->commentPositions[] = $i;
}
}
}

public function enterNode(Node $node) {
$nextCommentPos = current($this->commentPositions);
if ($nextCommentPos === false) {
// No more comments.
return self::STOP_TRAVERSAL;
}

$oldPos = $this->pos;
$this->pos = $pos = $node->getStartTokenPos();
if ($nextCommentPos > $oldPos && $nextCommentPos < $pos) {
$comments = [];
while (--$pos >= $oldPos) {
$token = $this->tokens[$pos];
if ($token->id === \T_DOC_COMMENT) {
$comments[] = new Comment\Doc(
$token->text, $token->line, $token->pos, $pos,
$token->getEndLine(), $token->getEndPos() - 1, $pos);
continue;
}
if ($token->id === \T_COMMENT) {
$comments[] = new Comment(
$token->text, $token->line, $token->pos, $pos,
$token->getEndLine(), $token->getEndPos() - 1, $pos);
continue;
}
if ($token->id !== \T_WHITESPACE) {
break;
}
}
if (!empty($comments)) {
$node->setAttribute('comments', array_reverse($comments));
}

do {
$nextCommentPos = next($this->commentPositions);
} while ($nextCommentPos !== false && $nextCommentPos < $this->pos);
}

$endPos = $node->getEndTokenPos();
if ($nextCommentPos > $endPos) {
// Skip children if there are no comments located inside this node.
$this->pos = $endPos;
return self::DONT_TRAVERSE_CHILDREN;
}

return null;
}
}
40 changes: 15 additions & 25 deletions lib/PhpParser/ParserAbstract.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use PhpParser\Node\Stmt\Property;
use PhpParser\Node\Stmt\TryCatch;
use PhpParser\Node\UseItem;
use PhpParser\NodeVisitor\CommentAnnotatingVisitor;

abstract class ParserAbstract implements Parser {
private const SYMBOL_NONE = -1;
Expand Down Expand Up @@ -201,6 +202,11 @@ public function parse(string $code, ?ErrorHandler $errorHandler = null): ?array
$this->semValue = null;
$this->createdArrays = null;

if ($result !== null) {
$traverser = new NodeTraverser(new CommentAnnotatingVisitor($this->tokens));
$traverser->traverse($result);
}

return $result;
}

Expand Down Expand Up @@ -473,19 +479,14 @@ protected function getExpectedTokens(int $state): array {
protected function getAttributes(int $tokenStartPos, int $tokenEndPos): array {
$startToken = $this->tokens[$tokenStartPos];
$afterEndToken = $this->tokens[$tokenEndPos + 1];
$attributes = [
return [
'startLine' => $startToken->line,
'startTokenPos' => $tokenStartPos,
'startFilePos' => $startToken->pos,
'endLine' => $afterEndToken->line,
'endTokenPos' => $tokenEndPos,
'endFilePos' => $afterEndToken->pos - 1,
];
$comments = $this->getCommentsBeforeToken($tokenStartPos);
if (!empty($comments)) {
$attributes['comments'] = $comments;
}
return $attributes;
}

/**
Expand All @@ -500,19 +501,14 @@ protected function getAttributesForToken(int $tokenPos): array {

// Get attributes for the sentinel token.
$token = $this->tokens[$tokenPos];
$attributes = [
return [
'startLine' => $token->line,
'startTokenPos' => $tokenPos,
'startFilePos' => $token->pos,
'endLine' => $token->line,
'endTokenPos' => $tokenPos,
'endFilePos' => $token->pos,
];
$comments = $this->getCommentsBeforeToken($tokenPos);
if (!empty($comments)) {
$attributes['comments'] = $comments;
}
return $attributes;
}

/*
Expand Down Expand Up @@ -902,35 +898,31 @@ protected function createCommentFromToken(Token $token, int $tokenPos): Comment
}

/**
* Get comments before the given token position.
*
* @return Comment[] Comments
* Get last comment before the given token position, if any
*/
protected function getCommentsBeforeToken(int $tokenPos): array {
$comments = [];
protected function getCommentBeforeToken(int $tokenPos): ?Comment {
while (--$tokenPos >= 0) {
$token = $this->tokens[$tokenPos];
if (!isset($this->dropTokens[$token->id])) {
break;
}

if ($token->id === \T_COMMENT || $token->id === \T_DOC_COMMENT) {
$comments[] = $this->createCommentFromToken($token, $tokenPos);
return $this->createCommentFromToken($token, $tokenPos);
}
}
return \array_reverse($comments);
return null;
}

/**
* Create a zero-length nop to capture preceding comments, if any.
*/
protected function maybeCreateZeroLengthNop(int $tokenPos): ?Nop {
$comments = $this->getCommentsBeforeToken($tokenPos);
if (empty($comments)) {
$comment = $this->getCommentBeforeToken($tokenPos);
if ($comment === null) {
return null;
}

$comment = $comments[\count($comments) - 1];
$commentEndLine = $comment->getEndLine();
$commentEndFilePos = $comment->getEndFilePos();
$commentEndTokenPos = $comment->getEndTokenPos();
Expand All @@ -941,14 +933,12 @@ protected function maybeCreateZeroLengthNop(int $tokenPos): ?Nop {
'endFilePos' => $commentEndFilePos,
'startTokenPos' => $commentEndTokenPos + 1,
'endTokenPos' => $commentEndTokenPos,
'comments' => $comments,
];
return new Nop($attributes);
}

protected function maybeCreateNop(int $tokenStartPos, int $tokenEndPos): ?Nop {
$comments = $this->getCommentsBeforeToken($tokenStartPos);
if (empty($comments)) {
if ($this->getCommentBeforeToken($tokenStartPos) === null) {
return null;
}
return new Nop($this->getAttributes($tokenStartPos, $tokenEndPos));
Expand Down
3 changes: 0 additions & 3 deletions test/code/parser/blockComments.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ array(
0: Stmt_Expression(
expr: Expr_Variable(
name: a
comments: array(
0: // baz
)
)
comments: array(
0: // baz
Expand Down
6 changes: 0 additions & 6 deletions test/code/parser/comments.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,6 @@ array(
0: Stmt_Expression(
expr: Expr_Variable(
name: var
comments: array(
0: /** doc 1 */
1: /* foobar 1 */
2: // foo 1
3: // bar 1
)
)
comments: array(
0: /** doc 1 */
Expand Down
3 changes: 0 additions & 3 deletions test/code/parser/expr/arrayDef.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,6 @@ array(
expr: Expr_Array(
items: array(
)
comments: array(
0: // short array syntax
)
)
comments: array(
0: // short array syntax
Expand Down
33 changes: 0 additions & 33 deletions test/code/parser/expr/assign.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,10 @@ array(
expr: Expr_Assign(
var: Expr_Variable(
name: a
comments: array(
0: // simple assign
)
)
expr: Expr_Variable(
name: b
)
comments: array(
0: // simple assign
)
)
comments: array(
0: // simple assign
Expand All @@ -60,16 +54,10 @@ array(
expr: Expr_AssignOp_BitwiseAnd(
var: Expr_Variable(
name: a
comments: array(
0: // combined assign
)
)
expr: Expr_Variable(
name: b
)
comments: array(
0: // combined assign
)
)
comments: array(
0: // combined assign
Expand Down Expand Up @@ -199,9 +187,6 @@ array(
expr: Expr_Assign(
var: Expr_Variable(
name: a
comments: array(
0: // chained assign
)
)
expr: Expr_AssignOp_Mul(
var: Expr_Variable(
Expand All @@ -216,9 +201,6 @@ array(
)
)
)
comments: array(
0: // chained assign
)
)
comments: array(
0: // chained assign
Expand All @@ -228,16 +210,10 @@ array(
expr: Expr_AssignRef(
var: Expr_Variable(
name: a
comments: array(
0: // by ref assign
)
)
expr: Expr_Variable(
name: b
)
comments: array(
0: // by ref assign
)
)
comments: array(
0: // by ref assign
Expand All @@ -256,16 +232,10 @@ array(
unpack: false
)
)
comments: array(
0: // list() assign
)
)
expr: Expr_Variable(
name: b
)
comments: array(
0: // list() assign
)
)
comments: array(
0: // list() assign
Expand Down Expand Up @@ -349,9 +319,6 @@ array(
var: Expr_Variable(
name: a
)
comments: array(
0: // inc/dec
)
)
comments: array(
0: // inc/dec
Expand Down
6 changes: 0 additions & 6 deletions test/code/parser/expr/exprInIsset.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ array(
name: b
)
)
comments: array(
0: // This is legal.
)
)
comments: array(
0: // This is legal.
Expand All @@ -37,9 +34,6 @@ array(
)
)
)
comments: array(
0: // This is illegal, but not a syntax error.
)
)
comments: array(
0: // This is illegal, but not a syntax error.
Expand Down
12 changes: 0 additions & 12 deletions test/code/parser/expr/exprInList.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,10 @@ array(
unpack: false
)
)
comments: array(
0: // This is legal.
)
)
expr: Expr_Variable(
name: x
)
comments: array(
0: // This is legal.
)
)
comments: array(
0: // This is legal.
Expand All @@ -62,16 +56,10 @@ array(
unpack: false
)
)
comments: array(
0: // This is illegal, but not a syntax error.
)
)
expr: Expr_Variable(
name: x
)
comments: array(
0: // This is illegal, but not a syntax error.
)
)
comments: array(
0: // This is illegal, but not a syntax error.
Expand Down

0 comments on commit 4e27a17

Please sign in to comment.