Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Specifiy json_decode default return type #1283

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

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
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.

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

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 deleted the specify-json-decode-default-return-type branch May 4, 2022 07:41
Copy link
Contributor

rajyan commented May 4, 2022
edited
Loading

@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 :)

Copy link
Contributor

rajyan commented May 4, 2022

I'll send a pull request for it!

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 reacted with thumbs up emoji

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...

Copy link
Contributor

rajyan commented May 4, 2022
edited
Loading

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /