Phpstan Level 3 #566

shmax · 2019-10-27T01:27:13Z

PHPStan Level 3!

This is already a really tight, beautifully put together code base, so there wasn't that much to do, but there were a few points of interest:

I wrote PHPStan extensions to direct inferred type for some static methods, namely isInputType, isOutputType, and isCompositeType
It turns out I was able to bring in native PHP type hinting in a lot of places, so a few of the tests wound up in the position of needlessly validating stuff that would get caught by PHP's natural checking, so I removed them

shmax · 2019-10-27T01:32:28Z

src/Type/Definition/FieldArgument.php

    private $type;

-    /** @var bool */
-    private $defaultValueExists = false;


I removed this because you can do the same logic by checking the config bucket, but there was and still is a potential bug, here--defaultValue is public with no setter, so if no default value is passed at construction time but is set after the fact, then defaultValueExists will still return false.

Technically GQL types are supposed to be immutable once created. Unfortunately, PHP doesn't provide any means for read-only properties. Doing getters for every single data point is tedious; __isset and __get are readability and static checking nightmares (although definitely useful for backward compatibility)

I prefer assuming that people are sane enough not to shoot themselves in the foot. But when they want to - nothing will stop them. We even have a dedicated tool for this called Reflection %)

I don't intend to fight this battle in this PR, but there are ways to have read-only properties. For example:

class FieldArgument { private $_defaultValue; public function __get($name) { switch($name) { case 'defaultValue`: return $_defaultValue; } } }

There are, of course, drawbacks to this kind of thing (eg. no intellisense), and I'm not seriously recommending it at this point.

shmax · 2019-10-27T01:45:53Z

tests/Type/ValidationTest.php

-    /**
-     * @see it('rejects an empty Object field type')
-     */
-    public function testRejectsAnEmptyObjectFieldType() : void


No longer needed! PHP itself will throw an error if type is not provided and not Type.

shmax · 2019-10-27T01:46:25Z

tests/Type/ValidationTest.php

-     */
-    public function testRejectsANonTypeValueAsAnObjectFieldType()
-    {
-        $schema = $this->schemaWithObjectFieldOfType($this->Number);


No longer necessary. PHP will throw.

shmax · 2019-10-27T01:46:40Z

tests/Type/ValidationTest.php

-     */
-    public function testRejectsAnEmptyInterfaceFieldType() : void
-    {
-        $schema = $this->schemaWithInterfaceFieldOfType(null);


Not needed.

shmax · 2019-10-27T01:47:54Z

tests/Utils/BreakingChangesFinderTest.php

-                        'arg1' => ['name' => 'arg1'],
+                        'arg1' => [
+                            'name' => 'arg1',
+                            'type' => Type::string()


This missing type property was caught by PHP's natural checking.

mfn · 2019-10-27T04:44:32Z

Multiline array shape syntax

🤔 wow, #TIL

When I look through the PR, 99% (just a feeling) of what I saw regarding the type changes (ecept the multiline array) looks great.

Any reason this would need to be part of this "waiting" PR?

Looks to me the majority of the changes (i.e. "no brainer changes") could be made independently already, no?

shmax · 2019-10-27T04:57:07Z

Looks to me the majority of the changes (i.e. "no brainer changes") could be made independently already, no?

That's a good point. I'll see if I can separate that stuff out tomorrow morning...

mfn · 2019-10-27T04:57:27Z

Multiline array shape syntax

Btw, its there a "standard" for this? Does any IDE out there support this? Quickly tried PhpStorm, doesn't seem so.

Neither the referenced PR nor the issue seem to reference the origin of this format.

Thanks

shmax · 2019-10-27T05:23:00Z

Btw, its there a "standard" for this? Does any IDE out there support this? Quickly tried PhpStorm, doesn't seem so.

I don't know. The original PR that brought it into phpstan/phpdoc-parser didn't mention anything; best I could find is this issue, which mentions a similar feature in psalm's docblocks.

Digging further, I found this, which describes the feature as a "special format for arrays". The PHPDoc proposed PSR-5 standard doesn't have this feature.

Phan also has something similar, though I didn't bother digging down to find its source (interestingly, phan seems to credit the idea to phpdoc).

I guess we'll have to consider this sort of a graphql/phan/psalm-only feature.

simPod · 2019-10-27T09:56:39Z

I think it was first introduced in psalm https://psalm.dev/docs/annotating_code/type_syntax/array_types/#object-like-arrays.

PHPStorm is kinda slow in implementing this, I think I even created ticket for them. But eg. ticket for generic collection support is there in top voted tickets for ages already.

mfn · 2019-10-27T14:28:57Z

PHPStorm is kinda slow in implementing this

Interesting. When PSR-12 was about to be ratified, I couldn't change my shirt faster than they started implementing it.

Guess having something "official" can boost this.

Sorry for OT and @shmax thanks for digging deeper, obviously I did a bad job at researching 👍

shmax · 2019-10-27T17:45:19Z

Removed the array shape stuff for now. Will open a new PR if this gets merged.

simPod

gj!

src/Executor/ReferenceExecutor.php

src/Experimental/Executor/Collector.php

src/Experimental/Executor/CoroutineExecutor.php

src/GraphQL.php

src/Language/DirectiveLocation.php

src/Utils/BreakingChangesFinder.php

src/Utils/TypeInfo.php

src/Validator/Rules/FieldsOnCorrectType.php

src/Validator/Rules/UniqueInputFieldNames.php

shmax · 2019-10-27T19:28:17Z

@simPod thanks very much for the thorough review! I think I addressed all your comments. Ready for the next round...

src/Type/Definition/NonNull.php

shmax · 2019-10-28T07:27:01Z

Sorry for all the extra commits in this PR--I had to keep fighting with it to make Scrutinizer happy. By the way, are you guys still all-in on Scrutinizer? Trying to keep its black-box code analyzer happy (its got plenty of its own bugs) is a serious pain. We could probably configure it to use PHPStan instead of its own checker; I'm willing to investigate the idea if anyone is interested...

docs/type-system/scalar-types.md

examples/01-blog/Blog/Type/Scalar/UrlType.php

phpstan.neon.dist

src/Language/Token.php

src/Utils/TypeInfo.php

tests/Type/ValidationTest.php

src/Type/Schema.php

src/Type/Definition/NonNull.php

src/Utils/AST.php

Co-Authored-By: Šimon Podlipský <simon@podlipsky.net>

shmax · 2019-11-01T16:30:47Z

I've addressed all open comments (thanks, @simPod !). At this point all tests and checkers pass (even Scrutinizer). There is still a lot to do, but I think we're at a good stopping point here, and I will resume in PHPStan lvl 4.

Would love some more reviewers! @vladar @spawnia @mfn

simPod

LGTM!

simPod · 2019-11-01T16:39:47Z

If @vladar thinks it's good to go, I'd squash commits before merge.

shmax · 2019-11-01T17:44:38Z

If @vladar thinks it's good to go, I'd squash commits before merge.

Sure, happy to squash, but doesn't Github have the Squash and Merge button?

vladar

That is trully a great job! I've left a bunch of comments but we'll definitely merge this as soon as remaining questions are resolved.

src/Executor/ReferenceExecutor.php

src/Experimental/Executor/Collector.php

src/Experimental/Executor/CoroutineExecutor.php

src/Language/AST/DocumentNode.php

src/Utils/TypeInfo.php

src/Validator/Rules/FieldsOnCorrectType.php

tests/Executor/DeferredFieldsTest.php

tests/Type/DefinitionTest.php

vladar · 2019-11-03T06:15:26Z

Merged! Thanks for this exceptionally useful PR!

shmax · 2019-11-03T09:22:36Z

Woot! Thanks for merging, and thanks all for reviewing!

shmax commented Oct 27, 2019

View reviewed changes

simPod self-requested a review October 27, 2019 18:03

simPod requested changes Oct 27, 2019

View reviewed changes

shmax requested a review from simPod October 27, 2019 19:51

shmax commented Oct 28, 2019

View reviewed changes

src/Type/Definition/NonNull.php Show resolved Hide resolved

simPod requested changes Oct 28, 2019

View reviewed changes

src/Type/Definition/NonNull.php Outdated Show resolved Hide resolved

src/Type/Definition/NonNull.php Show resolved Hide resolved

src/Utils/AST.php Show resolved Hide resolved

shmax and others added 10 commits October 29, 2019 07:17

phpstan level 3

5c8adc3

cleanup

36e8eec

linting, remove array shape stuff

c323c16

use protected

e4352c0

use concrete type

f091d91

remove null type

87ef68c

add null

9edeb73

use null syntax over ?

0ad36dd

address comments

e381be8

Update src/Validator/Rules/UniqueInputFieldNames.php

8574398

Co-Authored-By: Šimon Podlipský <simon@podlipsky.net>

shmax added 5 commits November 1, 2019 08:23

code sniffer fixes

7142c3d

use alt null syntax

6f00407

return type hint

0c0af8e

return type hint

519321a

more typehinting

f836716

shmax requested a review from simPod November 1, 2019 16:35

simPod approved these changes Nov 1, 2019

View reviewed changes

shmax added 2 commits November 1, 2019 21:37

refine type

1185c33

revert

2c70456

vladar requested changes Nov 2, 2019

View reviewed changes

vladar mentioned this pull request Nov 2, 2019

Use phpstan instead of scrutinizer's own checker #569

Closed

shmax added 9 commits November 2, 2019 08:56

narrow to ObjectType

d9f2ede

remove instanceOf check

bd650ab

remove unused imports

0807bb5

simplify type

e1e5b08

restore null init

bebaad9

restore isInputType check

3b428f7

restore check

78295d7

remove check

801033f

revert deferredfieldstest kookiness

18ed4af

vladar approved these changes Nov 3, 2019

View reviewed changes

vladar merged commit baad32b into webonyx:master Nov 3, 2019

vladar mentioned this pull request Nov 3, 2019

fix phpdoc annotation for GraphQL\Type\Definition\ObjectType::$resolveFieldFn #571

Closed

shmax deleted the stan-level-3 branch November 3, 2019 09:22

vladar mentioned this pull request Nov 20, 2019

Implement BuildClientSchema #539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phpstan Level 3 #566

Phpstan Level 3 #566

shmax commented Oct 27, 2019 •

edited

Loading

shmax Oct 27, 2019

vladar Nov 2, 2019

shmax Nov 2, 2019 •

edited

Loading

shmax Oct 27, 2019

shmax Oct 27, 2019

shmax Oct 27, 2019

shmax Oct 27, 2019

mfn commented Oct 27, 2019

shmax commented Oct 27, 2019

mfn commented Oct 27, 2019

shmax commented Oct 27, 2019

simPod commented Oct 27, 2019

mfn commented Oct 27, 2019

shmax commented Oct 27, 2019

simPod left a comment

shmax commented Oct 27, 2019

shmax commented Oct 28, 2019

shmax commented Nov 1, 2019 •

edited

Loading

simPod left a comment

simPod commented Nov 1, 2019

shmax commented Nov 1, 2019 •

edited

Loading

vladar left a comment

vladar commented Nov 3, 2019

shmax commented Nov 3, 2019

Phpstan Level 3 #566

Phpstan Level 3 #566

Conversation

shmax commented Oct 27, 2019 • edited Loading

PHPStan Level 3!

shmax Oct 27, 2019

Choose a reason for hiding this comment

vladar Nov 2, 2019

Choose a reason for hiding this comment

shmax Nov 2, 2019 • edited Loading

Choose a reason for hiding this comment

shmax Oct 27, 2019

Choose a reason for hiding this comment

shmax Oct 27, 2019

Choose a reason for hiding this comment

shmax Oct 27, 2019

Choose a reason for hiding this comment

shmax Oct 27, 2019

Choose a reason for hiding this comment

mfn commented Oct 27, 2019

shmax commented Oct 27, 2019

mfn commented Oct 27, 2019

shmax commented Oct 27, 2019

simPod commented Oct 27, 2019

mfn commented Oct 27, 2019

shmax commented Oct 27, 2019

simPod left a comment

Choose a reason for hiding this comment

shmax commented Oct 27, 2019

shmax commented Oct 28, 2019

shmax commented Nov 1, 2019 • edited Loading

simPod left a comment

Choose a reason for hiding this comment

simPod commented Nov 1, 2019

shmax commented Nov 1, 2019 • edited Loading

vladar left a comment

Choose a reason for hiding this comment

vladar commented Nov 3, 2019

shmax commented Nov 3, 2019

shmax commented Oct 27, 2019 •

edited

Loading

shmax Nov 2, 2019 •

edited

Loading

shmax commented Nov 1, 2019 •

edited

Loading

shmax commented Nov 1, 2019 •

edited

Loading