Friend functions #268

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

Jump to bottom

Closed

malarzm wants to merge 2 commits into phpstan:1.5.x from malarzm:friend-functions-v2

Closed

Friend functions #268

malarzm wants to merge 2 commits into phpstan:1.5.x from malarzm:friend-functions-v2

Conversation

@malarzm

Copy link

Contributor

@malarzm malarzm commented Jul 5, 2020 •

edited

Loading

First (completed) version of friend functions, closes phpstan/phpstan#3565

As for "What if we're in the same class for example?" I took the whitelist approach - if a user specifies @friend tag then they want to control who may access given method and it makes sense to restrict calls from same class as well. For instance from my original example, I wouldn't want anybody to call Category::addProduct from anywhere but the Product::addCategory. Also being friends is not inherited like showcased in the test.

Open issues that come to my mind:

There's one issue reported by PHPStan I'm not sure how to approach
Should FriendTag creation be hardened somehow?
I'm not fond of filtering FriendTags that contain TypeWithClassName in the rule class. Do you see a better place or is it ok?
Implement a rule for validating @friend annotations

@malarzm malarzm force-pushed the friend-functions-v2 branch 2 times, most recently from d32a628 to 57ec0b7 Compare

July 5, 2020 14:59

malarzm

malarzm commented

Jul 5, 2020

View reviewed changes

src/Rules/Methods/FriendMethodCallRule.php Outdated

Copy link

Contributor Author

@malarzm malarzm Jul 5, 2020

Choose a reason for hiding this comment

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

I'm not sure how to approach that this can be false. Should it also result in an early exit?

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

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

Yes, it means you're calling some internal PHP method.

Copy link

Contributor Author

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

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

Makes sense :)

ondrejmirtes

ondrejmirtes requested changes

Jul 5, 2020

View reviewed changes

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.

The rule needs to be added to a level, I'd prefer level2 (config.level2.neon).

src/PhpDoc/PhpDocNodeResolver.php Outdated

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

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

Other tags are filtered to the correct node with array_filter., it doesn't crash on wrong behaviour

src/PhpDoc/ResolvedPhpDocBlock.php Outdated

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

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

This property should be mentioned in merge method too.

Copy link

Contributor Author

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

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

I assume it should also read $result->friends = [] as friendship is not inherited? That's what I'm going with now

Copy link

Contributor Author

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

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

Or prolly not, added some cases to the test and turns out this way it should be inherited :)

src/Rules/Methods/FriendMethodCallRule.php Outdated

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

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

Yes, it means you're calling some internal PHP method.

src/Rules/Methods/FriendMethodCallRule.php Outdated

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

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

There should be a test with something like @friend bool to see that this code really works.

Copy link

Contributor Author

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

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

I'll filter these out in \PHPStan\PhpDoc\PhpDocNodeResolver::resolveFriends and make FriendTag always return TypeWithClassName. Will also add a test 👍

src/Rules/Methods/FriendMethodCallRule.php Outdated

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

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

I'd like array_values here too.

src/Rules/Methods/FriendMethodCallRule.php Outdated

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

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

Arguably these methods shouldn't be called outside of classes at all so this should be reported?

Copy link

Contributor Author

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

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

Will be :)

src/Rules/Methods/FriendMethodCallRule.php Outdated

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

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

%s::%s() for consistency + . at the end.

@ondrejmirtes

Copy link

Member

ondrejmirtes commented Jul 5, 2020

Nice job! 👍 It's really encouraging when someone that can find their way around PHPStan internals really fast shows up :)

@ondrejmirtes

Copy link

Member

ondrejmirtes commented Jul 5, 2020

Additionally, we could implement a separate rule that validates @friend above a method:

That the mentioned class exists (with ReflectionProvider injected through constructor)
That the frient method exists.

malarzm

malarzm commented

Jul 6, 2020

View reviewed changes

tests/PHPStan/Rules/Methods/FriendMethodCallRuleTest.php Outdated

Copy link

Contributor Author

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

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

I'm having a hard time figuring out why this violation doesn't pop up. I'll try to debug some more, pushing this already for other changes

Copy link

Contributor Author

@malarzm malarzm Jul 6, 2020 •

edited

Loading

Choose a reason for hiding this comment

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

$docComment = $method->getDocComment(); returns null for extended method and then rule is exiting early due to this. Is there a way to get a ResolvedPhpDocBlock for a method that would include all parents resolved?

Copy link

Contributor Author

@malarzm malarzm Jul 10, 2020

Choose a reason for hiding this comment

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

@ondrejmirtes any hint please?

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 10, 2020

Choose a reason for hiding this comment

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

Sorry, didn't notice you have a question here. Try something like this:

--- a/src/Reflection/Php/PhpClassReflectionExtension.php
+++ b/src/Reflection/Php/PhpClassReflectionExtension.php
@@ -498,6 +498,7 @@ class PhpClassReflectionExtension
 					$methodReflection->getName(),
 					$positionalParameterNames
 				);
+				$docComment = $resolvedPhpDoc->getPhpDocString();
 				$phpDocBlockClassReflection = $declaringClass;
 			}
 		} else {

Although I'm not sure + I don't know why the getPhpDocString() isn't nullable, it might be returning an empty string instead (which we should change to null here).

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 10, 2020

Choose a reason for hiding this comment

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

This definitely won't work, but at least I showed you the place that needs fixing :)

Copy link

Member

@ondrejmirtes ondrejmirtes Jul 10, 2020

Choose a reason for hiding this comment

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

(I'm in favor of having the inherited phpDoc be returned from a child's method getDocComment.)

@malarzm malarzm force-pushed the friend-functions-v2 branch from 8f0f4af to c3446d8 Compare

July 6, 2020 20:25

@ondrejmirtes

Copy link

Member

ondrejmirtes commented Jul 10, 2020

Alternatively what I'd like:

Make this PR green (besides the stuff that's currently failing on master, sorry about that). It won't support PHPDoc inheritance which is fine (remove tests about that).
Make another PR that makes MethodReflection::getDocComment() return inherited PHPDoc as well, because that's a separate feature.
Make a PR that adds the tests that will now be green from 1).

malarzm added 2 commits

July 12, 2020 16:47

@malarzm


 Friend functions

1240b10

@malarzm


 Add rule validating @Friend tags

a132192

@malarzm malarzm force-pushed the friend-functions-v2 branch from c3446d8 to a132192 Compare

July 12, 2020 15:15

@malarzm

Copy link

Contributor Author

malarzm commented Jul 12, 2020

@ondrejmirtes done! I think all that fails is not my fault :) From the original plan I had in the PR description I have removed a rule for static method calls as that just turned out to be something totally different when trying to determine a callee... so if that's fine with you I'll make a separate PR for that as a step 4

@ondrejmirtes ondrejmirtes force-pushed the master branch 2 times, most recently from f54fd5f to eca550a Compare

October 28, 2020 19:41

@ondrejmirtes ondrejmirtes force-pushed the master branch 3 times, most recently from d45166a to 3526237 Compare

December 12, 2020 10:56

@ondrejmirtes ondrejmirtes force-pushed the master branch 2 times, most recently from efd31ce to 0471f87 Compare

July 29, 2021 14:08

@ondrejmirtes ondrejmirtes force-pushed the master branch from 97c2f21 to a03a78f Compare

December 19, 2021 16:34

@ondrejmirtes ondrejmirtes force-pushed the 1.5.x branch 3 times, most recently from ddd20b4 to 95d480b Compare

March 18, 2022 19:53

@ondrejmirtes ondrejmirtes force-pushed the 1.5.x branch from 20539b3 to 9bb13b0 Compare

April 19, 2022 11:20

@malarzm

Copy link

Contributor Author

malarzm commented Jun 23, 2022

Closing as there's an external package providing the functionality: https://github.com/DaveLiddament/php-language-extensions#friend

@malarzm malarzm closed this

Jun 23, 2022

Labels

None yet

2 participants

@malarzm @ondrejmirtes

Friend functions #268

Friend functions #268

Uh oh!

Conversation

@malarzm malarzm commented Jul 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@malarzm malarzm Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 5, 2020

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Jul 5, 2020

Uh oh!

ondrejmirtes commented Jul 5, 2020

Uh oh!

@malarzm malarzm Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

@malarzm malarzm Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@malarzm malarzm Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Jul 10, 2020

Uh oh!

malarzm commented Jul 12, 2020

Uh oh!

malarzm commented Jun 23, 2022

@malarzm malarzm commented Jul 5, 2020 •

edited

Loading

@malarzm malarzm Jul 6, 2020 •

edited

Loading