Skip to content

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented May 30, 2022

This is basically the refactoring I had in mind in #1362

The current state is already a bit weird IMO, in general trait props and methods were ignored when gathering class nodes. Then recently I added trait props to a dedicated array, just to use them also via uninitialized props checks. I did not add them to the normal props because that would lead to annoying new errors via UnusedPrivatePropertyRule.
Then just yesterday I added trait property promotion usage to the normal property usage, but ignoring other potential trait prop usages elsewhere. And trait methods are still completely ignored.

So what does this do different now?
Trait props and methods are gathered together with their class counterparts. And rules just use that. Additionally the information if the prop or method came from a trait is also remembered and only that is used to skip trait props / methods in those 2 UnusedPrivate* rules, that were annoying.

I think in the long-run this makes more sense and avoids more weird trait problems. But maybe the details could be modified a bit. WDYT?

@herndlm
Copy link
Contributor Author

herndlm commented May 30, 2022

Aah I guess I could keep BC here still if really needed ;)
I added that method in #1340 which was released in 1.7.0 a week ago. Do you think we need it?

Oh and sorry for the title rename. I messed it up..

@herndlm herndlm force-pushed the refactor-trait-props-and-methods-in-class-nodes branch from 347e751 to fecbc7c Compare May 30, 2022 21:54
@herndlm herndlm changed the title Make trait properties and methods first citizens of class nodes Make trait properties and methods first class node cites May 30, 2022
@herndlm herndlm changed the title Make trait properties and methods first class node cites Make trait properties and methods first class node citizens May 30, 2022
@herndlm herndlm marked this pull request as ready for review June 1, 2022 06:09
@herndlm herndlm force-pushed the refactor-trait-props-and-methods-in-class-nodes branch from fecbc7c to b694a3f Compare June 1, 2022 06:09
@herndlm herndlm force-pushed the refactor-trait-props-and-methods-in-class-nodes branch from b694a3f to 42e024a Compare June 1, 2022 20:23
@herndlm
Copy link
Contributor Author

herndlm commented Jun 1, 2022

I added a new virtual node ClassMethodNode and added isDeclaredInTrait to it and to ClassPropertyNode. In general I like that approach, but there are a couple of open questions for those two classes:

  • Currently I only need the name, private visibility yes/no and the trait info. But I already started duplicating other ClassMethod methods like isPublic, isStatic, .. how far should we go here? it's not complete yet either, e.g. getParams, getReturnType, .. are all still missing. I don't like the intermediate state
  • The duplicated methods are, well, duplicated. Would it make more sense to either store $originalNode or use it in the constructor to get the needed info? Or should as much as possible be injected via constructor?
  • I assume ClassPropertyNode grew a bit over time, e.g. it didn't have the $originalNode in it's initial state, whatever we do here, should we do this to ClassPropertyNode too then? Maybe for consistency reasons a bit of cleanup afterwards would be good :) but currently I don't fully understand where to go with those 2 classes exactly.

I guess my first intuition would be to kind of decorate a ClassMethod, meaning to hold one internally and support the same methods by passing everything through. Or just expose that internal node completely. But additionally add my custom method. Is this problematic memory-wise I assume?

UPDATE: I was so focused on ClassPropertyNode that I completely overlooked your other example and the fact that they don't have to be a real node.. Currently cleaning this up heavily :) And I guess ClassPropertyNode can't be cleaned-up easily, looks like it's in BC-lockdown mode

@herndlm herndlm marked this pull request as draft June 1, 2022 20:33
@herndlm herndlm force-pushed the refactor-trait-props-and-methods-in-class-nodes branch 2 times, most recently from c19dd15 to 8c74303 Compare June 1, 2022 21:08
@herndlm herndlm force-pushed the refactor-trait-props-and-methods-in-class-nodes branch from 8c74303 to 9eee131 Compare June 1, 2022 21:11
@herndlm herndlm marked this pull request as ready for review June 1, 2022 21:18
@herndlm herndlm requested a review from ondrejmirtes June 1, 2022 21:18
@ondrejmirtes
Copy link
Member

Nice, thank you!

@ondrejmirtes ondrejmirtes merged commit d0a9d08 into phpstan:1.7.x Jun 2, 2022
@herndlm herndlm deleted the refactor-trait-props-and-methods-in-class-nodes branch June 2, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants