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

[TypeDeclaration] Adds AddClosureParamTypeFromArgRector & AddClosureParamTypeFromArgClassStringRector rule #6198

Closed

Conversation

peterfox
Copy link
Contributor

@peterfox peterfox commented Jul 28, 2024

Changes

  • Implements a new AddClosureParamTypeFromArgRector and AddClosureParamTypeFromArgClassStringRector rule.
  • Adds the AddClosureParamTypeFromArgRector value object for the rule.
  • Adds test and fixtures.
  • Improvements to AddParamTypeForFunctionLikeWithinCallLikeArgDeclarationRector

Why

Sometimes you want to apply the type to a FunctionLike Param based on other Args to the parent method, an example in Laravel:

-$app->extend(Foo::class, function ($fooInstance) {
+$app->extend(Foo::class, function (Foo $fooInstance) {
});
-$object->tap('foobar', function ($foobarString) {
+$object->tap('foobar', function (string $fooInstance) {
});

Notes

Now implemented as a single rule that can detect either a class constant string or it can use the type from the argument.

@peterfox peterfox force-pushed the feature/argument-type-closure branch from 05a193a to 5d62403 Compare August 12, 2024 20:58
@peterfox peterfox force-pushed the feature/argument-type-closure branch from 5d62403 to 18bc621 Compare August 21, 2024 15:51
@peterfox peterfox changed the title [TypeDeclaration] Allow for dynamic type generation for FunctionLike type declaration [TypeDeclaration] Adds AddParamTypeForFunctionLikeWithinCallLikeArgFromArgDeclarationRector rule Aug 22, 2024
@peterfox peterfox requested a review from TomasVotruba August 22, 2024 23:50
Comment on lines 125 to 131
if (($node->name ?? null) === null) {
continue;
}

if (! $node->name instanceof Identifier) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (($node->name ?? null) === null) {
continue;
}
if (! $node->name instanceof Identifier) {
continue;
}
if (! $node->name instanceof Identifier) {
continue;
}

I think this has the same result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails PHPStan instead I've done:

if (! ($node->name ?? null) instanceof Identifier) {
    continue;
}

public function __construct(
private string $className,
private string $methodName,
private int|string $callLikePosition,
Copy link
Member

Choose a reason for hiding this comment

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

Support for int is enough here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? If it's an arg it could be a named one and that might cause an issue for some people. It makes little difference to the rule but make rules more compatible with code.

Comment on lines 65 to 68
public function onlyAcceptClassString(): bool
{
return $this->onlyAcceptClassString;
}
Copy link
Member

Choose a reason for hiding this comment

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

How useful is this practise? I'd expect type should be filled in any case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've solved this by just splitting the rules into two unique ones.

The problem was without a mechanism to seperate the two, Class const would mean sometimes you might want a string for a type, it just seemed messy to not be specific

Copy link
Member

@TomasVotruba TomasVotruba left a comment

Choose a reason for hiding this comment

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

Looks good 👍 I've added few notes

@peterfox peterfox force-pushed the feature/argument-type-closure branch from 33dd198 to 2615b17 Compare August 25, 2024 12:51
@peterfox peterfox requested a review from TomasVotruba August 25, 2024 13:00
@peterfox peterfox changed the title [TypeDeclaration] Adds AddParamTypeForFunctionLikeWithinCallLikeArgFromArgDeclarationRector rule [TypeDeclaration] Adds AddClosureParamTypeFromArgRector & AddClosureParamTypeFromArgClassStringRector rule Aug 25, 2024
@TomasVotruba
Copy link
Member

I'm looking into this 👍

@TomasVotruba
Copy link
Member

I've rebased and tidy up added few changes to improve type detection in #6258

Added few tips I use to write rules 🚀

Cool addition, thank you Peter 👏

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