Skip to content

Commit

Permalink
Prevent unnecessary construction of union types during truthiness checks
Browse files Browse the repository at this point in the history
  • Loading branch information
muglug committed Dec 24, 2021
1 parent 75e4e0b commit 4dfc7ce
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 168 deletions.
10 changes: 5 additions & 5 deletions src/Psalm/Internal/Type/SimpleAssertionReconciler.php
Expand Up @@ -2237,13 +2237,13 @@ private static function reconcileFalsyOrEmpty(
foreach ($existing_var_type->getAtomicTypes() as $existing_var_type_key => $existing_var_type_part) {
//if any atomic in the union is either always truthy, we remove it. If not always falsy, we mark the check
//as not redundant.
$union = new Union([$existing_var_type_part]);

This comment has been minimized.

Copy link
@orklah

orklah Dec 24, 2021

Collaborator

@muglug was this a performance issue or more of a code style issue?

This comment has been minimized.

Copy link
@muglug

muglug Dec 24, 2021

Author Collaborator

Code style, but since the Union constructor is not zero-cost it probably also has a minor performance benefit.

$union->possibly_undefined = $existing_var_type->possibly_undefined;
$union->possibly_undefined_from_try = $existing_var_type->possibly_undefined_from_try;
if ($union->isAlwaysTruthy()) {
if (!$existing_var_type->possibly_undefined
&& !$existing_var_type->possibly_undefined_from_try
&& $existing_var_type_part->isTruthy()
) {
$did_remove_type = true;
$existing_var_type->removeType($existing_var_type_key);
} elseif (!$union->isAlwaysFalsy()) {
} elseif (!$existing_var_type_part->isFalsy()) {
$did_remove_type = true;
}
}
Expand Down
10 changes: 5 additions & 5 deletions src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php
Expand Up @@ -590,13 +590,13 @@ private static function reconcileFalsyOrEmpty(
foreach ($existing_var_type->getAtomicTypes() as $existing_var_type_key => $existing_var_type_part) {
//if any atomic in the union is either always falsy, we remove it. If not always truthy, we mark the check
//as not redundant.
$union = new Union([$existing_var_type_part]);
$union->possibly_undefined = $existing_var_type->possibly_undefined;
$union->possibly_undefined_from_try = $existing_var_type->possibly_undefined_from_try;
if ($union->isAlwaysFalsy()) {
if ($existing_var_type_part->isFalsy()) {
$did_remove_type = true;
$existing_var_type->removeType($existing_var_type_key);
} elseif (!$union->isAlwaysTruthy()) {
} elseif ($existing_var_type->possibly_undefined
|| $existing_var_type->possibly_undefined_from_try
|| !$existing_var_type_part->isTruthy()
) {
$did_remove_type = true;
}
}
Expand Down
159 changes: 159 additions & 0 deletions src/Psalm/Type/Atomic.php
Expand Up @@ -25,17 +25,23 @@
use Psalm\Type\Atomic\TClassStringMap;
use Psalm\Type\Atomic\TClosedResource;
use Psalm\Type\Atomic\TClosure;
use Psalm\Type\Atomic\TDependentGetClass;
use Psalm\Type\Atomic\TEmpty;
use Psalm\Type\Atomic\TEmptyMixed;
use Psalm\Type\Atomic\TEmptyNumeric;
use Psalm\Type\Atomic\TEmptyScalar;
use Psalm\Type\Atomic\TFalse;
use Psalm\Type\Atomic\TFloat;
use Psalm\Type\Atomic\TGenericObject;
use Psalm\Type\Atomic\THtmlEscapedString;
use Psalm\Type\Atomic\TInt;
use Psalm\Type\Atomic\TIntRange;
use Psalm\Type\Atomic\TIterable;
use Psalm\Type\Atomic\TKeyedArray;
use Psalm\Type\Atomic\TList;
use Psalm\Type\Atomic\TLiteralClassString;
use Psalm\Type\Atomic\TLiteralFloat;
use Psalm\Type\Atomic\TLiteralInt;
use Psalm\Type\Atomic\TLiteralString;
use Psalm\Type\Atomic\TLowercaseString;
use Psalm\Type\Atomic\TMixed;
Expand Down Expand Up @@ -651,4 +657,157 @@ public function equals(Atomic $other_type, bool $ensure_source_equality): bool
{
return get_class($other_type) === get_class($this);
}

public function isTruthy(): bool

This comment has been minimized.

Copy link
@orklah

orklah Dec 24, 2021

Collaborator

@muglug At one point, I wondered if this should have been defined by each Atomic instead of a big function like this. Do you think it would be worth it?

This comment has been minimized.

Copy link
@muglug

muglug Dec 24, 2021

Author Collaborator

Not really worth it — I think it makes sense to keep all this logic in one place, especially because many types are just returning false (so there'd be quite a bit of boilerplate)

This comment has been minimized.

Copy link
@orklah

orklah Dec 24, 2021

Collaborator

Thanks!

{
if ($this instanceof TTrue) {
return true;
}

if ($this instanceof TLiteralInt && $this->value !== 0) {
return true;
}

if ($this instanceof TLiteralFloat && $this->value !== 0.0) {
return true;
}

if ($this instanceof TLiteralString &&
($this->value !== '' && $this->value !== '0')
) {
return true;
}

if ($this instanceof TNonFalsyString) {
return true;
}

if ($this instanceof TCallableString) {
return true;
}

if ($this instanceof TNonEmptyArray) {
return true;
}

if ($this instanceof TNonEmptyScalar) {
return true;
}

if ($this instanceof TNonEmptyList) {
return true;
}

if ($this instanceof TNonEmptyMixed) {
return true;
}

if ($this instanceof TObject) {
return true;
}

if ($this instanceof TNamedObject
&& $this->value !== 'SimpleXMLElement'
&& $this->value !== 'SimpleXMLIterator') {
return true;
}

if ($this instanceof TIntRange && !$this->contains(0)) {
return true;
}

if ($this instanceof TPositiveInt) {
return true;
}

if ($this instanceof TLiteralClassString) {
return true;
}

if ($this instanceof TClassString) {
return true;
}

if ($this instanceof TDependentGetClass) {
return true;
}

if ($this instanceof TTraitString) {
return true;
}

if ($this instanceof TResource) {
return true;
}

if ($this instanceof TKeyedArray) {
foreach ($this->properties as $property) {
if ($property->possibly_undefined === false) {
return true;
}
}
}

if ($this instanceof TTemplateParam && $this->as->isAlwaysTruthy()) {
return true;
}

//we can't be sure the type is always truthy
return false;
}

public function isFalsy(): bool
{
if ($this instanceof TFalse) {
return true;
}

if ($this instanceof TLiteralInt && $this->value === 0) {
return true;
}

if ($this instanceof TLiteralFloat && $this->value === 0.0) {
return true;
}

if ($this instanceof TLiteralString &&
($this->value === '' || $this->value === '0')
) {
return true;
}

if ($this instanceof TNull) {
return true;
}

if ($this instanceof TEmptyMixed) {
return true;
}

if ($this instanceof TEmptyNumeric) {
return true;
}

if ($this instanceof TEmptyScalar) {
return true;
}

if ($this instanceof TTemplateParam && $this->as->isAlwaysFalsy()) {
return true;
}

if ($this instanceof TIntRange &&
$this->min_bound === 0 &&
$this->max_bound === 0
) {
return true;
}

if ($this instanceof TArray && $this->getId() === 'array<empty, empty>') {
return true;
}

//we can't be sure the type is always falsy
return false;
}
}

0 comments on commit 4dfc7ce

Please sign in to comment.