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

Specifiy json_decode default return type #1283

Closed

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented May 4, 2022

mixed is too vague IMO, this should be fine to specify with more details, see also https://3v4l.org/hEDja
But please somebody double and triple check that I didn't miss anything here. Together with #993 this allows us to get rid of stdClass for example.

I was also hoping that it would "fix" phpstan/phpstan#7073 but apparently NonexistentOffsetInArrayDimFetchRule is not happy with scalar null coalescing access. But not sure if that's coming from that rule or somewhere else CC @rajyan

@herndlm herndlm changed the base branch from 1.7.x to 1.6.x May 4, 2022 07:36
@herndlm herndlm force-pushed the specify-json-decode-default-return-type branch from b31db0e to d43c845 Compare May 4, 2022 07:36
@ondrejmirtes
Copy link
Member

I've rejected this idea in phpstan/phpstan-nette#89 (comment) and that's why I suggested to subtract from MixedType instead.

@herndlm
Copy link
Contributor Author

herndlm commented May 4, 2022

I've rejected this idea in phpstan/phpstan-nette#89 (comment) and that's why I suggested to subtract from MixedType instead.

ah I see, makes sense. ok then

@herndlm
Copy link
Contributor Author

herndlm commented May 4, 2022

I always forget that adding more type information / getting rid of mixed will potentially lead to more errors below level 9. I instinctively want to get rid of mixed everywhere :D

@herndlm herndlm closed this May 4, 2022
@herndlm herndlm deleted the specify-json-decode-default-return-type branch May 4, 2022 07:41
@rajyan
Copy link
Contributor

rajyan commented May 4, 2022

@herndlm
https://phpstan.org/r/de94db92-8cac-49f9-bac4-4eb8b7e76ec8
As you mentioned, this case (not related to json_decode) can be solved, but we have to be careful that the union type doesn't contain object related type, because array access on object throws a fatal error.
We can change here

if (($scope->isInExpressionAssign($node) || $scope->isUndefinedExpressionAllowed($node)) && $isOffsetAccessible->yes()) {

like I changed in AccessPropertiesRule with additional check for object? I think.
if ($type->canAccessProperties()->no() || $type->canAccessProperties()->maybe() && !$scope->isUndefinedExpressionAllowed($node)) {

It's just not fixed yet :)

@rajyan
Copy link
Contributor

rajyan commented May 4, 2022

I'll send a pull request for it!

@herndlm
Copy link
Contributor Author

herndlm commented May 4, 2022

ok sounds interesting, just don't spend too much time on the json_decode case. since mixed or mixed~stdClass is returned it might not work. but that's fine :)

@rajyan
Copy link
Contributor

rajyan commented May 4, 2022

My assumption was not correct...
https://phpstan.org/r/ff9a5441-1529-42a3-b1f6-3b7cb318a7da
While null and string is offsetAccessible. IntegerType, FloatType, BooleanType is not.
This means that we need a little more complicated check in case of $scope->isUndefinedExpressionAllowed($node) === true

if (($scope->isInExpressionAssign($node) || $scope->isUndefinedExpressionAllowed($node)) && $isOffsetAccessible->yes()) {

I'm not gonna try it now...

@rajyan
Copy link
Contributor

rajyan commented May 4, 2022

so, in case of $scope->isUndefinedExpressionAllowed($node) === true what we have to check is not $isOffsetAccessible->yes() but whether there is a Type that throws a fatal error on offset access even in null-coalesce scope. I will address this issue if more related issues are reported.

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