PHPStan Level 3 #385

dantleech · 2022-09-22T16:52:34Z

👋 in Phpactor one of the most common types of error is accessing a member on NULL on TP parser nodes. So I thought I'd try and increase the type coverage.

This PR:

Updates PHPStan to level 3.
Fixes type hints (avoiding refactoring code).
Adds PHPStan as a dev dependency (it ships as a PHAR now, so no conflicts).
Removes the overridden public opeand property on the children of UnaryExpression

As a follow up question is it worth ditching Travis and adding PHPStan to the main GHA workflow?

We should be able to get to PHPStan level 5 pretty easy (30 errors) but level 6 is 368 errors 😅

dantleech · 2022-09-22T16:54:18Z

src/Node.php

@@ -625,6 +628,7 @@ public function getNamespaceDefinition() {
            $namespaceDefinition = null;
        }

+        /** @var NamespaceDefinition|null $namespaceDefinition */
        return $namespaceDefinition;


we could add a runtime check for stuff like this or refactor but it's probably a risky move considering the number of similar changes needed across the whole code base.

src/Node/Expression/PrefixUpdateExpression.php

src/Parser.php

dantleech · 2022-09-22T18:18:14Z

Rebased my changes on main after making them on master...

dantleech · 2022-09-22T18:24:30Z

.travis.yml

@@ -27,7 +27,6 @@ cache:
    - validation/frameworks

 before_script:
-  - if [[ $STATIC_ANALYSIS = true ]]; then composer require phpstan/phpstan --no-update; fi


required in --dev now as PHPStan is a PHAR

src/Node/EnumCaseDeclaration.php

src/Parser.php

src/Node/Expression/UnaryOpExpression.php

src/Parser.php

dantleech · 2022-09-23T08:16:29Z

src/Parser.php

-     * @return InterfaceMembers
-     */
-    private function parseInterfaceMembers($parentNode) : Node {
+    private function parseInterfaceMembers($parentNode) : InterfaceMembers {


probably dozens of other places we can do this, but figure PHPStan will pick this up on level 4 or 5

dantleech · 2022-09-23T08:17:14Z

Updated

roblourens

Thanks for doing this and thanks for the review @TysonAndre. lgtm but I'll wait for the final ✔️ from @TysonAndre

roblourens · 2022-09-25T02:50:03Z

And yeah, if we can get all the same stuff into github actions that exists in Travis, I would like to get rid of Travis.

TysonAndre · 2022-09-25T12:33:13Z

src/Parser.php

        if ($expression instanceof Node) {
            $expression->parent = $scopedPropertyAccessExpression;
-            $scopedPropertyAccessExpression->scopeResolutionQualifier = $expression; // TODO ensure always a Node
+
+            // scopeResolutionQualifier does not accept `Node` but


LGTM overall for latest commits; they addressed review comments/nits.

In followup PRs, that should hopefully be addressed by making parameter types more precise as well, assuming that phpstan can narrow a mix of subclasses of Node and definite non-subclassess to just the expected subclasses Expression|QualifiedName (haven't checked)

(Strict typing overall is useful so we don't accidentally write to an undeclared property, which would be a notice in 8.2 since the classes declared here aren't using #[\AllowDynamicProperties], though I don't need AllowDynamicProperties for my own use case)

- Perform a shallow clone of submodules analyzed in validation test suite in GitHub workflows (should take around 30 seconds) - Switch badge in README.md to GitHub Workflows and update default branch for badge. - After microsoft#385 is merged we can probably start running phpstan in GitHub Actions for a single php version and remove .travis.yml entirely (as a followup PR)

- Perform a shallow clone of submodules analyzed in validation test suite in GitHub workflows (should take around 30 seconds) - Switch badge in README.md to GitHub Workflows and update default branch for badge. - Remove travis config - After microsoft#385 is merged run_phpstan.sh can remove the separate install step (as a followup PR)

TysonAndre · 2022-09-25T13:39:41Z

This should be rebased against main now that other changes were merged yesterday, to re-run tests/analysis steps. Optionally, It'd be useful to also squash changes (https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History Squashing Commits) either here or when maintainers merge this in the web ui

Other than not being rebased, all of the changes here should be safe to release, though I'd rather release 0.1.2 first if you expect merging this to delay the process of publishing a release - 0.1.2 is getting rather large

Removes the overridden public operand property on the children of UnaryExpression

Looks correct and the property is still declared on the subclasses. Double checking: This is confirmed by the fact that tests continue to pass, so CHILD_NAMES obviously still refers to properties that exist.

dantleech · 2022-09-25T17:18:19Z

Updated and squashed

TysonAndre

lgtm again; I confirmed locally that there are no new/missing changes in the rebase

TysonAndre · 2022-09-25T17:50:50Z

 ------ ---------------------------------------------------------------------- 
  Line   Parser.php                                                            
 ------ ---------------------------------------------------------------------- 
  3544   Property                                                              
         Microsoft\PhpParser\Node\Statement\NamespaceDefinition::$name         
         (Microsoft\PhpParser\Node\QualifiedName|null) does not accept         
         Microsoft\PhpParser\MissingToken.                                     
  3579   Property Microsoft\PhpParser\Node\NamespaceUseClause::$namespaceName  
         (Microsoft\PhpParser\Node\QualifiedName) does not accept              
         Microsoft\PhpParser\MissingToken.

Property declarations should say MissingToken now?

src/Parser.php

dantleech · 2022-09-25T18:35:57Z

Property declarations should say MissingToken now?

seems like it:

https://github.com/dantleech/tolerant-php-parser/blob/9bcf16fb6da32c6d504d9e1e5b0dcfe0762efdd5/src/Parser.php#L3544

updated

dantleech · 2022-09-28T17:05:09Z

FTR I've created a PR on my repo against this branch for level 4: dantleech#1 although probably best to merge this first before reviewing.

Added ReturnTypeWillChange Fix return type for Expression Add type hints for returned variables Add generic parameter for delimted list Fixing type hints Ignore co-variance error Remove TODO - it already must always be a Node Ignore error Add missing types Update phpstan to level 3 Add phpstan to dev reqs No need to install phpstan independently Do not override unary expression operand It seems to me that the operand can be any expression Added token to operand types -- should this be MissingToken? Include tokens in return types ExpressionStatement => EchoStatement It seems this is always an EchoStatement not an ExpressionStatement unaryExpressionOrHigher can return a ThrowExpression Remove overridden property Add Token to union Remove QualifiedName and rename local variable Remove unused import Add return types Remove trailing whitespace Add MissingToken type

TysonAndre

Not a maintainer, but familiar with this repo - I don't see any issues in the latest patch set. The changed types look accurate

- Perform a shallow clone of submodules analyzed in validation test suite in GitHub workflows (should take around 30 seconds) - Switch badge in README.md to GitHub Workflows and update default branch for badge. - Remove travis config - After microsoft#385 is merged run_phpstan.sh can remove the separate install step (as a followup PR) - Require a newer phpunit patch version in devDependencies Make it harder for contributors to have any inconsistencies with CI when running tests (e.g. php version support, fixed phpunit bugs, etc)

dantleech · 2022-10-15T08:35:10Z

appologetic ping cc @roblourens

dantleech · 2022-11-11T21:53:56Z

...

roblourens · 2022-11-17T17:22:28Z

Sorry @dantleech. Will take a look ASAP

dantleech · 2022-11-17T17:50:44Z

No pressure :) thanks.

- Perform a shallow clone of submodules analyzed in validation test suite in GitHub workflows (should take around 30 seconds) - Switch badge in README.md to GitHub Workflows and update default branch for badge. - Remove travis config - After microsoft#385 is merged run_phpstan.sh can remove the separate install step (as a followup PR) - Require a newer phpunit patch version in devDependencies Make it harder for contributors to have any inconsistencies with CI when running tests (e.g. php version support, fixed phpunit bugs, etc)

dantleech commented Sep 22, 2022

View reviewed changes

src/Node/Expression/PrefixUpdateExpression.php Outdated Show resolved Hide resolved

dantleech commented Sep 22, 2022

View reviewed changes

src/Parser.php Outdated Show resolved Hide resolved

dantleech force-pushed the phpstan-level-3 branch from b7cae20 to ef0d29f Compare September 22, 2022 18:08

dantleech commented Sep 22, 2022

View reviewed changes

src/Node/EnumCaseDeclaration.php Outdated Show resolved Hide resolved

TysonAndre reviewed Sep 23, 2022

View reviewed changes

src/Parser.php Outdated Show resolved Hide resolved

src/Node/Expression/UnaryOpExpression.php Outdated Show resolved Hide resolved

src/Parser.php Outdated Show resolved Hide resolved

dantleech commented Sep 23, 2022

View reviewed changes

roblourens previously approved these changes Sep 25, 2022

View reviewed changes

TysonAndre previously approved these changes Sep 25, 2022

View reviewed changes

TysonAndre mentioned this pull request Sep 25, 2022

Move validation test suite, phpstan to GitHub Workflows #386

Open

dantleech dismissed stale reviews from TysonAndre and roblourens via 9bcf16f September 25, 2022 17:17

dantleech force-pushed the phpstan-level-3 branch from 2d66556 to 9bcf16f Compare September 25, 2022 17:17

TysonAndre previously approved these changes Sep 25, 2022

View reviewed changes

TysonAndre reviewed Sep 25, 2022

View reviewed changes

src/Parser.php Outdated Show resolved Hide resolved

dantleech dismissed TysonAndre’s stale review via b4ee294 September 25, 2022 18:27

dantleech force-pushed the phpstan-level-3 branch from b4ee294 to 0145399 Compare September 25, 2022 18:35

dantleech mentioned this pull request Sep 28, 2022

Phpstan Level 4 (depends PHPStan Level 3) dantleech/tolerant-php-parser#1

Open

dantleech force-pushed the phpstan-level-3 branch from 0145399 to 937564f Compare October 8, 2022 08:50

TysonAndre approved these changes Oct 8, 2022

View reviewed changes

roblourens approved these changes Nov 21, 2022

View reviewed changes

rzhao271 approved these changes Nov 21, 2022

View reviewed changes

roblourens merged commit 6bd3e3b into microsoft:main Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPStan Level 3 #385

PHPStan Level 3 #385

dantleech commented Sep 22, 2022 •

edited

Loading

dantleech Sep 22, 2022

dantleech commented Sep 22, 2022

dantleech Sep 22, 2022

dantleech Sep 23, 2022

dantleech commented Sep 23, 2022

roblourens left a comment

roblourens commented Sep 25, 2022

TysonAndre Sep 25, 2022 •

edited

Loading

TysonAndre commented Sep 25, 2022

dantleech commented Sep 25, 2022

TysonAndre left a comment •

edited

Loading

TysonAndre commented Sep 25, 2022

dantleech commented Sep 25, 2022

dantleech commented Sep 28, 2022 •

edited

Loading

TysonAndre left a comment •

edited

Loading

dantleech commented Oct 15, 2022

dantleech commented Nov 11, 2022

roblourens commented Nov 17, 2022

dantleech commented Nov 17, 2022

PHPStan Level 3 #385

PHPStan Level 3 #385

Conversation

dantleech commented Sep 22, 2022 • edited Loading

dantleech Sep 22, 2022

Choose a reason for hiding this comment

dantleech commented Sep 22, 2022

dantleech Sep 22, 2022

Choose a reason for hiding this comment

dantleech Sep 23, 2022

Choose a reason for hiding this comment

dantleech commented Sep 23, 2022

roblourens left a comment

Choose a reason for hiding this comment

roblourens commented Sep 25, 2022

TysonAndre Sep 25, 2022 • edited Loading

Choose a reason for hiding this comment

TysonAndre commented Sep 25, 2022

dantleech commented Sep 25, 2022

TysonAndre left a comment • edited Loading

Choose a reason for hiding this comment

TysonAndre commented Sep 25, 2022

dantleech commented Sep 25, 2022

dantleech commented Sep 28, 2022 • edited Loading

TysonAndre left a comment • edited Loading

Choose a reason for hiding this comment

dantleech commented Oct 15, 2022

dantleech commented Nov 11, 2022

roblourens commented Nov 17, 2022

dantleech commented Nov 17, 2022

dantleech commented Sep 22, 2022 •

edited

Loading

TysonAndre Sep 25, 2022 •

edited

Loading

TysonAndre left a comment •

edited

Loading

dantleech commented Sep 28, 2022 •

edited

Loading

TysonAndre left a comment •

edited

Loading