Skip to content

Fix ConstantArrayType not accepting NeverType #3327

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

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

tscni
Copy link
Contributor

@tscni tscni commented Aug 17, 2024

$this->analyse([__DIR__ . '/../Comparison/data/bug-5474.php'], [
[
'Parameter #1 $data of function Bug5474\testData expects array{test: int}, *NEVER* given.',
26,
],
]);
$this->analyse([__DIR__ . '/../Comparison/data/bug-5474.php'], []);
Copy link
Contributor Author

@tscni tscni Aug 17, 2024

Choose a reason for hiding this comment

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

This would be the consequence of the change. (See phpstan/phpstan#5474, https://phpstan.org/r/4ba9826f-8be3-4782-97ea-127ba76a9950)

At least to me that seems acceptable, as never is after all the bottom type. And the same issue doesn't appear for other types that already accept never anyway.
This rather seems like a now undesired artifact from before notIdentical.alwaysFalse or similar.

@@ -300,6 +300,10 @@ public function acceptsWithReason(Type $type, bool $strictTypes): AcceptsResult
return $type->isAcceptedWithReasonBy($this, $strictTypes);
}

if ($type instanceof NeverType) {
Copy link
Member

Choose a reason for hiding this comment

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

Please try instanceof CompoundType. You may also remove the above if with MixedType.

Copy link
Contributor Author

@tscni tscni Aug 22, 2024

Choose a reason for hiding this comment

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

Unfortunately the best we can do right now is $type instanceof CompoundType && !$type instanceof IntersectionType, because accessor based intersection types currently don't support "constant" arrays / array shapes.
E.g. array{foo: mixed} will not be accepted by array&hasOffset('foo')

ConstantArrayType always needs to look at the sum of the intersection types in that regard. (or maybe I'm missing some simple adjustment I could do)

I spent some time looking into this, specifically adjusting TypeCombinator::intersect() to combine array&hasOffset('test') directly into array{test: mixed} etc.
There are quite a few cases to think through there, so it'll take me some more time for a proposal, which would be better suited for a separate PR anyway.
Maybe I'll look into phpstan/phpstan#8438 (comment) instead, especially because avoiding general array shape intersection along with it seems to make it extra cumbersome to implement.

Anyway, right now I have the issue that instanceof IntersectionType is not allowed. Is there a better way (or do you envision one) to check for an intersection without adding an entry to the baseline?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for this analysis. How come you can dissect a problem so well? :) Did you spend a lot of time writing custom extensions or looking through the codebase before you started contributing? It's awesome how well you can solve problems here! I'm looking forward to more contributions from you.

I'm going to push an update to the baseline that makes the error go away, we're already ignoring it once in the same file.

Why instanceof *Type is discouraged is described here https://phpstan.org/blog/why-is-instanceof-type-wrong-and-getting-deprecated - I'd like to get rid of all instances of such code. It's relatively easy to do on the outskirts of the codebase (like in Rules etc.) but it's harder when you're deep in the typesystem or in TypeCombinator.

Every time I get rid of instanceof *Type somewhere, one or two bugs get squished.

Once we're able to get rid of all instanceof *Type, we'll be able to stop using inheritance in the typesystem - so for example, ConstantStringType won't longer extend StringType. This will untie our hands and for example make it easy to make any type subtractable. (Subtractable types: Right now for example MixedType is subtractable - we're able to tell "this is mixed but it's never an int").

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 do have quite a lot of experience with PHP and complex code bases, so questions like "how would I build this?" or "what would make sense here?" seem to often lead me into the right direction. :)
But mainly it's just a test case, a debugger and time :P (and having done some small bugfixes in the Psalm code base helped familiarize myself a bit with static analyzers in general)

Though I also do tend to skim through existing (merged) PRs and issues before contributing anywhere to familiarize myself with the expectations and opinions of the maintainers.

@ondrejmirtes
Copy link
Member

Sealed/unsealed array shapes are a huge undertaking. You can look into it, it'd be very valuable, but we have to work together on it really closely.

It's very similar to when list type was first introduced: #1751

Every time we handle a ConstantArrayType, we have to decide what happens when the type is sealed or unsealed, and if it's still sealed/unsealed after the operation.

@ondrejmirtes ondrejmirtes merged commit bbd64a9 into phpstan:1.11.x Aug 22, 2024
94 checks passed
@ondrejmirtes
Copy link
Member

Awesome, thank you!

@tscni
Copy link
Contributor Author

tscni commented Aug 23, 2024

Sealed/unsealed array shapes are a huge undertaking. You can look into it, it'd be very valuable, but we have to work together on it really closely.

It's very similar to when list type was first introduced: #1751

Every time we handle a ConstantArrayType, we have to decide what happens when the type is sealed or unsealed, and if it's still sealed/unsealed after the operation.

I'd plan to just start with a PR to add support for extra item types (...<K, V>) in the phpdoc-parser and then just do some experimentation, probably starting with a comprehensive test suite about all possible array code interactions I can think of.

@ondrejmirtes
Copy link
Member

I'm going to comment on the sealed vs. unsealed array shapes issue with some steps to follow. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants