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

Fix some minor issues with references. #7684

Merged
merged 2 commits into from Feb 20, 2022

Conversation

AndrolGenhald
Copy link
Collaborator

References assigned to properties on $this should never be unused.
Using a @var docblock before a reference should be allowed if it targets a variable instead of the assignment statement.

References assigned to properties on `$this` should never be unused.
Using a `@var` docblock before a reference should be allowed if it targets a variable instead of the assignment statement.
@AndrolGenhald AndrolGenhald added the release:fix The PR will be included in 'Fixes' section of the release notes label Feb 17, 2022
@AndrolGenhald AndrolGenhald marked this pull request as draft February 17, 2022 16:28
@AndrolGenhald AndrolGenhald marked this pull request as ready for review February 17, 2022 16:40
@@ -1004,6 +1005,15 @@ public static function analyzeAssignmentRef(
: "property-assignment-as-reference"
);
}

if ($root_var_id === '$this') {
// Variables on `$this` are always used
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean that $this->whatever = $var constitutes a use of $var (but not necessarily $this->whatever)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it constitutes use of $this->whatever as well. We don't currently seem to track unused properties, and this caused me some issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reference assignment actually catches more than regular assignment for properties. I'm not too sure if it'd be better to just make it work the same or if non-reference assignment should be improved, so I decided to start small and make $this a special case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't currently seem to track unused properties,

We actually do, but you have to use the class itself and enable Detect unused classes and methods checkbox on psalm.dev: https://psalm.dev/r/031e79174f

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I missed that the class had to be used. That means this PR causes this to not mark Foo::$bar as unused, but I think that's an acceptable tradeoff for now. To fix it we'd need to somehow mark the assignment as used, but still keep the property itself as unused.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems ok if it only involves references

@orklah orklah merged commit b8cda9e into vimeo:master Feb 20, 2022
@orklah
Copy link
Collaborator

orklah commented Feb 20, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Need review 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.

None yet

3 participants