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

Correct hash function stub return type #343

Merged
merged 4 commits into from Oct 14, 2020
Merged

Conversation

Synchro
Copy link
Contributor

@Synchro Synchro commented Oct 14, 2020

Correct return type of the hash function. The PHP docs say that this function only returns a string, however this is not true. It can also return a boolean false if you pass in the name of a non-existent hash algorithm. For example:

var_dump(@hash('foo', 'bar', true));
bool(false)

@ondrejmirtes
Copy link
Member

This need a similar treatment hash_hmac got in #328.

@Synchro
Copy link
Contributor Author

Synchro commented Oct 14, 2020

Ah. OK. At least it doesn't have the PHP 7.1 problem as hash_algos is old enough. I would have a go at that, but I'm a bit stuck for now as #109 prevents me working on a copy locally.

@ondrejmirtes
Copy link
Member

I just merged #109 so let me know if master works for you :)

@ondrejmirtes
Copy link
Member

Please rebase instead of creating these merge commits.

Correct return type of the `hash` function. [The PHP docs](https://www.php.net/manual/en/function.hash.php) say that this function always returns a string, however this is not true. It can also return a boolean `false` if you pass in the name of a non-existent hash algorithm. For example:

    var_dump(@hash('foo', 'bar', true));
    bool(false)
@Synchro
Copy link
Contributor Author

Synchro commented Oct 14, 2020

Thanks for merging #109, it did indeed clean up my issue there.

I've had a go at addressing this issue, looks like it's not quite right – I think I've not understood what NodeScopeResolverTest does. The $hashRandom value is expected to be false as the algo doesn't exist – so why is it expecting string|false there?

I have rebased and force-pushed, but I'm not certain if it's made my earlier merge go away.

@ondrejmirtes
Copy link
Member

I don't see the conflict - false is expected for 'random' algorithm, so what's the problem?

@Synchro
Copy link
Contributor Author

Synchro commented Oct 14, 2020

The build fails saying it's expecting string|false when I told it to expect false. At least I think that's what I told it!

Similarly the other two that are expected to return string are complaining that they are not returning string|false. I think I have misinterpreted the scope resolver thing – but as far as I can see I followed what is done for the hash_hmac cases.

			[
				'string',
				'$hash',
			],
			[
				'string',
				'$hashRaw',
			],
			[
				'false',
				'$hashRandom',
			],

@Synchro
Copy link
Contributor Author

Synchro commented Oct 14, 2020

In case it's unclear, I could make this pass by setting all the expected values to string|false, but that seems like the wrong approach; I don't know why it's not expecting the values I've provided.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Just added three things that should make the tests pass :)

src/Type/Php/HashFunctionsReturnTypeExtension.php Outdated Show resolved Hide resolved
if ($functionReflection->getName() === 'hash') {
$defaultReturnType = new StringType();
} else {
$defaultReturnType = ParametersAcceptorSelector::selectSingle($functionReflection->getVariants())->getReturnType();
Copy link
Member

Choose a reason for hiding this comment

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

But we want this code be executed ;)

}
$string = $values[0];

return in_array($string->getValue(), hash_algos(), true) ? $defaultReturnType : new ConstantBooleanType(false);
Copy link
Member

Choose a reason for hiding this comment

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

This should be:

in_array($string->getValue(), hash_algos(), true) ? new StringType() : new ConstantBooleanType(false)

tests/PHPStan/Analyser/data/functions.php Show resolved Hide resolved
@Synchro
Copy link
Contributor Author

Synchro commented Oct 14, 2020

So, still failing the same way 🤷‍♂️

@ondrejmirtes
Copy link
Member

You haven't registered the extension in config.neon...

@ondrejmirtes
Copy link
Member

It's not executed at all.

@ondrejmirtes ondrejmirtes merged commit 41c4e02 into phpstan:master Oct 14, 2020
@ondrejmirtes
Copy link
Member

Thank you!

@Synchro
Copy link
Contributor Author

Synchro commented Oct 14, 2020

Yay! Thanks for helping me through this - it's a very complex project you've built here.

@Synchro Synchro deleted the hashType branch October 14, 2020 17:36
@ondrejmirtes
Copy link
Member

It's a complex field :)

}
$string = $values[0];

return in_array($string->getValue(), hash_algos(), true) ? new StringType() : new ConstantBooleanType(false);
Copy link
Contributor

@staabm staabm Oct 14, 2020

Choose a reason for hiding this comment

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

Would it be useful/possible to directly emit a phpstan error when hash() is used with a hash-algo not defined in hash_algos()?

Copy link
Member

Choose a reason for hiding this comment

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

You’ll get that error if you try to use false as a string.

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