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 count_chars stubs #7094

Merged
merged 2 commits into from Dec 8, 2021
Merged

Fix count_chars stubs #7094

merged 2 commits into from Dec 8, 2021

Conversation

kamil-tekiela
Copy link
Contributor

I am not sure why it doesn't work properly when they are all separated but it seems to work when they are combined with OR operator.

I don't know if there are tests I should write for this...

@orklah
Copy link
Collaborator

orklah commented Dec 7, 2021

Not sure either, never really looked at how Psalm finds the correct signature for a specific case.

I'd like some tests on this is possible FunctionCallTest::providerValidCodeParse would be a fine place for those.

In particular, I'd like to try what Psalm outputs for a call when Psalm doesn't know the literal value of $mode (a simple int for example)

@kamil-tekiela
Copy link
Contributor Author

@orklah Thanks for the hint. Can you help me a little bit with writing the test properly? I am not very familiar with this syntax.

@orklah
Copy link
Collaborator

orklah commented Dec 7, 2021

yeah, basically, you have a providerValidCodeParse and a providerInvalidCodeParse.

The first one is used to check that a snippet does not output errors while the second checks that you correctly emit a specific error.

For your tests, providerValidCodeParse is more on point.

You have to create a new offset in the big array. In the array you have:

  • the snippet of code
  • the second element is an array called assertions. You can put variable as offsets in there and specify the type of the variable as it should be inferred (Psalm will check that each variable have the right type).
  • Then you have an array of errors that will be ignored for this test
  • And finally you have the php version used by psalm to analyze this particular code

So, for example

'mb_strtolowerProducesLowercaseStringWithNullOrAbsentEncoding' => [

This test execute the snippet in PHP 8.1, check that $a and $b have the right types and will not tolerate any error.

(If you see === after a variable name in assertions, it's used to tell Psalm you want to check the detailed type (ex: int without ===, 6 with it)

@kamil-tekiela kamil-tekiela force-pushed the count_chars-fix branch 2 times, most recently from 67d4b2d to d274550 Compare December 7, 2021 23:44
@orklah
Copy link
Collaborator

orklah commented Dec 8, 2021

Can you add a test that shows what happens if you provide int to the mode? I'm curious to see what that will do

@kamil-tekiela
Copy link
Contributor Author

Can you add a test that shows what happens if you provide int to the mode? I'm curious to see what that will do

Isn't my second test showing this? I pass 5 which produces a warning saying that it should be 3|4

@orklah
Copy link
Collaborator

orklah commented Dec 8, 2021

you pass the literal 5, but I'd like to see what happens if you pass a variable that could contain any int.

For example

<?php
function scope(int $mode){
    $a = count_chars("foo", $mode);
}

@orklah
Copy link
Collaborator

orklah commented Dec 8, 2021

Well, the error message is kinda weird (incomplete at least) but I guess I'm fine with it.

Can you fix the test and I think we're good to merge

@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Dec 8, 2021
@orklah orklah merged commit 761d5f3 into vimeo:master Dec 8, 2021
@orklah
Copy link
Collaborator

orklah commented Dec 8, 2021

Thanks! That's great!

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

Successfully merging this pull request may close these issues.

None yet

2 participants