Skip to content

Fix mixing property and param attributes on promoted property#2825

Closed
RobertMe wants to merge 1 commit intophpstan:1.10.xfrom
RobertMe:mixed-param-attributes
Closed

Fix mixing property and param attributes on promoted property#2825
RobertMe wants to merge 1 commit intophpstan:1.10.xfrom
RobertMe:mixed-param-attributes

Conversation

@RobertMe
Copy link
Contributor

@RobertMe RobertMe commented Dec 13, 2023

Simplify the ParamAttributesRule by merging AttributeCheck::check calls and using an int mask to check for both TARGET_PARAMETER and TARGET_PROPERTY in one pass.

Fixes phpstan/phpstan#10298

@RobertMe RobertMe force-pushed the mixed-param-attributes branch from ec215cb to bad19f1 Compare December 13, 2023 14:58
Simplify the ParamAttributesRule by merging AttributeCheck::check calls
and using an int mask to check for both TARGET_PARAMETER and
TARGET_PROPERTY in one pass.

Fixes #10298
@RobertMe RobertMe force-pushed the mixed-param-attributes branch from bad19f1 to 0fc5866 Compare December 13, 2023 15:02
@ondrejmirtes
Copy link
Member

There is no bug to fix: phpstan/phpstan#10298 (comment)

public function __construct(
#[PropAttr]
#[ParamAttr]
public string $test
Copy link

@darenas31415 darenas31415 Jan 6, 2024

Choose a reason for hiding this comment

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

Coming from this issue phpstan/phpstan#10385 maybe you could improve the test suite by adding another test case to cover when an attribute has both property and parameter targets like:

#[\Attribute(\Attribute::TARGET_PROPERTY | \Attribute::TARGET_PARAMETER)]
class PropAndParamAttr {}

class Test
{
	public function __construct(
		#[PropAndParamAttr]
		public string $test
	) {}
}

Copy link
Member

Choose a reason for hiding this comment

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

I believe there is already such a test. Feel free to check that and send a PR in case it isn't 😊

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.

3 participants