-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHPLIB-1119: Update to Psalm 5 #1131
Conversation
34499a9
to
f8844c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at psalm-baseline.xml
, this was not enough to enforce the type of $document->encryptedField
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
</file> | ||
<file src="src/GridFS/ReadableStream.php"> | ||
<MixedArgument occurrences="2"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's nice they removed the occurrences
number. That will reduce merge conflicts.
</PropertyNotSetInConstructor> | ||
|
||
<!-- This is often the result of type checks due to missing native types --> | ||
<DocblockTypeContradiction errorLevel="info" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions and an optional suggestion or two. Changes LGTM, though.
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Psalm object to using printf()
with a single argument, or did you do this on your own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Psalm objected to this, complaining that printf
needs two arguments.
* @see Executable::execute() | ||
* @throws DriverRuntimeException for other driver errors (e.g. connection errors) | ||
*/ | ||
public function execute(Server $server): CachingIterator | ||
{ | ||
/** @var Cursor<array> $cursor */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was Iterable<type>
syntax introduced in Psalm 5? I assume this works on any iterable, as you also use it on CachingIterator above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no option to annotate an iterator over list
, so we have to stick to int
.
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
*/ | ||
class CachingIterator implements Countable, Iterator | ||
{ | ||
private const FIELD_KEY = 0; | ||
private const FIELD_VALUE = 1; | ||
|
||
/** @var list<array{0: TKey, 1: TValue}> */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be list<list<TKey, TValue>>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*/ | ||
#[ReturnTypeWillChange] | ||
public function current() | ||
{ | ||
return $this->isValid ? parent::current() : null; | ||
return $this->valid() ? parent::current() : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this necessary to use the @psalm-assert-if-true
annotation on valid()
? Otherwise, I see no reason to add a method call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some checking on the Playground, and unfortunately it doesn't work with variables: https://psalm.dev/r/bf4920bc0b
@@ -26,6 +26,7 @@ | |||
* This iterator is used for enumerating collections in a database. | |||
* | |||
* @see \MongoDB\Database::listCollections() | |||
* @template-extends Iterator<int, CollectionInfo> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to my earlier question, I assume there's no notation for a list-like Traversable?
@@ -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>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to repeat the variable name in these annotations? I have some recollection of not doing so previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
f8844c7
to
d33462f
Compare
PHPLIB-1119
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.