PHPLIB-610: Psalm Integration #973

alcaeus · 2022-09-14T06:47:02Z

This adds psalm to our CI pipeline to better check our code for typing errors. All existing code passes psalm checks on error level 3, while new code is expected to adhere to level 1. More information about error levels can be found in the psalm documentation.

The upcoming release will include an updated callmap for the MongoDB classes.

Existing errors are added to the baseline, meaning that new code needs to adhere to the strictest psalm level.

alcaeus · 2022-09-14T06:48:01Z

composer.json

@@ -19,7 +19,8 @@
    "require-dev": {
        "squizlabs/php_codesniffer": "^3.6",
        "doctrine/coding-standard": "^9.0",
-        "symfony/phpunit-bridge": "^5.2"
+        "symfony/phpunit-bridge": "^5.2",
+        "vimeo/psalm": "^4.x-dev"


This is to include changes from vimeo/psalm#8432 which haven't been released yet. In the future we may consider shipping a plugin so we can control releases ourselves.

Filed PHPLIB-984 to track updating this.

alcaeus · 2022-09-14T06:49:38Z

src/Client.php

@@ -117,7 +117,7 @@ public function __construct(string $uri = 'mongodb://127.0.0.1/', array $uriOpti
        $driverOptions['driver'] = $this->mergeDriverInfo($driverOptions['driver'] ?? []);

        $this->uri = $uri;
-        $this->typeMap = $driverOptions['typeMap'] ?? null;
+        $this->typeMap = $driverOptions['typeMap'];


The null coalesce is unnecessary as we add a type map to the $driverOptions array at the top of the method.

alcaeus · 2022-09-14T06:51:01Z

src/GridFS/Bucket.php

@@ -412,6 +413,7 @@ public function getFileIdForStream($stream)
         */
        $typeMap = ['root' => 'stdClass'] + $this->typeMap;
        $file = apply_type_map_to_document($file, $typeMap);
+        assert(is_object($file));


There are a number of assertions that cover this. These can be removed by making the function templated to allow psalm to understand that this will return an instance of whatever was specified as the root type. I consider that part of a future cleanup since that's a little more involved.

I assume this is necessary because psalm only knows that apply_type_map_to_document() returns array|object and can't infer that the root type map will ensure it's a stdClass instance. I assume the assertion is enough to silence psalm's complaints.

This is the first instance of assert() in the library. Is this kosher because of the new handling for PHP 7+ code (see: Expectations (PHP 7 only))?

I assume this is necessary because psalm only knows that apply_type_map_to_document() returns array|object and can't infer that the root type map will ensure it's a stdClass instance. I assume the assertion is enough to silence psalm's complaints.

Yes. The typeMap argument can be defined better (e.g. array{?root: class-string<T>}), which should then allow us to use T as a return type for the method. I haven't yet played around with it and how it would behave when the given typeMap doesn't contain a root element.

This is the first instance of assert() in the library. Is this kosher because of the new handling for PHP 7+ code (see: Expectations (PHP 7 only))?

Exactly. In development, the assert would trip if we're receiving anything other than an object, whilst in a typical production configuration it's entirely optimised away (i.e. doesn't generate an opcode) so has zero performance impact.

I've made a note to double-check the value of zend.assertions in our CI processes to ensure we're using development mode.

alcaeus · 2022-09-14T06:52:14Z

src/GridFS/Bucket.php

@@ -644,6 +646,7 @@ public function uploadFromStream(string $filename, $source, array $options = [])
    private function createPathForFile(object $file): string
    {
        if (! is_object($file->_id) || method_exists($file->_id, '__toString')) {
+            /** @psalm-suppress PossiblyInvalidCast */


For some reason, psalm kept tripping over this cast. I've suppressed the error instead of adding it to the baseline as I'm not aware of anything that would trip this conditional.

I imagine psalm may be considering the edge case where _id is an array, even thought we know that MongoDB would not permit that. Quoting Document: Field Names:

The field name _id is reserved for use as a primary key; its value must be unique in the collection, is immutable, and may be of any type other than an array. If the _id contains subfields, the subfield names cannot begin with a ($) symbol.

Beyond that, I think any other non-object, non-array value should be castable to a string. Even resource types have a representation, although we'd never expect that in a document. I think the only error conditions would be casting objects without __toString() and arrays.

Do you want to see if adding ! is_array($file_id) to the conditional fixes this? If not, OK to leave as-is.

The full error is:

ERROR: PossiblyInvalidCast - src/GridFS/Bucket.php:649:28 - object cannot be cast to string (see https://psalm.dev/190)

I rewrote the conditional to avoid the error:

if (is_array($file->_id) || (is_object($file->_id) && ! method_exists($file->_id, '__toString'))) { $id = toJSON(fromPHP(['_id' => $file->_id])); } else { $id = (string) $file->_id; }

I believe this covers all cases: arrays and objects without a __toString method are passed to fromPHP, while all other values are cast to string.

Oh, so this has nothing to do with an array. Psalm just had trouble understanding the inverted conditional. SGTM.

alcaeus · 2022-09-14T06:53:56Z

src/GridFS/StreamWrapper.php

@@ -66,6 +68,8 @@ public function __destruct()
     */
    public function getFile(): object
    {
+        assert($this->stream !== null);


The way PHP implements stream wrappers means that the stream property SHOULD be set whenever one of these methods is called, but strictly speaking any user could instantiate this class and break this. So, we add an assertion to surface errors when assertion checking is on, and let users should themselves in the foot if it's off.

alcaeus · 2022-09-14T06:54:46Z

src/GridFS/WritableStream.php

@@ -65,7 +66,7 @@ class WritableStream
    /** @var array */
    private $file;

-    /** @var resource */
+    /** @var HashContext|null */


hash_init was changed to return a HashContext instance in PHP 7.2 (see changelog).

Ah, very convenient that we don't have to worry about PHP <7.2 anymore.

alcaeus · 2022-09-14T07:00:21Z

src/functions.php

-    if ($session instanceof Session && $session->getServer() !== null) {
-        return $session->getServer();
+    $server = $session instanceof Session ? $session->getServer() : null;
+    if ($server !== null) {
+        return $server;


Interestingly enough, psalm was quite right to complain about this. Even if the first call to getServer() in the conditional returns a Server instance, it's not guaranteed that the second call in the return statement will return the same Server object, as the session could've been unpinned in between evaluating the conditional and executing the return. In that case, getServer() would return null which would be an invalid return value triggering a fatal error. (I'll leave the exercise of proving me wrong to the curious reader).

Since mongoc_client_session_get_server_id() returns the server_id property of the mongoc_client_session_t struct, I'm not sure how you'd be able to interrupt this without some hacking approach like runtime debugging or register_tick_function().

That said, avoiding a second call to getServer() seems entirely reasonable.

alcaeus · 2022-09-14T07:02:06Z

src/functions.php

@@ -561,5 +563,7 @@ function select_server_for_aggregate_write_stage(Manager $manager, array &$optio
        throw $serverSelectionError;
    }

+    assert($server instanceof Server);


Necessary as psalm trips over our fancy logic above. We can assume that if the original select_server call didn't return a server, $serverSelectionError would be set, so the return wouldn't be reached. This logic seems to be too advanced for psalm, so using this to silence the error.

Works for me. Since this differs from some other instances where you needed to add assert() in that psalm fails to detect a potential assignment via catch, I wonder if they might be interested in a bug report to address this.

I also checked whether this is expected behaviour based on a simplified example: https://psalm.dev/r/b1102ead61

Nice that they have a web-based tool available. Is that example suitable to open a bug report? I assume this isn't desired behavior.

Beyond the scope of this PR to be sure, so no need to follow up here.

vimeo/psalm#8361 looks related and I added the snippet there. We'll see what comes of it.

alcaeus · 2022-09-14T07:08:59Z

src/Model/ChangeStreamIterator.php

@@ -143,6 +135,14 @@ public function current()
        return $this->isValid ? parent::current() : null;
    }

+    final public function getInnerIterator(): Cursor


Adding this method to reduce the number of assertions needed in the ChangeStream model class. Strictly speaking any instance of ChangeStreamIterator would return a cursor when calling getInnerIterator(), as that's enforced through the constructor. However, as the class isn't final and there are no covariance checks for constructors, a user could potentially sidestep this.

The assert call is to satisfy psalm, and when assertion checking is disabled a type error will be thrown. I believe this is better than letting a user run into an error about calling a method that doesn't exist. Let me know if you'd rather remove this method and instead use assertions in ChangeStream.

Note that this method can be removed when making the class final, which we should do in 2.0.

Doesn't this only affect ChangeStream::getCursorId()? I don't see any other place where getInnerIterator() is called in the library apart from two instances in WatchFunctionalTest.

This is also interesting because ChangeStreamIterator extends IteratorIterator and enforces that its constructor is always a Cursor instance. I suppose psalm cannot infer that IteratorIterator::getInnerIterator() will always return an instance of its constructor argument without additional templating.

No objection to leaving this method in place, but I think a doc block above that briefly explains why it's needed and that it can potentially be removed in 2.0 would be helpful. You can also make a ticket with a 2.0 fixVersion to remind us to remove this down the line.

Doesn't this only affect ChangeStream::getCursorId()? I don't see any other place where getInnerIterator() is called in the library apart from two instances in WatchFunctionalTest.

That is correct.

This is also interesting because ChangeStreamIterator extends IteratorIterator and enforces that its constructor is always a Cursor instance. I suppose psalm cannot infer that IteratorIterator::getInnerIterator() will always return an instance of its constructor argument without additional templating.

Yes and no. While this is certainly true within the context of ChangeStreamIterator, technically an extending class could skip calling the parent constructor and go straight to IteratorIterator::__construct: https://3v4l.org/iir32. In that case, our assumption that the inner iterator is always a cursor no longer holds true. Yes, it is very much theoretical, but then that's what psalm was made for.

No objection to leaving this method in place, but I think a doc block above that briefly explains why it's needed and that it can potentially be removed in 2.0 would be helpful. You can also make a ticket with a 2.0 fixVersion to remind us to remove this down the line.

Added 👍

stof · 2022-09-14T08:29:11Z

.github/workflows/coding-standards.yml

@@ -5,14 +5,16 @@ on:
    branches:
      - "v*.*"
      - "master"
+      - "feature/*"


Do you really want to run the CI on PR targetting a feature branch ?

Sure, if such a PR were to exist.

stof · 2022-09-14T08:30:20Z

.github/workflows/coding-standards.yml

  push:
    branches:
      - "v*.*"
      - "master"
+      - "feature/*"


As you use such branches to open PRs, this will trigger 2 CI builds for each push (one for the push to the branch and one for the update to the PR).

I push my branches to GitHub usually long before opening a pull request. This allows me to test a wider range of environments than I cover locally (before pushing) and ensuring things are stable and done before opening a pull request.

IIRC, we have only ever used feature branches a handful of times when a large PR was ready but we weren't completely done with the JIRA epic (e.g. SDAM Monitoring in PHPC). In that case, we opened PRs against a feature branch from arbitrary branches on our forks and the feature branch was a waiting area before we could merge the functionality down the master.

I don't expect we will push directly to feature branches during development, so this shouldn't invite more CI builds than our typical workflow of PRs against master and release branches. @alcaeus: please correct me if I'm mistaken.

@jmikola the current PR is opened from a feature branch...

To summarise what we're currently testing and why it's useful:

PRs to any versioned branches (v*.*), the master branch and any feature branch (feature/*) should be tested, as we're using them to ensure we're only merging good code. The latter will not occur very often, but it could if we ever decide to build a larger feature in multiple PRs but don't want any logic in master before it's completely done.

Pushes to versioned branches and the master branch are important in the main repo. Pushes to feature branches will not occur very often here, however they do occur in forks (where we do our development work before creating a PR to the main repo). For quick PRs where we don't want to know if the checks are stable we can omit the feature/ prefix to skip CI before making a PR.

Enabling builds on push is also done for the benefit of outside contributors: this allows them to see CI results before creating a pull request even when they don't have a good test environment set up locally.

@jmikola the current PR is opened from a feature branch...

Totally missed that. @alcaeus and I talked about this today and I noted our workflows differ. I only recalled using feature branches on the mongodb repo when we wanted to merge down larger PRs without putting them into master. Andreas explained this is more for his fork, which inherits the workflow configuration.

He was last looking at whether some repo config options were available to conserve CI resources, so I'll defer to him on this.

stof · 2022-09-14T08:31:23Z

composer.json

@@ -19,7 +19,8 @@
    "require-dev": {
        "squizlabs/php_codesniffer": "^3.6",
        "doctrine/coding-standard": "^9.0",
-        "symfony/phpunit-bridge": "^5.2"
+        "symfony/phpunit-bridge": "^5.2",
+        "vimeo/psalm": "^4.x-dev"


is it expected that it does not allow using stable releases ?

See my original comment: #973 (comment)

We're currently relying on unreleased fixes to the MongoDB call maps.

jmikola

A minor suggestion or two and some questions. Excellent work!

jmikola · 2022-09-14T19:32:45Z

psalm.xml

+            <directory name="vendor" />
+        </ignoreFiles>
+    </projectFiles>
+</psalm>


Does psalm follow PHPUnit conventions and support a dist config file? I'm curious if that's what we should be adding in this PR, along with a .gitignore for psalm.xml.

Good point, I didn't consider that. Psalm does indeed support a list file, so I moved configuration to psalm.xml.dist and added psalm.xml to .gitignore as suggested 👍

jmikola · 2022-09-14T19:41:16Z

src/ChangeStream.php

        $this->iterator = call_user_func($this->resumeCallable, $this->getResumeToken(), $this->hasAdvanced);
+        if (! $this->iterator instanceof ChangeStreamIterator) {
+            throw new UnexpectedValueException('Expected change stream iterator from callable');
+        }


Even though Watch::resume() always returns a ChangeStreamIterator, ChangeStream's constructor only knows that resumeCallable is a callable type. Is there any way we could add extra typing around the callable, or would that require something like an interface defining __invoke() with a specific return type?

There also seems to be a discussion about this in vimeo/psalm#580, so maybe there's some syntax we can add to the ChangeStream constructor that would remove the need for this exception.

Very good point. I've added a new psalm type definition that is then referenced later on:

/** * @psalm-type ResumeCallable = callable(array|object|null, bool): ChangeStreamIterator */ // ... /** @var ResumeCallable|null */ private $resumeCallable;

This in fact removes the exception and has the added benefit of having psalm shout at us if we do indeed pass an invalid callable 👍

jmikola · 2022-09-14T19:41:19Z

src/ChangeStream.php

@@ -250,7 +252,15 @@ private function onIteration(bool $incrementKey): void
     */
    private function resume(): void
    {
+        if (! $this->resumeCallable) {
+            throw new BadMethodCallException('Cannot resume a closed change stream.');
+        }


I'm impressed that psalm was able to figure out a code path where resumeCallable might still be null. If I understand correctly, this would only happen if onIteration() detected a zero cursor ID and cleared the callable, and then resume() was invoked from either next() or rewind() (via resumeOrThrow() catching an exception).

Yes - that has been my experience whenever adding strict static analysis: the errors are quite unlikely to happen, but you know that they'd be head scratchers if a user ever presented you with a bug report where they ran into the issue 😅

jmikola · 2022-09-14T19:43:44Z

src/DeleteResult.php

@@ -43,7 +43,7 @@ public function __construct(WriteResult $writeResult)
     * This method should only be called if the write was acknowledged.
     *
     * @see DeleteResult::isAcknowledged()
-     * @return integer
+     * @return integer|null


WriteResult::getDeletedCount in PHPC always returns an integer. Is this necessary because of the exception code path? Same question applies to the other Result classes in this PR.

Side note: this might have been previously discussed in the previous typing PR, but we couldn't add a return type directly to these methods because the class isn't final, correct?

Unfortunately, WriteResult::getDeletedCount may return null - like all other classes in WriteResult. This is due to the logic in PHONGO_WRITERESULT_RETURN_LONG_FROM_BSON_INT32: if the result document doesn't contain the key, the macro will not initialise the return value, thus returning null. This is one of those instances where we relied on a non-null return type. We can update the logic in 1.15 to return 0 if no value was found (which would make sense), which would also allow us to revert the change. As is, psalm complains about this (which is how I spotted the entire thing).

Ah, I totally missed that when looking at the code locally. Confirmed the stubs and PHP.net docs both correctly report ?int return types. I opened PHPC-2142 to track this.

jmikola · 2022-09-14T19:48:46Z

src/GridFS/Bucket.php

@@ -412,6 +413,7 @@ public function getFileIdForStream($stream)
         */
        $typeMap = ['root' => 'stdClass'] + $this->typeMap;
        $file = apply_type_map_to_document($file, $typeMap);
+        assert(is_object($file));


I assume this is necessary because psalm only knows that apply_type_map_to_document() returns array|object and can't infer that the root type map will ensure it's a stdClass instance. I assume the assertion is enough to silence psalm's complaints.

This is the first instance of assert() in the library. Is this kosher because of the new handling for PHP 7+ code (see: Expectations (PHP 7 only))?

jmikola · 2022-09-14T20:43:12Z

src/Operation/DatabaseCommand.php

@@ -38,7 +38,7 @@ class DatabaseCommand implements Executable
    /** @var string */
    private $databaseName;

-    /** @var array|Command|object */


This was originally added in 4a764fe#diff-75b1bd6340fef4dfe6e4d23cb317e77f242e2a7f8344ba6c911b80e9fe87f119R40, but there's no code path where $command would be anything other than a Command. I suppose that change was made based on the constructor signature. Good catch.

jmikola · 2022-09-14T20:43:52Z

src/Operation/DropCollection.php

@@ -19,6 +19,7 @@

 use MongoDB\Driver\Command;
 use MongoDB\Driver\Exception\CommandException;
+use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException;


Interesting that phpcbf didn't catch this. It's only referenced from a @throws annotation.

I don't think phpcbf validates that @throws references a valid class. It probably cannot do it, as it works only on a single file IIRC.

jmikola · 2022-09-14T20:44:44Z

src/Operation/FindAndModify.php

@@ -253,7 +253,7 @@ public function execute(Server $server)

        $result = current($cursor->toArray());

-        return $result->value ?? null;
+        return is_object($result) ? $result->value ?? null : null;


I know PHP doesn't require it, but this helps readability without having to remember operator precedence rules.

Suggested change

return is_object($result) ? $result->value ?? null : null;

return is_object($result) ? ($result->value ?? null) : null;

Alternatively, since this is one of many cases where psalm presumably doesn't like us using -> on potential non-objects within an isset() context, perhaps there's a config option that we can use to ignore this? I don't feel strongly about it, so up to you if you're comfortable with the extra is_object calls.

Good point. We could technically ignore the error and add it to the baseline, deferring it to later when we handle type maps correctly. For now I applied your suggestion to make the code more readable 👍

jmikola · 2022-09-14T20:51:13Z

src/functions.php

-    if ($session instanceof Session && $session->getServer() !== null) {
-        return $session->getServer();
+    $server = $session instanceof Session ? $session->getServer() : null;
+    if ($server !== null) {
+        return $server;


Since mongoc_client_session_get_server_id() returns the server_id property of the mongoc_client_session_t struct, I'm not sure how you'd be able to interrupt this without some hacking approach like runtime debugging or register_tick_function().

That said, avoiding a second call to getServer() seems entirely reasonable.

jmikola · 2022-09-14T20:53:38Z

src/functions.php

@@ -561,5 +563,7 @@ function select_server_for_aggregate_write_stage(Manager $manager, array &$optio
        throw $serverSelectionError;
    }

+    assert($server instanceof Server);


Works for me. Since this differs from some other instances where you needed to add assert() in that psalm fails to detect a potential assignment via catch, I wonder if they might be interested in a bug report to address this.

* Add base psalm configuration * Run psalm on GitHub Actions * Fix psalm errors * Use development version of psalm The upcoming release will include an updated callmap for the MongoDB classes. * Add unfixable error to psalm baseline * Enforce psalm errorLevel 1 for new code Existing errors are added to the baseline, meaning that new code needs to adhere to the strictest psalm level. * Run coding standards and static analysis checks on feature branches * Add contribution docs for psalm * Improve ChangeStreamIterator handling of wrapped iterator * Specify callable explicitly in ChangeStream * Make null coalesce within ternary more readable * Rewrite conditional to avoid suppressing psalm errors * Move psalm config to dist file * Add comment about ChangeStreamIterator::getInnerIterator

alcaeus added 8 commits September 13, 2022 14:51

Add base psalm configuration

0b21162

Run psalm on GitHub Actions

cdd86d7

Fix psalm errors

992433b

Use development version of psalm

6ff84d1

The upcoming release will include an updated callmap for the MongoDB classes.

Add unfixable error to psalm baseline

06520a6

Enforce psalm errorLevel 1 for new code

ec431fa

Existing errors are added to the baseline, meaning that new code needs to adhere to the strictest psalm level.

Run coding standards and static analysis checks on feature branches

5c9fb35

Add contribution docs for psalm

8ee9459

alcaeus self-assigned this Sep 14, 2022

Improve ChangeStreamIterator handling of wrapped iterator

6b3448e

alcaeus commented Sep 14, 2022

View reviewed changes

alcaeus requested review from jmikola and levon80999 September 14, 2022 07:09

stof reviewed Sep 14, 2022

View reviewed changes

jmikola approved these changes Sep 14, 2022

View reviewed changes

alcaeus added 4 commits September 15, 2022 10:31

Specify callable explicitly in ChangeStream

dc6ab3b

Make null coalesce within ternary more readable

c84d4ed

Rewrite conditional to avoid suppressing psalm errors

5b38a82

Move psalm config to dist file

3ba3033

alcaeus force-pushed the feature/phplib-610-psalm branch from f2e480a to 3ba3033 Compare September 15, 2022 11:21

Add comment about ChangeStreamIterator::getInnerIterator

47c6ded

jmikola approved these changes Sep 16, 2022

View reviewed changes

alcaeus changed the title ~~PHPLIB-610 Psalm Integration~~ PHPLIB-610: Psalm Integration Sep 16, 2022

alcaeus merged commit aa04362 into mongodb:master Sep 16, 2022

alcaeus deleted the feature/phplib-610-psalm branch September 16, 2022 07:05

	return is_object($result) ? $result->value ?? null : null;
	return is_object($result) ? ($result->value ?? null) : null;

PHPLIB-610: Psalm Integration #973

PHPLIB-610: Psalm Integration #973

Conversation

alcaeus commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Sep 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Sep 15, 2022 •

edited