PHPLIB-1119: Update to Psalm 5 #1131

alcaeus · 2023-07-14T14:51:07Z

Along with the update, I decided to fix some low-hanging fruit from the baseline. I've also moved some other issues to the exclusion list in the config as they will appear more often until we have actual parameter types throughout the codebase.

GromNaN

LGTM

GromNaN · 2023-07-17T07:32:24Z

docs/examples/csfle-explicit_encryption.php

@@ -41,6 +41,8 @@

 /* Using the client configured without encryption, find the document and observe
 * that the field is not automatically decrypted. */
+
+/** @var object{encryptedField: Binary} $document */


Looking at psalm-baseline.xml, this was not enough to enforce the type of $document->encryptedField?

The error is regarding the return type of decrypt, which is mixed and can't be made any more specific:

ERROR: MixedArgument - docs/examples/csfle-explicit_encryption.php:51:27 - Argument 2 of printf cannot be mixed, expecting float|int|string

The @var annotation here prevents a different Psalm error regarding accessing a potentially undefined property.

GromNaN · 2023-07-17T07:33:39Z

psalm-baseline.xml

  </file>
  <file src="src/GridFS/ReadableStream.php">
-    <MixedArgument occurrences="2">


That's nice they removed the occurrences number. That will reduce merge conflicts.

GromNaN · 2023-07-17T07:35:57Z

psalm.xml.dist

+        </PropertyNotSetInConstructor>
+
+        <!-- This is often the result of type checks due to missing native types -->
+        <DocblockTypeContradiction errorLevel="info" />


GromNaN · 2023-07-17T07:45:52Z

src/Model/CachingIterator.php

@@ -83,12 +88,13 @@ public function current()
    {
        $currentItem = current($this->items);

-        return $currentItem !== false ? $currentItem[self::FIELD_VALUE] : false;
+        return $currentItem !== false ? $currentItem[self::FIELD_VALUE] : null;


This is something that surprised me when I worked on this class #1118.

The return value is not specified. ArrayIterator returns null while EmptyIterator throws an exception.

The fix looks good.

The class originally used current(), which returns false. That was preserved when it was later refactored in 3049b5d.

That said, I have no object to being consistent with ArrayIterator and returning null.

jmikola

Some questions and an optional suggestion or two. Changes LGTM, though.

jmikola · 2023-07-17T15:46:41Z

examples/changestream.php

@@ -53,7 +53,7 @@ function toJSON(object $document): string
    $changeStream->next();

    if (time() - $startTime > 3) {
-        printf("Aborting after 3 seconds...\n");
+        echo "Aborting after 3 seconds...\n";


Does Psalm object to using printf() with a single argument, or did you do this on your own?

Psalm objected to this, complaining that printf needs two arguments.

jmikola · 2023-07-17T15:49:21Z

src/Command/ListCollections.php

     * @see Executable::execute()
     * @throws DriverRuntimeException for other driver errors (e.g. connection errors)
     */
    public function execute(Server $server): CachingIterator
    {
+        /** @var Cursor<array> $cursor */


Was Iterable<type> syntax introduced in Psalm 5? I assume this works on any iterable, as you also use it on CachingIterator above.

I believe this was part of Psalm 4 already; the iterator stubs have templates to indicate the key and value types. Since I was working on these classes anyways, I added the annotations for consistency.

jmikola · 2023-07-17T15:51:29Z

src/MapReduceResult.php

@@ -33,10 +33,15 @@
 *
 * @see \MongoDB\Collection::mapReduce()
 * @see https://mongodb.com/docs/manual/reference/command/mapReduce/
+ * @template-implements IteratorAggregate<int, array|object>
+ * @psalm-type MapReduceCallable = callable(): Traversable<int, array|object>


Is the int key necessary because list pseudo-type implies in array? I suppose there's no syntax to define a Traversable that is a list/sequence with sequential, numeric indexes?

There's no option to annotate an iterator over list, so we have to stick to int.

jmikola · 2023-07-17T15:53:10Z

src/MapReduceResult.php

@@ -82,7 +88,7 @@ public function getExecutionTimeMS()
     * Return the mapReduce results as a Traversable.
     *
     * @see https://php.net/iteratoraggregate.getiterator
-     * @return Traversable
+     * @return Traversable<int, array|object>


Would it make sense to consolidate this type with the @psalm-type defined above? This return type is integral to the definition of MapReduceCallable.

OK to leave this as-is, but I was curious considering the type you made for MapReduceCallable.

Edit: Seeing MapReduce.php later in the PR, I assume the MapReduceCallable definition was only necessary to use @psalm-import-type elsewhere.

Correct, I needed to import this elsewhere, hence the @psalm-type. I'm not quite sure how much the various PHPDoc tooling (e.g. PhpStorm) can do with the Psalm types, so I tried to not overdo it with Psalm types in regular PHPDoc annotations.

jmikola · 2023-07-17T15:56:51Z

src/Model/CachingIterator.php

 */
 class CachingIterator implements Countable, Iterator
 {
    private const FIELD_KEY = 0;
    private const FIELD_VALUE = 1;

+    /** @var list<array{0: TKey, 1: TValue}> */


Should this be list<list<TKey, TValue>>?

list<TKey, TValue> is invalid, as list<TValue> resolves to array<int, TValue>. In this case the array shape is correct, as we only expect two elements in the list: one for the key, one for the value.

jmikola · 2023-07-17T16:00:08Z

src/Model/CachingIterator.php

@@ -83,12 +88,13 @@ public function current()
    {
        $currentItem = current($this->items);

-        return $currentItem !== false ? $currentItem[self::FIELD_VALUE] : false;
+        return $currentItem !== false ? $currentItem[self::FIELD_VALUE] : null;


The class originally used current(), which returns false. That was preserved when it was later refactored in 3049b5d.

That said, I have no object to being consistent with ArrayIterator and returning null.

jmikola · 2023-07-17T16:01:17Z

src/Model/ChangeStreamIterator.php

     */
    #[ReturnTypeWillChange]
    public function current()
    {
-        return $this->isValid ? parent::current() : null;
+        return $this->valid() ? parent::current() : null;


Was this necessary to use the @psalm-assert-if-true annotation on valid()? Otherwise, I see no reason to add a method call.

That is correct. I'll do some checking to see if @psalm-assert-if-true also works on variables and if psalm picks up on that to avoid the method call.

Did some checking on the Playground, and unfortunately it doesn't work with variables: https://psalm.dev/r/bf4920bc0b

jmikola · 2023-07-17T16:02:53Z

src/Model/CollectionInfoIterator.php

@@ -26,6 +26,7 @@
 * This iterator is used for enumerating collections in a database.
 *
 * @see \MongoDB\Database::listCollections()
+ * @template-extends Iterator<int, CollectionInfo>


Related to my earlier question, I assume there's no notation for a list-like Traversable?

jmikola · 2023-07-17T16:05:38Z

src/Model/IndexInfoIteratorIterator.php

@@ -34,11 +34,13 @@
 * @see https://github.com/mongodb/specifications/blob/master/source/enumerate-indexes.rst
 * @see https://mongodb.com/docs/manual/reference/command/listIndexes/
 * @see https://mongodb.com/docs/manual/reference/system-collections/
+ * @template-extends IteratorIterator<int, array, Traversable<int, array>>


Why are there three types under IteratorIterator<>? Are the first two types always consistent with the two types for Traversable? Presumably "Traversable" here is just whatever is returned by getIterator()?

The template for IteratorIterator includes the inner iterator, unfortunately also locking it to the same key type: https://github.com/vimeo/psalm/blob/9c814c8a69b0fd80f7fb81de5740434ed1ce7022/stubs/CoreGenericIterators.phpstub#L105..L111. This has already proved problematic in the AsListIterator I was working on for our Codec implementation, so I'll have to revisit this to figure out what the purpose of the third template parameter is. This works for us, so we're good.

jmikola · 2023-07-17T16:06:15Z

src/Operation/ListIndexes.php

@@ -144,6 +144,9 @@ private function executeCommand(Server $server): IndexInfoIteratorIterator

        $cursor->setTypeMap(['root' => 'array', 'document' => 'array']);

-        return new IndexInfoIteratorIterator(new CachingIterator($cursor), $this->databaseName . '.' . $this->collectionName);
+        /** @var CachingIterator<int, array> $iterator */


Is it necessary to repeat the variable name in these annotations? I have some recollection of not doing so previously.

It's unnecessary when used on a property, but required in the code.

Instantiating static in non-final classes makes assumptions about child class constructors, which is unsafe.

alcaeus requested review from jmikola and GromNaN July 14, 2023 14:51

alcaeus self-assigned this Jul 14, 2023

alcaeus force-pushed the phplib-1119-psalm-5 branch from 34499a9 to f8844c7 Compare July 14, 2023 15:06

GromNaN approved these changes Jul 17, 2023

View reviewed changes

jmikola approved these changes Jul 17, 2023

View reviewed changes

alcaeus added 5 commits July 18, 2023 08:46

Update to psalm 5

c6c7805

Enable reporting about unused baseline entries

8f103c3

Remove dangerous instantiation

7f0593d

Instantiating static in non-final classes makes assumptions about child class constructors, which is unsafe.

Fix missing return types

5f09bd6

Fix more psalm errors

d33462f

alcaeus force-pushed the phplib-1119-psalm-5 branch from f8844c7 to d33462f Compare July 18, 2023 06:49

alcaeus merged commit 0c1b8a6 into mongodb:master Jul 18, 2023
44 checks passed

alcaeus deleted the phplib-1119-psalm-5 branch July 18, 2023 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1119: Update to Psalm 5 #1131

PHPLIB-1119: Update to Psalm 5 #1131

alcaeus commented Jul 14, 2023

GromNaN left a comment

GromNaN Jul 17, 2023

alcaeus Jul 17, 2023

GromNaN Jul 17, 2023

GromNaN Jul 17, 2023

GromNaN Jul 17, 2023

jmikola Jul 17, 2023

jmikola left a comment

jmikola Jul 17, 2023

alcaeus Jul 18, 2023

jmikola Jul 17, 2023

alcaeus Jul 18, 2023

jmikola Jul 17, 2023

alcaeus Jul 18, 2023

jmikola Jul 17, 2023

alcaeus Jul 18, 2023

jmikola Jul 17, 2023

alcaeus Jul 18, 2023

jmikola Jul 17, 2023

jmikola Jul 17, 2023

alcaeus Jul 18, 2023

alcaeus Jul 18, 2023

jmikola Jul 17, 2023

jmikola Jul 17, 2023

alcaeus Jul 18, 2023

jmikola Jul 17, 2023

alcaeus Jul 18, 2023

PHPLIB-1119: Update to Psalm 5 #1131

PHPLIB-1119: Update to Psalm 5 #1131

Conversation

alcaeus commented Jul 14, 2023

GromNaN 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment