Skip to content

Commit eab17f3

Browse files
TysonAndreTysonAndre
committed
WIP (Broken) Optimize getDiagnostics
Unit tests are failing Add a benchmarking script improve benchmark Extreme optimization, sacrificing code style. (is this correct?) This commit changes time to generate diagnostics from 0.010 to 0.007 seconds. WIP Fix undefined imports
1 parent 0c1765f commit eab17f3

11 files changed

+299
-123
lines changed

‎benchmark_get_diagnostics.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<?php
2+
// Autoload required classes
3+
require __DIR__ . "/vendor/autoload.php";
4+
5+
use Microsoft\PhpParser\{DiagnosticsProvider, Node, Parser, PositionUtilities};
6+
7+
8+
const ITERATIONS = 100;
9+
// Return and print an AST from string contents
10+
$main = function() {
11+
// Instantiate new parser instance
12+
$parser = new Parser();
13+
$t0 = microtime(true);
14+
$astNode = $parser->parseSourceFile(file_get_contents('src/Parser.php'));
15+
$t1 = microtime(true);
16+
for ($i = 0; $i < ITERATIONS; $i++) {
17+
$diagnostics = DiagnosticsProvider::getDiagnostics($astNode);
18+
}
19+
$t2 = microtime(true);
20+
printf("Average time to generate diagnostics: %.6f\n", ($t2 - $t1) / ITERATIONS);
21+
printf("time to parse: %.6f\n", ($t1 - $t0));
22+
global $__counts;
23+
var_export($__counts);
24+
};
25+
$main();

‎phpunit.xml‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,21 +20,8 @@
2020
<file>tests/ParserGrammarTest.php</file>
2121
</testsuite>
2222

23-
<testsuite name="validation">
24-
<!--
25-
Validates against real-world scenarios.
26-
* INPUT: All files in `validation/frameworks/<framework-name>/*`
27-
* OUTPUT: Failing tests are output to `tests/output/<framework-name>`
28-
-->
29-
<file>tests/ParserFrameworkValidationTests.php</file>
30-
</testsuite>
31-
3223
<testsuite name="api">
33-
<file>tests/api/NodeApiTest.php</file>
34-
<file>tests/api/getNodeAtPosition.php</file>
35-
<file>tests/api/getResolvedName.php</file>
36-
<file>tests/api/PositionUtilitiesTest.php</file>
37-
<file>tests/api/TextEditTest.php</file>
24+
3825
</testsuite>
3926

4027
<testsuite name="performance">

‎src/DiagnosticsProvider.php‎

Lines changed: 47 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -10,31 +10,63 @@
1010

1111
class DiagnosticsProvider {
1212

13-
private static $tokenKindToText;
13+
/**
14+
* @param int $kind (must be a valid token kind)
15+
* @return string
16+
*/
17+
public static function getTextForTokenKind($kind) {
18+
static $tokenKindToText;
19+
if (!isset($tokenKindToText)) {
20+
$tokenKindToText = \array_flip(\array_merge(
21+
TokenStringMaps::OPERATORS_AND_PUNCTUATORS,
22+
TokenStringMaps::KEYWORDS,
23+
TokenStringMaps::RESERVED_WORDS
24+
));
25+
}
26+
return $tokenKindToText[$kind];
27+
}
1428

1529
/**
1630
* Returns the diagnostic for $node, or null.
17-
* @param \Microsoft\PhpParser\Node $node
31+
* @param \Microsoft\PhpParser\Node|\Microsoft\PhpParser\Token $node
1832
* @return Diagnostic|null
1933
*/
2034
public static function checkDiagnostics($node) {
21-
if (!isset(self::$tokenKindToText)) {
22-
self::$tokenKindToText = \array_flip(\array_merge(
35+
if ($node instanceof Token) {
36+
if (\get_class($node) === Token::class) {
37+
return null;
38+
}
39+
return self::checkDiagnosticForUnexpectedToken($node);
40+
}
41+
42+
if ($node instanceof Node) {
43+
return $node->getDiagnosticForNode();
44+
}
45+
return null;
46+
}
47+
48+
/**
49+
* @param Token $token
50+
* @return Diagnostic|null
51+
*/
52+
private static function checkDiagnosticForUnexpectedToken($token) {
53+
static $tokenKindToText;
54+
if (!isset($tokenKindToText)) {
55+
$tokenKindToText = \array_flip(\array_merge(
2356
TokenStringMaps::OPERATORS_AND_PUNCTUATORS,
2457
TokenStringMaps::KEYWORDS,
2558
TokenStringMaps::RESERVED_WORDS
2659
));
2760
}
28-
2961
if ($node instanceof SkippedToken) {
3062
// TODO - consider also attaching parse context information to skipped tokens
3163
// this would allow us to provide more helpful error messages that inform users what to do
3264
// about the problem rather than simply pointing out the mistake.
3365
return new Diagnostic(
3466
DiagnosticKind::Error,
3567
"Unexpected '" .
36-
(isset(self::$tokenKindToText[$node->kind])
37-
? self::$tokenKindToText[$node->kind]
68+
(isset($tokenKindToText[$node->kind])
69+
? $tokenKindToText[$node->kind]
3870
: Token::getTokenKindNameFromValue($node->kind)) .
3971
"'",
4072
$node->start,
@@ -44,93 +76,14 @@ public static function checkDiagnostics($node) {
4476
return new Diagnostic(
4577
DiagnosticKind::Error,
4678
"'" .
47-
(isset(self::$tokenKindToText[$node->kind])
48-
? self::$tokenKindToText[$node->kind]
79+
(isset($tokenKindToText[$node->kind])
80+
? $tokenKindToText[$node->kind]
4981
: Token::getTokenKindNameFromValue($node->kind)) .
5082
"' expected.",
5183
$node->start,
5284
$node->getEndPosition() - $node->start
5385
);
5486
}
55-
56-
if ($node === null || $node instanceof Token) {
57-
return null;
58-
}
59-
60-
if ($node instanceof Node) {
61-
if ($node instanceof Node\MethodDeclaration) {
62-
foreach ($node->modifiers as $modifier) {
63-
if ($modifier->kind === TokenKind::VarKeyword) {
64-
return new Diagnostic(
65-
DiagnosticKind::Error,
66-
"Unexpected modifier '" . self::$tokenKindToText[$modifier->kind] . "'",
67-
$modifier->start,
68-
$modifier->length
69-
);
70-
}
71-
}
72-
}
73-
elseif ($node instanceof Node\Statement\NamespaceUseDeclaration) {
74-
if (
75-
$node->useClauses != null
76-
&& \count($node->useClauses->children) > 1
77-
) {
78-
foreach ($node->useClauses->children as $useClause) {
79-
if($useClause instanceof Node\NamespaceUseClause && !is_null($useClause->openBrace)) {
80-
return new Diagnostic(
81-
DiagnosticKind::Error,
82-
"; expected.",
83-
$useClause->getEndPosition(),
84-
1
85-
);
86-
}
87-
}
88-
}
89-
}
90-
else if ($node instanceof Node\Statement\BreakOrContinueStatement) {
91-
if ($node->breakoutLevel === null) {
92-
return null;
93-
}
94-
95-
$breakoutLevel = $node->breakoutLevel;
96-
while ($breakoutLevel instanceof Node\Expression\ParenthesizedExpression) {
97-
$breakoutLevel = $breakoutLevel->expression;
98-
}
99-
100-
if (
101-
$breakoutLevel instanceof Node\NumericLiteral
102-
&& $breakoutLevel->children->kind === TokenKind::IntegerLiteralToken
103-
) {
104-
$literalString = $breakoutLevel->getText();
105-
$firstTwoChars = \substr($literalString, 0, 2);
106-
107-
if ($firstTwoChars === '0b' || $firstTwoChars === '0B') {
108-
if (\bindec(\substr($literalString, 2)) > 0) {
109-
return null;
110-
}
111-
}
112-
else if (\intval($literalString, 0) > 0) {
113-
return null;
114-
}
115-
}
116-
117-
if ($breakoutLevel instanceof Token) {
118-
$start = $breakoutLevel->getStartPosition();
119-
}
120-
else {
121-
$start = $breakoutLevel->getStart();
122-
}
123-
$end = $breakoutLevel->getEndPosition();
124-
125-
return new Diagnostic(
126-
DiagnosticKind::Error,
127-
"Positive integer literal expected.",
128-
$start,
129-
$end - $start
130-
);
131-
}
132-
}
133-
return null;
13487
}
13588

13689
/**
@@ -141,11 +94,15 @@ public static function checkDiagnostics($node) {
14194
public static function getDiagnostics(Node $n) : array {
14295
$diagnostics = [];
14396

144-
foreach ($n->getDescendantNodesAndTokens() as $node) {
97+
/**
98+
* @param \Microsoft\PhpParser\Node|\Microsoft\PhpParser\Token $node
99+
*/
100+
$n->walkDescendantNodesAndTokens(function($node) use (&$diagnostics) {
101+
// echo "Processing " . get_class($node) . "\n";
145102
if (($diagnostic = self::checkDiagnostics($node)) !== null) {
146103
$diagnostics[] = $diagnostic;
147104
}
148-
}
105+
});
149106

150107
return $diagnostics;
151108
}

‎src/Node.php‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,48 @@ public function getDescendantNodesAndTokens(callable $shouldDescendIntoChildrenF
170170
}
171171
}
172172

173+
/**
174+
* Iterate over all descendant Nodes and Tokens, calling $callback
175+
*
176+
* @param callable $callback a callback that accepts Node|Token
177+
* @param callable|null $shouldDescendIntoChildrenFn
178+
* @return void
179+
*/
180+
public function walkDescendantNodesAndTokens(\Closure $callback, callable $shouldDescendIntoChildrenFn = null) {
181+
foreach ($this->getDescendantNodesAndTokens($shouldDescendIntoChildrenFn) as $child) {
182+
$callback($node);
183+
}
184+
}
185+
/**
186+
public function walkDescendantNodesAndTokens(\Closure $callback, callable $shouldDescendIntoChildrenFn = null) {
187+
// TODO - write unit tests to prove invariants
188+
// (concatenating all descendant Tokens should produce document, concatenating all Nodes should produce document)
189+
foreach (static::CHILD_NAMES as $name) {
190+
$child = $this->$name;
191+
// Check conditions in order of frequency
192+
if ($child instanceof Token) {
193+
$callback($child);
194+
} elseif ($child instanceof Node) {
195+
$callback($child);
196+
if ($shouldDescendIntoChildrenFn === null || $shouldDescendIntoChildrenFn($child)) {
197+
$child->walkDescendantNodesAndTokens($callback, $shouldDescendIntoChildrenFn);
198+
}
199+
} elseif (\is_array($child)) {
200+
foreach ($child as $childElement) {
201+
if ($childElement instanceof Token) {
202+
$callback($childElement);
203+
} elseif ($childElement instanceof Node) {
204+
$callback($childElement);
205+
if ($shouldDescendIntoChildrenFn === null || $shouldDescendIntoChildrenFn($childElement)) {
206+
$childElement->walkDescendantNodesAndTokens($callback, $shouldDescendIntoChildrenFn);
207+
}
208+
}
209+
}
210+
}
211+
}
212+
}
213+
*/
214+
173215
/**
174216
* Gets a generator containing all descendant Nodes.
175217
* @param callable|null $shouldDescendIntoChildrenFn
@@ -640,4 +682,13 @@ private function addToImportTable($alias, $functionOrConst, $namespaceNameParts,
640682
}
641683
return array($namespaceImportTable, $functionImportTable, $constImportTable);
642684
}
685+
686+
/**
687+
* This is overridden in subclasses
688+
* @return Diagnostic|null - Callers should use DiagnosticsProvider::getDiagnostics instead
689+
* @internal
690+
*/
691+
public function getDiagnosticForNode() {
692+
return null;
693+
}
643694
}

‎src/Node/MethodDeclaration.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66

77
namespace Microsoft\PhpParser\Node;
88

9+
use Microsoft\PhpParser\Diagnostic;
10+
use Microsoft\PhpParser\DiagnosticKind;
11+
use Microsoft\PhpParser\DiagnosticsProvider;
912
use Microsoft\PhpParser\FunctionLike;
1013
use Microsoft\PhpParser\Node;
1114
use Microsoft\PhpParser\Token;
@@ -52,4 +55,22 @@ public function isStatic() : bool {
5255
public function getName() {
5356
return $this->name->getText($this->getFileContents());
5457
}
58+
59+
/**
60+
* @return Diagnostic|null - Callers should use DiagnosticsProvider::getDiagnostics instead
61+
* @internal
62+
* @override
63+
*/
64+
public function getDiagnosticForNode() {
65+
foreach ($this->modifiers as $modifier) {
66+
if ($modifier->kind === TokenKind::VarKeyword) {
67+
return new Diagnostic(
68+
DiagnosticKind::Error,
69+
"Unexpected modifier '" . DiagnosticsProvider::getTextForTokenKind($modifier->kind) . "'",
70+
$modifier->start,
71+
$modifier->length
72+
);
73+
}
74+
}
75+
}
5576
}

‎src/Node/Statement/BreakOrContinueStatement.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,13 @@
66

77
namespace Microsoft\PhpParser\Node\Statement;
88

9+
use Microsoft\PhpParser\Diagnostic;
10+
use Microsoft\PhpParser\DiagnosticKind;
11+
use Microsoft\PhpParser\Node;
912
use Microsoft\PhpParser\Node\Expression;
1013
use Microsoft\PhpParser\Node\StatementNode;
1114
use Microsoft\PhpParser\Token;
15+
use Microsoft\PhpParser\TokenKind;
1216

1317
class BreakOrContinueStatement extends StatementNode {
1418
/** @var Token */
@@ -23,4 +27,52 @@ class BreakOrContinueStatement extends StatementNode {
2327
'breakoutLevel',
2428
'semicolon'
2529
];
30+
31+
/**
32+
* @return Diagnostic|null - Callers should use DiagnosticsProvider::getDiagnostics instead
33+
* @internal
34+
* @override
35+
*/
36+
public function getDiagnosticForNode() {
37+
if ($this->breakoutLevel === null) {
38+
return null;
39+
}
40+
41+
$breakoutLevel = $this->breakoutLevel;
42+
while ($breakoutLevel instanceof Node\Expression\ParenthesizedExpression) {
43+
$breakoutLevel = $breakoutLevel->expression;
44+
}
45+
46+
if (
47+
$breakoutLevel instanceof Node\NumericLiteral
48+
&& $breakoutLevel->children->kind === TokenKind::IntegerLiteralToken
49+
) {
50+
$literalString = $breakoutLevel->getText();
51+
$firstTwoChars = \substr($literalString, 0, 2);
52+
53+
if ($firstTwoChars === '0b' || $firstTwoChars === '0B') {
54+
if (\bindec(\substr($literalString, 2)) > 0) {
55+
return null;
56+
}
57+
}
58+
else if (\intval($literalString, 0) > 0) {
59+
return null;
60+
}
61+
}
62+
63+
if ($breakoutLevel instanceof Token) {
64+
$start = $breakoutLevel->getStartPosition();
65+
}
66+
else {
67+
$start = $breakoutLevel->getStart();
68+
}
69+
$end = $breakoutLevel->getEndPosition();
70+
71+
return new Diagnostic(
72+
DiagnosticKind::Error,
73+
"Positive integer literal expected.",
74+
$start,
75+
$end - $start
76+
);
77+
}
2678
}

0 commit comments

Comments
 (0)