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

Add Exception->getCode() return type provider #7390

Merged
merged 8 commits into from Jan 24, 2022

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jan 13, 2022

Close #4295

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Jan 13, 2022
@VincentLanglet
Copy link
Contributor Author

@orklah Seems like my return type provider is not used. Any idea why ?

@orklah
Copy link
Collaborator

orklah commented Jan 13, 2022

Any idea why ?

Yeah, it's because I'm a moron 😄

MethodReturnTypes are actually registered here:

$this->registerClass(ClosureFromCallableReturnTypeProvider::class);

@VincentLanglet VincentLanglet force-pushed the exceptionCode branch 3 times, most recently from a950b89 to 2fa0a3a Compare January 13, 2022 22:56
@orklah
Copy link
Collaborator

orklah commented Jan 15, 2022

@weirdan This will be an important change. If you take a look at test with real projects, you'll see that phpunit for example has a lot of redundant casts now.

This is probably due to Psalm not being able to infer the exact type before, it pushed users into adding cast to please static analyzers, but now it will treat those as redundant.

I talked with @VincentLanglet about this and suggested adding this to Psalm 5, but he made a good point mentioning this PR is the equivalent of PHPStan behaviour right now and for users that like to use both, this difference makes things difficult. (they're forced to suppress everything on one analyzer or the other because behaviour is opposed)

What's your take on this? Can we release that on 4.x?

@weirdan
Copy link
Collaborator

weirdan commented Jan 24, 2022

If we marked the int type generated here with the from_calculation flag, casts would be allowed but not required:

if ($maybe_type->isInt()) {
$valid_int_type = $maybe_type;
if (!$maybe_type->from_calculation) {
self::handleRedundantCast($maybe_type, $statements_analyzer, $stmt);
}

@weirdan
Copy link
Collaborator

weirdan commented Jan 24, 2022

And then we could drop the flag in Psalm 5

@orklah
Copy link
Collaborator

orklah commented Jan 24, 2022

Good,idea :)

@VincentLanglet can you apply the changes?

@VincentLanglet
Copy link
Contributor Author

Done @orklah

@orklah
Copy link
Collaborator

orklah commented Jan 24, 2022

Thanks!

@orklah orklah merged commit 0619b40 into vimeo:4.x Jan 24, 2022
@VincentLanglet
Copy link
Contributor Author

Thanks!

You're welcome. Any plan for the next release ?

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

Successfully merging this pull request may close these issues.

False positive on Exception::getCode
4 participants