Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do not report serialize as unused #8650

Merged
merged 2 commits into from
Nov 2, 2022
Merged

Do not report serialize as unused #8650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef0a17e
Do not report serialize as unused
VincentLanglet Nov 2, 2022
0bdfc4b
Mark serialize as impure only for object
VincentLanglet Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/Psalm/Internal/Codebase/Functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,12 +518,27 @@ public function isCallMapFunctionPure(

//gettext
'bindtextdomain',

// unserialize
'unserialize'
];

if (in_array(strtolower($function_id), $impure_functions, true)) {
return false;
}

if ($function_id === 'serialize' && isset($args[0]) && $type_provider) {
$serialize_type = $type_provider->getType($args[0]->value);

if ($serialize_type) {
foreach ($serialize_type->getAtomicTypes() as $atomic_serialize_type) {
if ($atomic_serialize_type->isObjectType()) {
return false;
}
}
Comment on lines +534 to +538
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would work for anything but the simplest cases. E.g. serialize([new RuntimeException]); would still be marked as unused, wouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid the error for
String[] but didnt think about object[] indeed.

Do you see an easy way to improve this or should we test for lot of atomic types ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orklah @weirdan Seems like this work #8652

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, serialize() is impure when it operates on anything that may contain an object. Your PRs are focusing on the cases when serialize() is called on something that does contain an object. The distinction is important because there are supertypes to object types, such as object|int or iterable or mixed. In all of those cases serialize() should be considered impure:

function a(mixed $p): void { serialize($p); } // impure as an object may be passed here
a(new stdClass); 

function b(iterable $p): void { serialize($p); } // ditto
b(new class implements IteratorAggregate { 
   public function __serialize() { throw new RuntimeException(); } 
   public function getIterator() { return new ArrayIterator([]); }
});

However, that's only a part of the problem. The other part is that the types that may contain objects could themselves be a part of an arbitrarily deep type. Say you fixed the case of serialize([new stdClass]);, but then you have to fix serialize([[new stdClass]]); (note the array depth) and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ContainsObjectTypeVisitor is supposed to resolve the array depth issue, I can add a test about it

For the "may contain an object" issue, I'm not sure how I should change my

if ($type instanceof TObject
            || $type instanceof TNamedObject
            || ($type instanceof TTemplateParam
                && $type->as->hasObjectType())
        ) {

check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and didn't succeed to use this function because it doesn't work for arrays.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type visitor approach is fine, you can just use TypeComparator (either Union* or Atomic*, depending on what the visitor gets) to compare individual type parts.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way, you'd be able to handle iterable[][], for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR, it works with the previous test but still doesn't work with mixed or iterable.

Did I mess up with the check:

$type instanceof Union
            && UnionTypeComparator::canBeContainedBy($this->codebase, $type, new Union([new TObject()]))
            ||
            $type instanceof Atomic
            && AtomicTypeComparator::isContainedBy($this->codebase, $type, new TObject())

?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the types should be the other way around. The container is the $type in this case, and the $input_type is object, because you want to check whether the object can be contained by the $type.

You may have to allow coercion (not sure if that method allows it, but there should be one that does) because object is not strictly contained by, e.g., ClassName|int, but it can be coerced into.

}
}

if (strpos($function_id, 'image') === 0) {
return false;
}
Expand Down
25 changes: 25 additions & 0 deletions tests/UnusedCodeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,31 @@ public function unserialize($_serialized) : void {}

new Foo();'
],
'ignoreSerializeAndUnserialize' => [
'<?php
class Foo
{
public function __sleep(): array
{
throw new BadMethodCallException();
}
public function __wakeup(): void
{
throw new BadMethodCallException();
}
}

function test(): bool {
try {
serialize(new Foo());
unserialize("");
} catch (\Throwable) {
return false;
}

return true;
}'
],
'useIteratorMethodsWhenCallingForeach' => [
'<?php
/** @psalm-suppress UnimplementedInterfaceMethod */
Expand Down