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.

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

Fix/issue 7144 #1299

Closed
wants to merge 4 commits into from
Closed

Fix/issue 7144 #1299

wants to merge 4 commits into from

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented May 11, 2022

fixes phpstan/phpstan#7144

The root cause of this issue is expressed in this test dc5e865.

When specifying ArrayDimFetch type, it was relying only on $dimType, which can be any type as seen in the test.
The type to be specified should be only be a key existing in array`.

@ondrejmirtes
Copy link
Member

I think that somehow, you need to handle multidimensional arrays too. The new error in CI is a false-positive: https://github.com/composer/composer/blob/3ae111140facdba8ae82adcd1085e4adfc7d715c/src/Composer/Util/Http/CurlDownloader.php#L230-L236


 ------ ----------------------------------------------------------------- 
  Line   src/Composer/Util/Http/CurlDownloader.php                        
 ------ ----------------------------------------------------------------- 
  232    Offset 'ssl' does not exist on array{http: array, ssl?: array}.  
  232    Offset 'ssl' does not exist on array{http: array, ssl?: array}.  
 ------ ----------------------------------------------------------------- 

@rajyan
Copy link
Contributor Author

rajyan commented May 11, 2022

@ondrejmirtes
It is a regression, but is a expected behavior for me.
This PR could make the type of $option in foreach more specified (correct? I think) compared to the current playground result which lead to this regression.

To solve this case we need to implement type specification of isset with expr key.

Maybe I should solve these issues before this PR to avoid these kind of regressions (I'm looking into these issues but not easy to solve...)
phpstan/phpstan#6508
phpstan/phpstan#7000

if (isset($options[$type][$name])) {
\PHPStan\Testing\assertType('array{http: array, ssl?: array}', $options);
if ($type === 'ssl' && $name === 'verify_peer_name') {
\PHPStan\Testing\assertType('array{http: array, ssl?: array}', $options);
Copy link
Contributor Author

@rajyan rajyan May 11, 2022

Choose a reason for hiding this comment

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

The goal for #1299 (comment) is to make $options[$type] specified by isset and also from $type === 'ssl' the array type can be array{http: array, ssl: array} (which looks not that easy to solve, because it is a result of multiple conditional expressions)

@ondrejmirtes
Copy link
Member

I'm happy with the improved type inference of this PR, I just don't want this code https://github.com/composer/composer/blob/3ae111140facdba8ae82adcd1085e4adfc7d715c/src/Composer/Util/Http/CurlDownloader.php#L230-L236 to report these errors:

 ------ ----------------------------------------------------------------- 
  Line   src/Composer/Util/Http/CurlDownloader.php                        
 ------ ----------------------------------------------------------------- 
  232    Offset 'ssl' does not exist on array{http: array, ssl?: array}.  
  232    Offset 'ssl' does not exist on array{http: array, ssl?: array}.  
 ------ ----------------------------------------------------------------- 

Because the code involves expressions like isset($options[$type][$name]), this cannot be solved entirely by the typesystem. But I think it could be solved in NonexistentOffsetInArrayDimFetchRule / NonexistentOffsetInArrayDimFetchCheck. Because surely, isset($options[$type][$name]) makes $options[$type][$name] appear in MutatingScope::$moreSpecificTypes. So this rule/check could ask Scope::isSpecified() about this to prevent the false positive.

@rajyan
Copy link
Contributor Author

rajyan commented May 11, 2022

I had investigated this issue in #1187.
The problem is, because type of array before and after isset doesn't change in these cases 83fe253, phpstan/phpstan#7000

Because surely, isset($options[$type][$name]) makes $options[$type][$name] appear in MutatingScope::$moreSpecificTypes.

this part is not true due to

if (
!$expr instanceof Variable
&& $exprType->equals($typeAfterRemove)
&& !$exprType instanceof ErrorType
&& !$exprType instanceof NeverType
) {
return $this;
}

I tried to remove this line, but was not sure how to solve the failing tests.

I'll look into it again!:+1:

@rajyan rajyan marked this pull request as draft May 11, 2022 12:40
@ondrejmirtes
Copy link
Member

The linked lines are supposed to be just an optimization that we might be able to remove I think. (But it's a long time since I touched these parts last time).

@rajyan rajyan mentioned this pull request May 12, 2022
@rajyan rajyan mentioned this pull request May 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants