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

Allow testing expected CallMap return types #8166

Merged
merged 1 commit into from Jul 6, 2022

Conversation

othercorey
Copy link
Contributor

@othercorey othercorey commented Jun 25, 2022

This test the CallMap return type against the reflected return type similar to the way parameter types are checked.

Because we already have an ignore list where users were only fixing parameters, this adds a separate ignore list that includes functions where only the return type is ignored. If a function is in the original $ignoredFunctions list, it skips all checks.

Cleaned up some of the debug testing code to handle unexpected types without killing the test script.

Return types that include resource fail when compared to the reflected type so they are ignored until that is resolved.

@othercorey
Copy link
Contributor Author

I added this because I didn't realized until after fixing the array_* function signatures that return types were ignored. I wanted a way to check those and also allow for other functions with parameter-only fixes.

@othercorey othercorey marked this pull request as draft June 25, 2022 22:21
@othercorey othercorey force-pushed the func-return-signatures branch 2 times, most recently from e7cddec to 8075cc4 Compare June 25, 2022 22:50
@othercorey othercorey marked this pull request as ready for review June 25, 2022 22:50
@othercorey
Copy link
Contributor Author

The tests are failing because the ignore list was generated with only some extensions enabled. Need to add the failing functions to the ignore list.

@AndrolGenhald AndrolGenhald added the release:internal The PR will be included in 'Internal changes' section of the release notes label Jun 25, 2022
@othercorey othercorey force-pushed the func-return-signatures branch 2 times, most recently from 24f2779 to 84dae7f Compare June 26, 2022 02:09
@othercorey
Copy link
Contributor Author

Looks like the psalm shepherd test needs to run with PHP 8.1 the same as the unit tests for this to pass.


if ($functionIgnored) {
try {
/** @var array<string, string> $callMapEntry */
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you document this with an @param maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added one for the CallMap entry. I don't think there's a way to define the shape so it's just array<int|string, string>

I am traveling until Friday so you can make changes or wait until I'm back for further issues.

@othercorey
Copy link
Contributor Author

othercorey commented Jul 4, 2022

@orklah Fixed the psalm errors and updated some parameter annotations. If I have time, I will come back for another round of clean up and try to get the class methods working as well. For now, this only adds return type testing for functions.

We need an equality check for the types instead of using IsContainedBy().

psalm.xml.dist Outdated
@@ -166,5 +166,12 @@
<directory name="vendor/nikic/php-parser" />
</errorLevel>
</MixedAssignment>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you suppress them with an @psalm-suppress instead please? Putting them here would mean any future usage of those outside of tests would not get flagged by Psalm so I'd like to avoid this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@AndrolGenhald
Copy link
Collaborator

We need an equality check for the types instead of using IsContainedBy()

That's something I plan to do with longer term type system improvements. There's Union::equals right now, but it's probably not what you want.

@othercorey
Copy link
Contributor Author

That's something I plan to do with longer term type system improvements. There's Union::equals right now, but it's probably not what you want.

The main issue this causes is nullable types aren't checked. If the PHP type is int|null and we just have int the test passes.

An example that is easter_days where year should be nullable:

'easter_days' => ['int', 'year='=>'int', 'mode='=>'int'],

https://www.php.net/manual/en/function.easter-days.php

@AndrolGenhald
Copy link
Collaborator

The main issue this causes is nullable types aren't checked. If the PHP type is int|null and we just have int the test passes.

Oh, I didn't notice that in the diff because it was already there. You can just do UnionTypeComparator::isContainedBy($codebase, $a, $b) && UnionTypeComparator::isContainedBy($codebase, $b, $a). It's a bit verbose, but it's the best way to do it for now.

@orklah orklah merged commit 38443ef into vimeo:4.x Jul 6, 2022
@orklah
Copy link
Collaborator

orklah commented Jul 6, 2022

Thanks!

@othercorey othercorey deleted the func-return-signatures branch July 6, 2022 07:56
@othercorey
Copy link
Contributor Author

Oh, I didn't notice that in the diff because it was already there. You can just do UnionTypeComparator::isContainedBy($codebase, $a, $b) && UnionTypeComparator::isContainedBy($codebase, $b, $a). It's a bit verbose, but it's the best way to do it for now.

FYI, a quick test shows this doesn't work with certain subsets such as for strlen. The reflection type is int which contains 0|positive-int but 0|positive-int does not contain int.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants