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

[CountOnNullRector] Should understand array/countable variable in trait method #2650

Conversation

gnutix
Copy link
Contributor

@gnutix gnutix commented Jan 12, 2020

This PR adds two fixtures relating to trait handling by CountOnNullRector. Currently :

  • PropertyWithinTraitMethod : passes 👍
  • VariableWithinTraitMethod : fails 👎

If I change the Fixture from a trait to a class they both pass, so this is related to how a variable within a trait is handled.

I spent some time digging and tried to use \Rector\NodeTypeResolver\PerNodeTypeResolver\VariableTypeResolver::resolve() that seems to be written to handle resolution of variables (though it's used nowhere in Rector's code, only in its own Test file 🤔 ) by calling :

  • resolveTypesFromScope($node), which calls...
  • resolveNodeScope($node), which calls on \Rector\NodeTypeResolver\PHPStan\Collector\TraitNodeScopeCollector...
  • getScopeForTraitAndNode($traitName, $node))

But it returned null, which was resolved to MixedType in resolveTypesFromScope and bubbled up back to \Rector\Php71\Rector\FuncCall\CountOnNullRector::refactor which passes the isNullableType check and thus refactors the code.

@TomasVotruba Any idea how to deal with this one ?

@gnutix
Copy link
Contributor Author

gnutix commented Jan 12, 2020

Could this be related to phpstan/phpstan#1601 (fixed in the new release by PHPStan 0.12.5) ?

@TomasVotruba
Copy link
Member

TomasVotruba commented Jan 12, 2020

Could this be related to phpstan/phpstan#1601 (fixed in the new release by PHPStan 0.12.5) ?

We might guess, but testing would answer us.

@TomasVotruba
Copy link
Member

TomasVotruba commented Jan 12, 2020

master now has phpstan/phpstan 0.12.5.

I tried to re-run checks, but I think you'll have to rebase

@gnutix gnutix force-pushed the CountOnNullRector/should-understand-variables-in-trait-methods branch from bdad270 to 13ed224 Compare January 13, 2020 10:27
@gnutix
Copy link
Contributor Author

gnutix commented Jan 13, 2020

Last push force is the rebase on master. We'll see.

@gnutix
Copy link
Contributor Author

gnutix commented Jan 14, 2020

@TomasVotruba Not related to PHPStan 0.12.5. Any idea ?

@TomasVotruba
Copy link
Member

@gnutix What do you mean? Link might help

@gnutix
Copy link
Contributor Author

gnutix commented Jan 14, 2020

I mean that I've rebased this PR on master, which includes PHPStan 0.12.15, and it didn't fix the broken test. So the issue remains, and I have no lead / clue on why this is broken nor how to fix this. So I'm asking for your help / guidance. :)

@TomasVotruba
Copy link
Member

This might be more tricky. Traits sucks hard.

Are you ok with merging as it is?
I'll have to look at it locally with trial error.

@gnutix
Copy link
Contributor Author

gnutix commented Feb 14, 2020

Sure!

@TomasVotruba TomasVotruba merged commit 49eb6ef into rectorphp:master Feb 16, 2020
@TomasVotruba TomasVotruba deleted the CountOnNullRector/should-understand-variables-in-trait-methods branch February 16, 2020 16:50
@TomasVotruba
Copy link
Member

Both passes after merge 👍 probably fixed by type refactoring

@TomasVotruba
Copy link
Member

Or not... false positive: paths have changed from packages to rules

@TomasVotruba
Copy link
Member

TomasVotruba commented Feb 16, 2020

After 30-40 minutes of digging, traits are impossible to analyse without class by PHPStan. It provides many false positives on standalone analysis, so we better skip them.

If you find a stable way of using PHPStan for traits here, I'm open to the PR.

TomasVotruba added a commit that referenced this pull request Jul 10, 2022
rectorphp/rector-src@7e1c2bb [TypeDeclaration] Skip Type modified between Assign and Return_ on ReturnTypeFromReturnNewRector (#2650)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants